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 <[email protected]>
---
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
Hi Wolfram,
On Tue, Jan 30, 2024 at 10:42 AM Wolfram Sang
<[email protected]> 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 <[email protected]>
> --- 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-m68korg
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
> 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
<[email protected]> 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-m68korg
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
> > 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
<[email protected]> 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 <[email protected]>
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-m68korg
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
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