-Wconstant-conversion in drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

-Wconstant-conversion in drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[Nathan Chancellor]

Hi all,

I was building s390 allyesconfig with clang and came across a curious
warning:

drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:580:41: warning:
implicit conversion from 'unsigned long' to 'int' changes value from
18446744073709551584 to -32 [-Wconstant-conversion]
        mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE;
                                             ~ ^~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/mvpp2/mvpp2.h:699:33: note: expanded from
macro 'MVPP2_BM_SHORT_PKT_SIZE'
#define MVPP2_BM_SHORT_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE)
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/mvpp2/mvpp2.h:634:30: note: expanded from
macro 'MVPP2_RX_MAX_PKT_SIZE'
        ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE)
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

As far as I understand it, the warning comes from the fact that
MVPP2_BM_SHORT_FRAME_SIZE is treated as size_t because
MVPP2_SKB_SHINFO_SIZE ultimately calls ALIGN with sizeof(struct
skb_shared_info), which has typeof called on it.

The implicit conversion probably is fine but it would be nice to take
care of the warning. I am not sure what would be the best way to do that
would be though. An explicit cast would take care of it, maybe in
MVPP2_SKB_SHINFO_SIZE since the actual value I see is 320, which is able
to be fit into type int easily.

Any comments would be appreciated, there does not appear to be a
dedicated maintainer of this driver according to get_maintainer.pl.

Cheers,
Nathan

Re: -Wconstant-conversion in drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[Nathan Chancellor]

On Thu, Apr 16, 2020 at 05:41:20PM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> I was building s390 allyesconfig with clang and came across a curious
> warning:
> 
> drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:580:41: warning:
> implicit conversion from 'unsigned long' to 'int' changes value from
> 18446744073709551584 to -32 [-Wconstant-conversion]
>         mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE;
>                                              ~ ^~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h:699:33: note: expanded from
> macro 'MVPP2_BM_SHORT_PKT_SIZE'
> #define MVPP2_BM_SHORT_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE)
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h:634:30: note: expanded from
> macro 'MVPP2_RX_MAX_PKT_SIZE'
>         ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE)
>                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
> 
> As far as I understand it, the warning comes from the fact that
> MVPP2_BM_SHORT_FRAME_SIZE is treated as size_t because
> MVPP2_SKB_SHINFO_SIZE ultimately calls ALIGN with sizeof(struct
> skb_shared_info), which has typeof called on it.
> 
> The implicit conversion probably is fine but it would be nice to take
> care of the warning. I am not sure what would be the best way to do that
> would be though. An explicit cast would take care of it, maybe in
> MVPP2_SKB_SHINFO_SIZE since the actual value I see is 320, which is able
> to be fit into type int easily.
> 
> Any comments would be appreciated, there does not appear to be a
> dedicated maintainer of this driver according to get_maintainer.pl.

Sorry for the necrobump, I am doing a bug scrub and it seems like this
driver now has maintainers so keying them in in case they have any
comments/suggestions.

Cheers,
Nathan

Re: -Wconstant-conversion in drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[Marcin Wojtas]

Hi Nathan,


