| Message ID | 20240130094053.10672-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6500ad28fd5d67d5ca0fee9da73c463090842440 |
| Headers | show |
| Series | spi: sh-msiof: avoid integer overflow in constants | expand |
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
> 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...).
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
> > But they don't match, so we can't unify them? > > I stand corrected... So, the patch is good as-is?
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
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
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)
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(-)