-Wsizeof-array-div warnings in ethernet drivers
Subsystems (2)
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
From: Nathan Chancellor <[email protected]>
Date: Sep/17/2019, 08:32:32 (UTC+00:00)
Quoted 44 lines
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:
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
Quoted 6 lines
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
From: Jose Abreu
Quoted 10 lines
Sent: 17 September 2019 08:59
From: Nathan Chancellor <[email protected]>
Date: Sep/17/2019, 08:32:32 (UTC+00:00)Quoted 6 lines
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:
...
Quoted 6 lines
Quoted 6 lines
../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); ~~~~~~~~~~~~~~ ^
...
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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 thatcfg->keyis 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)
From: David Laight <[email protected]>
Date: Sep/17/2019, 11:36:21 (UTC+00:00)
Quoted 26 lines
From: Jose Abreu
Quoted 10 lines
Sent: 17 September 2019 08:59
From: Nathan Chancellor <[email protected]>
Date: Sep/17/2019, 08:32:32 (UTC+00:00)Quoted 6 lines
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:
...
Quoted 6 lines
Quoted 6 lines
../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); ~~~~~~~~~~~~~~ ^...
Quoted 7 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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 thatcfg->keyis 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
Quoted 36 lines · Jose Abreu
From: David Laight <[email protected]>
Date: Sep/17/2019, 11:36:21 (UTC+00:00)Quoted 32 lines
From: Jose Abreu
Quoted 10 lines
Sent: 17 September 2019 08:59
From: Nathan Chancellor <[email protected]>
Date: Sep/17/2019, 08:32:32 (UTC+00:00)Quoted 6 lines
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:
...
Quoted 6 lines
Quoted 6 lines
../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); ~~~~~~~~~~~~~~ ^...
Quoted 12 lines
Quoted 6 lines
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 thatcfg->keyis 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.
Quoted 4 lines
Quoted 1 line
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
__alignthe 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.
Signature · 2 lines
Thanks, ~Nick Desaulniers
Quoted 38 lines · 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]
Quoted 8 lines
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 warningWhat 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
Quoted 7 lines
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