śr., 28 kwi 2021 o 20:04 Nathan Chancellor <nathan@kernel.org> napisał(a):
>
> On Thu, Apr 16, 2020 at 05:41:20PM -0700, Nathan Chancellor wrote:
> > Hi all,
> >
> > I was building s390 allyesconfig with clang and came across a curious
> > warning:
> >
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:580:41: warning:
> > implicit conversion from 'unsigned long' to 'int' changes value from
> > 18446744073709551584 to -32 [-Wconstant-conversion]
> >         mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE;
> >                                              ~ ^~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/ethernet/marvell/mvpp2/mvpp2.h:699:33: note: expanded from
> > macro 'MVPP2_BM_SHORT_PKT_SIZE'
> > #define MVPP2_BM_SHORT_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE)
> >                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/ethernet/marvell/mvpp2/mvpp2.h:634:30: note: expanded from
> > macro 'MVPP2_RX_MAX_PKT_SIZE'
> >         ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE)
> >                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > 1 warning generated.
> >
> > As far as I understand it, the warning comes from the fact that
> > MVPP2_BM_SHORT_FRAME_SIZE is treated as size_t because
> > MVPP2_SKB_SHINFO_SIZE ultimately calls ALIGN with sizeof(struct
> > skb_shared_info), which has typeof called on it.
> >
> > The implicit conversion probably is fine but it would be nice to take
> > care of the warning. I am not sure what would be the best way to do that
> > would be though. An explicit cast would take care of it, maybe in
> > MVPP2_SKB_SHINFO_SIZE since the actual value I see is 320, which is able
> > to be fit into type int easily.
> >
> > Any comments would be appreciated, there does not appear to be a
> > dedicated maintainer of this driver according to get_maintainer.pl.
>
> Sorry for the necrobump, I am doing a bug scrub and it seems like this
> driver now has maintainers so keying them in in case they have any
> comments/suggestions.
>

Thank you for your interest. Are you still reproducing the issue? With
clang 10.0.0 the compilation passes in my setup:
$ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make CC=clang
drivers/net/ethernet/marvell/mvpp2/
  SYNC    include/config/auto.conf.cmd
  CC      scripts/mod/empty.o
  MKELF   scripts/mod/elfconfig.h
  HOSTCC  scripts/mod/modpost.o
  CC      scripts/mod/devicetable-offsets.s
  HOSTCC  scripts/mod/file2alias.o
  HOSTCC  scripts/mod/sumversion.o
  HOSTLD  scripts/mod/modpost
  CC      kernel/bounds.s
  CC      arch/arm64/kernel/asm-offsets.s
  UPD     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  LDS     arch/arm64/kernel/vdso/vdso.lds
  CC      arch/arm64/kernel/vdso/vgettimeofday.o
  AS      arch/arm64/kernel/vdso/note.o
  AS      arch/arm64/kernel/vdso/sigreturn.o
  LD      arch/arm64/kernel/vdso/vdso.so.dbg
  VDSOSYM include/generated/vdso-offsets.h
  OBJCOPY arch/arm64/kernel/vdso/vdso.so
  CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_main.o
  CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.o
  CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.o
  CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.o
  AR      drivers/net/ethernet/marvell/mvpp2/built-in.a

Best regards,
Marcin

Re: -Wconstant-conversion in drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[Nathan Chancellor]

Hi Marcin,

