-Wsizeof-array-div warnings in ethernet drivers

-Wsizeof-array-div warnings in ethernet drivers

[Nathan Chancellor]

Hi all,

Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
that tries to warn when sizeof(X) / sizeof(Y) does not compute the
number of elements in an array X (i.e., sizeof(Y) is wrong). See that
commit for more details:

https://github.com/llvm/llvm-project/commit/3240ad4ced0d3223149b72a4fc2a4d9b67589427

Some ethernet drivers have an instance of this warning due to receive
side scaling support:


../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
does not compute the number of elements in this array; element type is
'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
[-Wsizeof-array-div]
        unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
                                       ~~~~~~~~~~~~~~  ^
../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: note: place
parentheses around the 'sizeof(u32)' expression to silence this warning


../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: warning:
expression does not compute the number of elements in this array;
element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
int') [-Wsizeof-array-div]
        for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) {
                                ~~~~~~~~  ^
../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: note:
place parentheses around the 'sizeof(u32)' expression to silence this
warning


../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: warning:
expression does not compute the number of elements in this array;
element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
int') [-Wsizeof-array-div]
        unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
                                       ~~~~~~~~~~~~~~  ^
../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: note: place
parentheses around the 'sizeof(u32)' expression to silence this warning


What is the reasoning behind having the key being an array of u8s but
seemlingly converting it into an array of u32s? It's not immediately
apparent from reading over the code but I am not familiar with it so I
might be making a mistake. I assume this is intentional? If so, the
warning can be silenced and we'll send patches to do so but we want to
make sure we aren't actually papering over a mistake.

Cheers!
Nathan

RE: -Wsizeof-array-div warnings in ethernet drivers

[Jose Abreu]

From: Nathan Chancellor <natechancellor@gmail.com>
Date: Sep/17/2019, 08:32:32 (UTC+00:00)

> Hi all,
> 
> Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> commit for more details:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_llvm_llvm-2Dproject_commit_3240ad4ced0d3223149b72a4fc2a4d9b67589427&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=XFx3a9bS-B05voQh4LJquxHuP4TOowjC82tcMo4gPz0&s=NpzuPgHoSG2p4Mg6ko4MgHHV3TpTwEGm2XNTyw-s3XY&e= 
> 
> Some ethernet drivers have an instance of this warning due to receive
> side scaling support:
> 
> 
> ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
> does not compute the number of elements in this array; element type is
> 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
> [-Wsizeof-array-div]
>         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
>                                        ~~~~~~~~~~~~~~  ^
> ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: note: place
> parentheses around the 'sizeof(u32)' expression to silence this warning
> 
> 
> ../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: warning:
> expression does not compute the number of elements in this array;
> element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
> int') [-Wsizeof-array-div]
>         for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) {
>                                 ~~~~~~~~  ^
> ../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: note:
> place parentheses around the 'sizeof(u32)' expression to silence this
> warning
> 
> 
> ../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: warning:
> expression does not compute the number of elements in this array;
> element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
> int') [-Wsizeof-array-div]
>         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
>                                        ~~~~~~~~~~~~~~  ^
> ../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: note: place
> parentheses around the 'sizeof(u32)' expression to silence this warning
> 
> 
> What is the reasoning behind having the key being an array of u8s but
> seemlingly converting it into an array of u32s? It's not immediately
> apparent from reading over the code but I am not familiar with it so I
> might be making a mistake. I assume this is intentional? If so, the
> warning can be silenced and we'll send patches to do so but we want to
> make sure we aren't actually papering over a mistake.

This is because we write 32 bits at a time to the reg but internally the 
driver uses 8 bits to store the array. If you look at 
dwxgmac2_rss_configure() you'll see that cfg->key is casted to u32 which 
is the value we use in HW writes. Then the for loop just does the math 
to check how many u32's has to write.

---
Thanks,
Jose Miguel Abreu

RE: -Wsizeof-array-div warnings in ethernet drivers

[David Laight]

From: Jose Abreu
> Sent: 17 September 2019 08:59
> From: Nathan Chancellor <natechancellor@gmail.com>
> Date: Sep/17/2019, 08:32:32 (UTC+00:00)
> 
> > Hi all,
> >
> > Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> > that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> > number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> > commit for more details:
...
> > ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
> > does not compute the number of elements in this array; element type is
> > 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
> > [-Wsizeof-array-div]
> >         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
> >                                        ~~~~~~~~~~~~~~  ^
...
> > What is the reasoning behind having the key being an array of u8s but
> > seemlingly converting it into an array of u32s? It's not immediately
> > apparent from reading over the code but I am not familiar with it so I
> > might be making a mistake. I assume this is intentional? If so, the
> > warning can be silenced and we'll send patches to do so but we want to
> > make sure we aren't actually papering over a mistake.
> 
> This is because we write 32 bits at a time to the reg but internally the
> driver uses 8 bits to store the array. If you look at
> dwxgmac2_rss_configure() you'll see that cfg->key is casted to u32 which
> is the value we use in HW writes. Then the for loop just does the math
> to check how many u32's has to write.

That stinks of a possible misaligned data access.....

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: -Wsizeof-array-div warnings in ethernet drivers

[Jose Abreu]

From: David Laight <David.Laight@ACULAB.COM>
Date: Sep/17/2019, 11:36:21 (UTC+00:00)

> From: Jose Abreu
> > Sent: 17 September 2019 08:59
> > From: Nathan Chancellor <natechancellor@gmail.com>
> > Date: Sep/17/2019, 08:32:32 (UTC+00:00)
> > 
> > > Hi all,
> > >
> > > Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> > > that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> > > number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> > > commit for more details:
> ...
> > > ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
> > > does not compute the number of elements in this array; element type is
> > > 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
> > > [-Wsizeof-array-div]
> > >         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
> > >                                        ~~~~~~~~~~~~~~  ^
> ...
> > > What is the reasoning behind having the key being an array of u8s but
> > > seemlingly converting it into an array of u32s? It's not immediately
> > > apparent from reading over the code but I am not familiar with it so I
> > > might be making a mistake. I assume this is intentional? If so, the
> > > warning can be silenced and we'll send patches to do so but we want to
> > > make sure we aren't actually papering over a mistake.
> > 
> > This is because we write 32 bits at a time to the reg but internally the
> > driver uses 8 bits to store the array. If you look at
> > dwxgmac2_rss_configure() you'll see that cfg->key is casted to u32 which
> > is the value we use in HW writes. Then the for loop just does the math
> > to check how many u32's has to write.
> 
> That stinks of a possible misaligned data access.....

It's possible to happen only if structure field is not aligned. I guess 
I can either change all to u32 or just __align the field of the struct 
...

---
Thanks,
Jose Miguel Abreu

Re: -Wsizeof-array-div warnings in ethernet drivers

[Nick Desaulniers]

On Tue, Sep 17, 2019 at 6:27 AM Jose Abreu <Jose.Abreu@synopsys.com> wrote:
>
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Sep/17/2019, 11:36:21 (UTC+00:00)
>
> > From: Jose Abreu
> > > Sent: 17 September 2019 08:59
> > > From: Nathan Chancellor <natechancellor@gmail.com>
> > > Date: Sep/17/2019, 08:32:32 (UTC+00:00)
> > >
> > > > Hi all,
> > > >
> > > > Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> > > > that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> > > > number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> > > > commit for more details:
> > ...
> > > > ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
> > > > does not compute the number of elements in this array; element type is
> > > > 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
> > > > [-Wsizeof-array-div]
> > > >         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
> > > >                                        ~~~~~~~~~~~~~~  ^
> > ...
> > > > What is the reasoning behind having the key being an array of u8s but
> > > > seemlingly converting it into an array of u32s? It's not immediately
> > > > apparent from reading over the code but I am not familiar with it so I
> > > > might be making a mistake. I assume this is intentional? If so, the
> > > > warning can be silenced and we'll send patches to do so but we want to
> > > > make sure we aren't actually papering over a mistake.
> > >
> > > This is because we write 32 bits at a time to the reg but internally the
> > > driver uses 8 bits to store the array. If you look at
> > > dwxgmac2_rss_configure() you'll see that cfg->key is casted to u32 which
> > > is the value we use in HW writes. Then the for loop just does the math
> > > to check how many u32's has to write.
> >
> > That stinks of a possible misaligned data access.....
>
> It's possible to happen only if structure field is not aligned. I guess
> I can either change all to u32 or just __align the field of the struct

Would __aligning the struct still produce the warning?  It's good to
know that this case is intentional, but I would like to consider other
instances of it before we seriously consider turning it off.  If the
driver can be rewritten to just make use of u32, I would find that
preferrable.
-- 
Thanks,
~Nick Desaulniers

Re: -Wsizeof-array-div warnings in ethernet drivers

[Lendacky, Thomas]

On 9/17/19 2:32 AM, Nathan Chancellor wrote:
> Hi all,
> 
> Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> commit for more details:
> 
> https://github.com/llvm/llvm-project/commit/3240ad4ced0d3223149b72a4fc2a4d9b67589427
> 
> Some ethernet drivers have an instance of this warning due to receive
> side scaling support:
> 
> 
> ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression
> does not compute the number of elements in this array; element type is
> 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int')
> [-Wsizeof-array-div]
>         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
>                                        ~~~~~~~~~~~~~~  ^
> ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: note: place
> parentheses around the 'sizeof(u32)' expression to silence this warning
> 
> 
> ../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: warning:
> expression does not compute the number of elements in this array;
> element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
> int') [-Wsizeof-array-div]
>         for (i = 0; i < (sizeof(cfg->key) / sizeof(u32)); i++) {
>                                 ~~~~~~~~  ^
> ../drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:537:36: note:
> place parentheses around the 'sizeof(u32)' expression to silence this
> warning
> 
> 
> ../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: warning:
> expression does not compute the number of elements in this array;
> element type is 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned
> int') [-Wsizeof-array-div]
>         unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32);
>                                        ~~~~~~~~~~~~~~  ^
> ../drivers/net/ethernet/synopsys/dwc-xlgmac-hw.c:2329:49: note: place
> parentheses around the 'sizeof(u32)' expression to silence this warning
> 
> 
> What is the reasoning behind having the key being an array of u8s but
> seemlingly converting it into an array of u32s? It's not immediately

Probably because the ethtool functions that get and set the RSS key passes
the key buffer in as a u8 pointer.  Having said that, there's no reason
that any casting can't be done in the ethtool callback functions, if
needed (which I don't think it is, since the key buffer is used in
memcpy() calls), instead.

Thanks,
Tom

> apparent from reading over the code but I am not familiar with it so I
> might be making a mistake. I assume this is intentional? If so, the
> warning can be silenced and we'll send patches to do so but we want to
> make sure we aren't actually papering over a mistake.>
> Cheers!
> Nathan
>