* -Wsizeof-array-div warnings in ethernet drivers
@ 2019-09-17 7:32 Nathan Chancellor
2019-09-17 7:58 ` Jose Abreu
2019-09-17 17:23 ` Lendacky, Thomas
0 siblings, 2 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-09-17 7:32 UTC (permalink / raw)
To: Tom Lendacky, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
David S. Miller
Cc: Nick Desaulniers, Ilie Halip, David Bolvansky, netdev,
clang-built-linux
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
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: -Wsizeof-array-div warnings in ethernet drivers
2019-09-17 7:32 -Wsizeof-array-div warnings in ethernet drivers Nathan Chancellor
@ 2019-09-17 7:58 ` Jose Abreu
2019-09-17 10:36 ` David Laight
2019-09-17 17:23 ` Lendacky, Thomas
1 sibling, 1 reply; 6+ messages in thread
From: Jose Abreu @ 2019-09-17 7:58 UTC (permalink / raw)
To: Nathan Chancellor, Tom Lendacky, Giuseppe Cavallaro,
Alexandre Torgue, David S. Miller
Cc: Nick Desaulniers, Ilie Halip, David Bolvansky,
netdev@vger.kernel.org, clang-built-linux@googlegroups.com
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
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: -Wsizeof-array-div warnings in ethernet drivers
2019-09-17 7:58 ` Jose Abreu
@ 2019-09-17 10:36 ` David Laight
2019-09-17 13:26 ` Jose Abreu
0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2019-09-17 10:36 UTC (permalink / raw)
To: 'Jose Abreu', Nathan Chancellor, Tom Lendacky,
Giuseppe Cavallaro, Alexandre Torgue, David S. Miller
Cc: Nick Desaulniers, Ilie Halip, David Bolvansky,
netdev@vger.kernel.org, clang-built-linux@googlegroups.com
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: -Wsizeof-array-div warnings in ethernet drivers
2019-09-17 10:36 ` David Laight
@ 2019-09-17 13:26 ` Jose Abreu
2019-09-17 16:00 ` Nick Desaulniers
0 siblings, 1 reply; 6+ messages in thread
From: Jose Abreu @ 2019-09-17 13:26 UTC (permalink / raw)
To: David Laight, 'Jose Abreu', Nathan Chancellor,
Tom Lendacky, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller
Cc: Nick Desaulniers, Ilie Halip, David Bolvansky,
netdev@vger.kernel.org, clang-built-linux@googlegroups.com
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: -Wsizeof-array-div warnings in ethernet drivers
2019-09-17 13:26 ` Jose Abreu
@ 2019-09-17 16:00 ` Nick Desaulniers
0 siblings, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-09-17 16:00 UTC (permalink / raw)
To: Jose Abreu
Cc: David Laight, Nathan Chancellor, Tom Lendacky, Giuseppe Cavallaro,
Alexandre Torgue, David S. Miller, Ilie Halip, David Bolvansky,
netdev@vger.kernel.org, clang-built-linux@googlegroups.com
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: -Wsizeof-array-div warnings in ethernet drivers
2019-09-17 7:32 -Wsizeof-array-div warnings in ethernet drivers Nathan Chancellor
2019-09-17 7:58 ` Jose Abreu
@ 2019-09-17 17:23 ` Lendacky, Thomas
1 sibling, 0 replies; 6+ messages in thread
From: Lendacky, Thomas @ 2019-09-17 17:23 UTC (permalink / raw)
To: Nathan Chancellor, Giuseppe Cavallaro, Alexandre Torgue,
Jose Abreu, David S. Miller
Cc: Nick Desaulniers, Ilie Halip, David Bolvansky,
netdev@vger.kernel.org, clang-built-linux@googlegroups.com
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-17 17:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-17 7:32 -Wsizeof-array-div warnings in ethernet drivers Nathan Chancellor
2019-09-17 7:58 ` Jose Abreu
2019-09-17 10:36 ` David Laight
2019-09-17 13:26 ` Jose Abreu
2019-09-17 16:00 ` Nick Desaulniers
2019-09-17 17:23 ` Lendacky, Thomas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).