On Thu, Apr 29, 2021 at 09:08:13AM +0200, Marcin Wojtas wrote:
> Hi Nathan,
> 
> 
> śr., 28 kwi 2021 o 20:04 Nathan Chancellor <nathan@kernel.org> napisał(a):
> >
> > On Thu, Apr 16, 2020 at 05:41:20PM -0700, Nathan Chancellor wrote:
> > > Hi all,
> > >
> > > I was building s390 allyesconfig with clang and came across a curious
> > > warning:
> > >
> > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:580:41: warning:
> > > implicit conversion from 'unsigned long' to 'int' changes value from
> > > 18446744073709551584 to -32 [-Wconstant-conversion]
> > >         mvpp2_pools[MVPP2_BM_SHORT].pkt_size = MVPP2_BM_SHORT_PKT_SIZE;
> > >                                              ~ ^~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/net/ethernet/marvell/mvpp2/mvpp2.h:699:33: note: expanded from
> > > macro 'MVPP2_BM_SHORT_PKT_SIZE'
> > > #define MVPP2_BM_SHORT_PKT_SIZE MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE)
> > >                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > drivers/net/ethernet/marvell/mvpp2/mvpp2.h:634:30: note: expanded from
> > > macro 'MVPP2_RX_MAX_PKT_SIZE'
> > >         ((total_size) - NET_SKB_PAD - MVPP2_SKB_SHINFO_SIZE)
> > >                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
> > > 1 warning generated.
> > >
> > > As far as I understand it, the warning comes from the fact that
> > > MVPP2_BM_SHORT_FRAME_SIZE is treated as size_t because
> > > MVPP2_SKB_SHINFO_SIZE ultimately calls ALIGN with sizeof(struct
> > > skb_shared_info), which has typeof called on it.
> > >
> > > The implicit conversion probably is fine but it would be nice to take
> > > care of the warning. I am not sure what would be the best way to do that
> > > would be though. An explicit cast would take care of it, maybe in
> > > MVPP2_SKB_SHINFO_SIZE since the actual value I see is 320, which is able
> > > to be fit into type int easily.
> > >
> > > Any comments would be appreciated, there does not appear to be a
> > > dedicated maintainer of this driver according to get_maintainer.pl.
> >
> > Sorry for the necrobump, I am doing a bug scrub and it seems like this
> > driver now has maintainers so keying them in in case they have any
> > comments/suggestions.
> >
> 
> Thank you for your interest. Are you still reproducing the issue? With
> clang 10.0.0 the compilation passes in my setup:
> $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make CC=clang
> drivers/net/ethernet/marvell/mvpp2/
>   SYNC    include/config/auto.conf.cmd
>   CC      scripts/mod/empty.o
>   MKELF   scripts/mod/elfconfig.h
>   HOSTCC  scripts/mod/modpost.o
>   CC      scripts/mod/devicetable-offsets.s
>   HOSTCC  scripts/mod/file2alias.o
>   HOSTCC  scripts/mod/sumversion.o
>   HOSTLD  scripts/mod/modpost
>   CC      kernel/bounds.s
>   CC      arch/arm64/kernel/asm-offsets.s
>   UPD     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CALL    scripts/atomic/check-atomics.sh
>   LDS     arch/arm64/kernel/vdso/vdso.lds
>   CC      arch/arm64/kernel/vdso/vgettimeofday.o
>   AS      arch/arm64/kernel/vdso/note.o
>   AS      arch/arm64/kernel/vdso/sigreturn.o
>   LD      arch/arm64/kernel/vdso/vdso.so.dbg
>   VDSOSYM include/generated/vdso-offsets.h
>   OBJCOPY arch/arm64/kernel/vdso/vdso.so
>   CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_main.o
>   CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.o
>   CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.o
>   CC      drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.o
>   AR      drivers/net/ethernet/marvell/mvpp2/built-in.a

My apologies for taking a while to get back to you, it took me a while
to understand what is going on here.

On s390, MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE) evaluates to
704 - 224 - 512 = -32, which is implicitly converted to size_t or
unsigned long because of the

SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

resulting in the super large number that clang shows above. Then that
large number is converted back into int, resulting in the same value but
with the warning. The 512 comes from the fact that L1_CACHE_BYTES on
s390 is 256, resulting in a large aligned value.

On arm64, MVPP2_RX_MAX_PKT_SIZE(MVPP2_BM_SHORT_FRAME_SIZE) evaluates to
704 - 224 - 320, which is 160, which is the same value signed or
unsigned.

I understand that this probably does not matter in practice because this
driver does not run on anything other than Marvell SoCs but it might be
nice to fix the warning :) this patch below seems like a reasonable
option, let me know what you think.

Cheers,
Nathan

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index 8edba5ea90f0..db23da9a0658 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -832,7 +832,7 @@
 
 /* RX buffer constants */
 #define MVPP2_SKB_SHINFO_SIZE \
-	SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
+	SKB_DATA_ALIGN((ssize_t)sizeof(struct skb_shared_info))
 
 #define MVPP2_RX_PKT_SIZE(mtu) \
 	ALIGN((mtu) + MVPP2_MH_SIZE + MVPP2_VLAN_TAG_LEN + \