* [PATCH] spi: sh-msiof: avoid integer overflow in constants @ 2024-01-30 9:40 Wolfram Sang 2024-01-30 10:14 ` Geert Uytterhoeven 2024-01-30 19:38 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Wolfram Sang @ 2024-01-30 9:40 UTC (permalink / raw) To: linux-renesas-soc; +Cc: Wolfram Sang, Mark Brown, linux-spi, linux-kernel cppcheck rightfully warned: drivers/spi/spi-sh-msiof.c:792:28: warning: Signed integer overflow for expression '7<<29'. [integerOverflow] sh_msiof_write(p, SIFCTR, SIFCTR_TFWM_1 | SIFCTR_RFWM_1); Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/spi/spi-sh-msiof.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c index cfc3b1ddbd22..6f12e4fb2e2e 100644 --- a/drivers/spi/spi-sh-msiof.c +++ b/drivers/spi/spi-sh-msiof.c @@ -136,14 +136,14 @@ struct sh_msiof_spi_priv { /* SIFCTR */ #define SIFCTR_TFWM_MASK GENMASK(31, 29) /* Transmit FIFO Watermark */ -#define SIFCTR_TFWM_64 (0 << 29) /* Transfer Request when 64 empty stages */ -#define SIFCTR_TFWM_32 (1 << 29) /* Transfer Request when 32 empty stages */ -#define SIFCTR_TFWM_24 (2 << 29) /* Transfer Request when 24 empty stages */ -#define SIFCTR_TFWM_16 (3 << 29) /* Transfer Request when 16 empty stages */ -#define SIFCTR_TFWM_12 (4 << 29) /* Transfer Request when 12 empty stages */ -#define SIFCTR_TFWM_8 (5 << 29) /* Transfer Request when 8 empty stages */ -#define SIFCTR_TFWM_4 (6 << 29) /* Transfer Request when 4 empty stages */ -#define SIFCTR_TFWM_1 (7 << 29) /* Transfer Request when 1 empty stage */ +#define SIFCTR_TFWM_64 (0UL << 29) /* Transfer Request when 64 empty stages */ +#define SIFCTR_TFWM_32 (1UL << 29) /* Transfer Request when 32 empty stages */ +#define SIFCTR_TFWM_24 (2UL << 29) /* Transfer Request when 24 empty stages */ +#define SIFCTR_TFWM_16 (3UL << 29) /* Transfer Request when 16 empty stages */ +#define SIFCTR_TFWM_12 (4UL << 29) /* Transfer Request when 12 empty stages */ +#define SIFCTR_TFWM_8 (5UL << 29) /* Transfer Request when 8 empty stages */ +#define SIFCTR_TFWM_4 (6UL << 29) /* Transfer Request when 4 empty stages */ +#define SIFCTR_TFWM_1 (7UL << 29) /* Transfer Request when 1 empty stage */ #define SIFCTR_TFUA_MASK GENMASK(26, 20) /* Transmit FIFO Usable Area */ #define SIFCTR_TFUA_SHIFT 20 #define SIFCTR_TFUA(i) ((i) << SIFCTR_TFUA_SHIFT) -- 2.39.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 9:40 [PATCH] spi: sh-msiof: avoid integer overflow in constants Wolfram Sang @ 2024-01-30 10:14 ` Geert Uytterhoeven 2024-01-30 11:26 ` Wolfram Sang 2024-01-30 19:38 ` Mark Brown 1 sibling, 1 reply; 7+ messages in thread From: Geert Uytterhoeven @ 2024-01-30 10:14 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-renesas-soc, Mark Brown, linux-spi, linux-kernel Hi Wolfram, On Tue, Jan 30, 2024 at 10:42 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > cppcheck rightfully warned: > > drivers/spi/spi-sh-msiof.c:792:28: warning: Signed integer overflow for expression '7<<29'. [integerOverflow] > sh_msiof_write(p, SIFCTR, SIFCTR_TFWM_1 | SIFCTR_RFWM_1); > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- a/drivers/spi/spi-sh-msiof.c > +++ b/drivers/spi/spi-sh-msiof.c > @@ -136,14 +136,14 @@ struct sh_msiof_spi_priv { > > /* SIFCTR */ > #define SIFCTR_TFWM_MASK GENMASK(31, 29) /* Transmit FIFO Watermark */ > -#define SIFCTR_TFWM_64 (0 << 29) /* Transfer Request when 64 empty stages */ > -#define SIFCTR_TFWM_32 (1 << 29) /* Transfer Request when 32 empty stages */ > -#define SIFCTR_TFWM_24 (2 << 29) /* Transfer Request when 24 empty stages */ > -#define SIFCTR_TFWM_16 (3 << 29) /* Transfer Request when 16 empty stages */ > -#define SIFCTR_TFWM_12 (4 << 29) /* Transfer Request when 12 empty stages */ > -#define SIFCTR_TFWM_8 (5 << 29) /* Transfer Request when 8 empty stages */ > -#define SIFCTR_TFWM_4 (6 << 29) /* Transfer Request when 4 empty stages */ > -#define SIFCTR_TFWM_1 (7 << 29) /* Transfer Request when 1 empty stage */ > +#define SIFCTR_TFWM_64 (0UL << 29) /* Transfer Request when 64 empty stages */ > +#define SIFCTR_TFWM_32 (1UL << 29) /* Transfer Request when 32 empty stages */ > +#define SIFCTR_TFWM_24 (2UL << 29) /* Transfer Request when 24 empty stages */ > +#define SIFCTR_TFWM_16 (3UL << 29) /* Transfer Request when 16 empty stages */ > +#define SIFCTR_TFWM_12 (4UL << 29) /* Transfer Request when 12 empty stages */ > +#define SIFCTR_TFWM_8 (5UL << 29) /* Transfer Request when 8 empty stages */ > +#define SIFCTR_TFWM_4 (6UL << 29) /* Transfer Request when 4 empty stages */ > +#define SIFCTR_TFWM_1 (7UL << 29) /* Transfer Request when 1 empty stage */ > #define SIFCTR_TFUA_MASK GENMASK(26, 20) /* Transmit FIFO Usable Area */ > #define SIFCTR_TFUA_SHIFT 20 > #define SIFCTR_TFUA(i) ((i) << SIFCTR_TFUA_SHIFT) There is a similar issue with the SIFCTR_RFWM_* definitions below, but these don't trigger, as no data is shifted into the sign bit. What about unifying the individual SIFCTR_?FWM_[0-9]* definitions into SIFCTR_xFWM_[0-9]* instead, and using the bitfield helpers in its sole user? - sh_msiof_write(p, SIFCTR, SIFCTR_TFWM_1 | SIFCTR_RFWM_1); + sh_msiof_write(p, SIFCTR, + FIELD_PREP(SIFCTR_TFWM_MASK, SIFCTR_xFWM_1) | + FIELD_PREP(SIFCTR_RFWM_MASK, SIFCTR_xFWM_1); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 10:14 ` Geert Uytterhoeven @ 2024-01-30 11:26 ` Wolfram Sang 2024-01-30 11:39 ` Geert Uytterhoeven 0 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2024-01-30 11:26 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc, Mark Brown, linux-spi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 458 bytes --] > What about unifying the individual SIFCTR_?FWM_[0-9]* definitions > into SIFCTR_xFWM_[0-9]* instead, and using the bitfield helpers in its > sole user? But they don't match, so we can't unify them? #define SIFCTR_TFWM_1 (7UL << 29) /* Transfer Request when 1 empty stage */ vs #define SIFCTR_RFWM_1 (0 << 13) /* Transfer Request when 1 valid stages */ Also, the steps don't match (1, 4, 8, 12..) vs (1, 4, 8, 16...). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 11:26 ` Wolfram Sang @ 2024-01-30 11:39 ` Geert Uytterhoeven 2024-01-30 13:40 ` Wolfram Sang 0 siblings, 1 reply; 7+ messages in thread From: Geert Uytterhoeven @ 2024-01-30 11:39 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-renesas-soc, Mark Brown, linux-spi, linux-kernel Hi Wolfram, On Tue, Jan 30, 2024 at 12:26 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > What about unifying the individual SIFCTR_?FWM_[0-9]* definitions > > into SIFCTR_xFWM_[0-9]* instead, and using the bitfield helpers in its > > sole user? > > But they don't match, so we can't unify them? > > #define SIFCTR_TFWM_1 (7UL << 29) /* Transfer Request when 1 empty stage */ > > vs > > #define SIFCTR_RFWM_1 (0 << 13) /* Transfer Request when 1 valid stages */ > > Also, the steps don't match (1, 4, 8, 12..) vs (1, 4, 8, 16...). I stand corrected... /me looks envious for a brown paper bag... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 11:39 ` Geert Uytterhoeven @ 2024-01-30 13:40 ` Wolfram Sang 2024-01-30 14:09 ` Geert Uytterhoeven 0 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2024-01-30 13:40 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc, Mark Brown, linux-spi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 115 bytes --] > > But they don't match, so we can't unify them? > > I stand corrected... So, the patch is good as-is? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 13:40 ` Wolfram Sang @ 2024-01-30 14:09 ` Geert Uytterhoeven 0 siblings, 0 replies; 7+ messages in thread From: Geert Uytterhoeven @ 2024-01-30 14:09 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-renesas-soc, Mark Brown, linux-spi, linux-kernel Hi Wolfram, On Tue, Jan 30, 2024 at 2:40 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > But they don't match, so we can't unify them? > > > > I stand corrected... > > So, the patch is good as-is? Yeah... Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Still, it might make sense to apply the same change to the SIFCTR_RFWM_* definitions. However, that would still leave us with inconsistencies with other bitfield definitions in the file... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants 2024-01-30 9:40 [PATCH] spi: sh-msiof: avoid integer overflow in constants Wolfram Sang 2024-01-30 10:14 ` Geert Uytterhoeven @ 2024-01-30 19:38 ` Mark Brown 1 sibling, 0 replies; 7+ messages in thread From: Mark Brown @ 2024-01-30 19:38 UTC (permalink / raw) To: linux-renesas-soc, Wolfram Sang; +Cc: linux-spi, linux-kernel On Tue, 30 Jan 2024 10:40:53 +0100, Wolfram Sang wrote: > cppcheck rightfully warned: > > drivers/spi/spi-sh-msiof.c:792:28: warning: Signed integer overflow for expression '7<<29'. [integerOverflow] > sh_msiof_write(p, SIFCTR, SIFCTR_TFWM_1 | SIFCTR_RFWM_1); > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: sh-msiof: avoid integer overflow in constants commit: 6500ad28fd5d67d5ca0fee9da73c463090842440 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-30 19:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-30 9:40 [PATCH] spi: sh-msiof: avoid integer overflow in constants Wolfram Sang 2024-01-30 10:14 ` Geert Uytterhoeven 2024-01-30 11:26 ` Wolfram Sang 2024-01-30 11:39 ` Geert Uytterhoeven 2024-01-30 13:40 ` Wolfram Sang 2024-01-30 14:09 ` Geert Uytterhoeven 2024-01-30 19:38 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox