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
