[PATCH] spi: sh-msiof: avoid integer overflow in constants

[PATCH] spi: sh-msiof: avoid integer overflow in constants

[Wolfram Sang]

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

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Geert Uytterhoeven]

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

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Wolfram Sang]

[-- 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 --]

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Geert Uytterhoeven]

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

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Wolfram Sang]

[-- 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 --]

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Geert Uytterhoeven]

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

Re: [PATCH] spi: sh-msiof: avoid integer overflow in constants

[Mark Brown]

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