Re: [PATCH 08/10] mfd: atmel-smc: Silence comparison of unsigned expression < 0 is always false warning (W=1)

[Alexandre Belloni]

Hi,

On 25/06/2020 07:46:17+0100, Lee Jones wrote:
> GENMASK and it's callees conduct checking to ensure the passed
> parameters are valid.  One of those checks is for '< 0'.  So if an
> unsigned value is passed, in an invalid comparison takes place.
> 
> Judging from the current code, it looks as though 'unsigned int'
> is the correct type to use, so simply cast these small values
> with no chance of being false negative to signed int for
> comparison/error checking purposes.
> 

I've been thinking about that one but shouldn't the proper fix be in
GENMASK? My understanding is that this happens because l is 0 and I
don't think GENMASK would ever expect negative number. What about simply
checking that h != l when l is 0?

> Squashes the following W=1 warnings:
> 
>  In file included from /home/lee/projects/linux/kernel/include/linux/bits.h:23,
>  from /home/lee/projects/linux/kernel/include/linux/bitops.h:5,
>  from /home/lee/projects/linux/kernel/include/linux/kernel.h:12,
>  from /home/lee/projects/linux/kernel/include/linux/mfd/syscon/atmel-smc.h:14,
>  from /home/lee/projects/linux/kernel/drivers/mfd/atmel-smc.c:11:
>  /home/lee/projects/linux/kernel/drivers/mfd/atmel-smc.c: In function âatmel_smc_cs_encode_ncyclesâ:
>  /home/lee/projects/linux/kernel/include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>  26 | __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>  | ^
>  /home/lee/projects/linux/kernel/include/linux/build_bug.h:16:62: note: in definition of macro âBUILD_BUG_ON_ZEROâ
>  16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>  | ^
>  /home/lee/projects/linux/kernel/include/linux/bits.h:39:3: note: in expansion of macro âGENMASK_INPUT_CHECKâ
>  39 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>  | ^~~~~~~~~~~~~~~~~~~
>  /home/lee/projects/linux/kernel/drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro âGENMASKâ
>  49 | unsigned int lsbmask = GENMASK(msbpos - 1, 0);
>  | ^~~~~~~
>  /home/lee/projects/linux/kernel/include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>  26 | __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>  | ^
>  /home/lee/projects/linux/kernel/include/linux/build_bug.h:16:62: note: in definition of macro âBUILD_BUG_ON_ZEROâ
>  16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>  | ^
>  /home/lee/projects/linux/kernel/include/linux/bits.h:39:3: note: in expansion of macro âGENMASK_INPUT_CHECKâ
>  39 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>  | ^~~~~~~~~~~~~~~~~~~
> 
> Cc: <stable@xxxxxxxxxxxxxxx>
> Cc: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Cc: Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> ---
>  drivers/mfd/atmel-smc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/atmel-smc.c b/drivers/mfd/atmel-smc.c
> index 1fa2ec950e7df..17bbe9d1fa740 100644
> --- a/drivers/mfd/atmel-smc.c
> +++ b/drivers/mfd/atmel-smc.c
> @@ -46,8 +46,8 @@ static int atmel_smc_cs_encode_ncycles(unsigned int ncycles,
>  				       unsigned int msbfactor,
>  				       unsigned int *encodedval)
>  {
> -	unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> -	unsigned int msbmask = GENMASK(msbwidth - 1, 0);
> +	unsigned int lsbmask = GENMASK((int)msbpos - 1, 0);
> +	unsigned int msbmask = GENMASK((int)msbwidth - 1, 0);
>  	unsigned int msb, lsb;
>  	int ret = 0;
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com