| Message ID | 20190307105619.2780591-1-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: work around clang bug in SPI_BPW_RANGE_MASK() | expand |
On Thu, Mar 07, 2019 at 11:56:07AM +0100, Arnd Bergmann wrote: > Link: https://bugs.llvm.org/show_bug.cgi?id=38789 > #define SPI_BPW_MASK(bits) BIT((bits) - 1) > -#define SPI_BIT_MASK(bits) (((bits) == 32) ? ~0U : (BIT(bits) - 1)) > -#define SPI_BPW_RANGE_MASK(min, max) (SPI_BIT_MASK(max) - SPI_BIT_MASK(min - 1)) > +#define SPI_BIT_MASK(bits) ((BIT((bits) - 1) << 1) - 1) > +#define SPI_BPW_RANGE_MASK(min, max) (SPI_BIT_MASK(max) - (BIT((min) - 1) - 1)) Can we have a comment that this is for the clang warning please so nobody goes in and simplifies the code?
On 07/03/2019 11.56, Arnd Bergmann wrote: > Clang-8 evaluates both sides of a ?: expression to check for > valid arithmetic even in the side that is never taken. This > results in a build warning: > > drivers/spi/spi-sh-msiof.c:1052:24: error: shift count >= width of type [-Werror,-Wshift-count-overflow] > .bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32), > ^~~~~~~~~~~~~~~~~~~~~~~~~ > > Change it to shift one less than we want, and then shift one > more bit afterwards. This should give the correct result for > all valid input, since it has to be in the range 1..32 anyway. Why not use GENMASK which is provided by the same header that #defines BIT? Rasmus
On Thu, Mar 7, 2019 at 2:09 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > On 07/03/2019 11.56, Arnd Bergmann wrote: > > Clang-8 evaluates both sides of a ?: expression to check for > > valid arithmetic even in the side that is never taken. This > > results in a build warning: > > > > drivers/spi/spi-sh-msiof.c:1052:24: error: shift count >= width of type [-Werror,-Wshift-count-overflow] > > .bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32), > > ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Change it to shift one less than we want, and then shift one > > more bit afterwards. This should give the correct result for > > all valid input, since it has to be in the range 1..32 anyway. > > Why not use GENMASK which is provided by the same header that #defines BIT? It might be an options, but - I had not thought of it - It looks like it would have the same problem with shifting right by 32 bits (?) - it seems to have slightly different semantics from SPI_BPW_RANGE_MASK(), counting the bits from 0 instead of 1. I tried this version now, which doesn't produce any warnings as far as I can tell, but I'm not convinced that it's actually correct. Can you have a look? #define SPI_BPW_RANGE_MASK(min, max) GENMASK((min) - 1, (max) - 1) Arnd
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 662b336aa2e4..758b6287cad6 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -444,8 +444,8 @@ struct spi_controller { /* bitmask of supported bits_per_word for transfers */ u32 bits_per_word_mask; #define SPI_BPW_MASK(bits) BIT((bits) - 1) -#define SPI_BIT_MASK(bits) (((bits) == 32) ? ~0U : (BIT(bits) - 1)) -#define SPI_BPW_RANGE_MASK(min, max) (SPI_BIT_MASK(max) - SPI_BIT_MASK(min - 1)) +#define SPI_BIT_MASK(bits) ((BIT((bits) - 1) << 1) - 1) +#define SPI_BPW_RANGE_MASK(min, max) (SPI_BIT_MASK(max) - (BIT((min) - 1) - 1)) /* limits on transfer speed */ u32 min_speed_hz;
Clang-8 evaluates both sides of a ?: expression to check for valid arithmetic even in the side that is never taken. This results in a build warning: drivers/spi/spi-sh-msiof.c:1052:24: error: shift count >= width of type [-Werror,-Wshift-count-overflow] .bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 32), ^~~~~~~~~~~~~~~~~~~~~~~~~ Change it to shift one less than we want, and then shift one more bit afterwards. This should give the correct result for all valid input, since it has to be in the range 1..32 anyway. Link: https://bugs.llvm.org/show_bug.cgi?id=38789 Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- include/linux/spi/spi.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.20.0