[PATCH] spi: work around clang bug in SPI_BPW_RANGE_MASK()

[PATCH] spi: work around clang bug in SPI_BPW_RANGE_MASK()

[Arnd Bergmann]

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(-)

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;
-- 
2.20.0

Re: [PATCH] spi: work around clang bug in SPI_BPW_RANGE_MASK()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: work around clang bug in SPI_BPW_RANGE_MASK()

[Rasmus Villemoes]

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

Re: [PATCH] spi: work around clang bug in SPI_BPW_RANGE_MASK()

[Arnd Bergmann]

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