diff mbox series

pinctrl: renesas: rzg2l: Use BIT_ULL for PIN_CFG_* macros

Message ID 20240617131511.160877-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series pinctrl: renesas: rzg2l: Use BIT_ULL for PIN_CFG_* macros | expand

Commit Message

Prabhakar June 17, 2024, 1:15 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Commit 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct
rzg2l_variable_pin_cfg") introduced a Smatch static checker warning:

    drivers/pinctrl/renesas/pinctrl-rzg2l.c:374 rzg2l_pinctrl_get_variable_pin_cfg()
    warn: was expecting a 64 bit value instead of '~((((1))) << (16))'

The function `rzg2l_pinctrl_get_variable_pin_cfg` attempts to mask out
`PIN_CFG_VARIABLE` using `BIT(16)`. However, since `pincfg` is a `u64`,
this inadvertently masks the high 32 bits as well, which is unintended
(on non 64-bit platforms). To correct this, `PIN_CFG_VARIABLE` should
be defined using `BIT_ULL(16)`, ensuring proper 64-bit masking.

To avoid such issues, update `PIN_CFG_*` macros to use `BIT_ULL()`.

Fixes: 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct rzg2l_variable_pin_cfg")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/5c1bf20b-7e94-4b06-95e5-da9f99750203@moroto.mountain/
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 44 ++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Geert Uytterhoeven June 17, 2024, 1:36 p.m. UTC | #1 
Hi Prabhakar,

On Mon, Jun 17, 2024 at 3:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Commit 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct
> rzg2l_variable_pin_cfg") introduced a Smatch static checker warning:
>
>     drivers/pinctrl/renesas/pinctrl-rzg2l.c:374 rzg2l_pinctrl_get_variable_pin_cfg()
>     warn: was expecting a 64 bit value instead of '~((((1))) << (16))'
>
> The function `rzg2l_pinctrl_get_variable_pin_cfg` attempts to mask out
> `PIN_CFG_VARIABLE` using `BIT(16)`. However, since `pincfg` is a `u64`,
> this inadvertently masks the high 32 bits as well, which is unintended
> (on non 64-bit platforms). To correct this, `PIN_CFG_VARIABLE` should
> be defined using `BIT_ULL(16)`, ensuring proper 64-bit masking.
>
> To avoid such issues, update `PIN_CFG_*` macros to use `BIT_ULL()`.
>
> Fixes: 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct rzg2l_variable_pin_cfg")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/5c1bf20b-7e94-4b06-95e5-da9f99750203@moroto.mountain/
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I would like to brainstorm a bit about this, though. See below...

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -41,28 +41,28 @@
>  #define MUX_FUNC_MASK          GENMASK(31, 16)
>
>  /* PIN capabilities */
> -#define PIN_CFG_IOLH_A                 BIT(0)
> -#define PIN_CFG_IOLH_B                 BIT(1)
> -#define PIN_CFG_SR                     BIT(2)
> -#define PIN_CFG_IEN                    BIT(3)
> -#define PIN_CFG_PUPD                   BIT(4)
> -#define PIN_CFG_IO_VMC_SD0             BIT(5)
> -#define PIN_CFG_IO_VMC_SD1             BIT(6)
> -#define PIN_CFG_IO_VMC_QSPI            BIT(7)
> -#define PIN_CFG_IO_VMC_ETH0            BIT(8)
> -#define PIN_CFG_IO_VMC_ETH1            BIT(9)
> -#define PIN_CFG_FILONOFF               BIT(10)
> -#define PIN_CFG_FILNUM                 BIT(11)
> -#define PIN_CFG_FILCLKSEL              BIT(12)
> -#define PIN_CFG_IOLH_C                 BIT(13)
> -#define PIN_CFG_SOFT_PS                        BIT(14)
> -#define PIN_CFG_OEN                    BIT(15)
> -#define PIN_CFG_VARIABLE               BIT(16)
> -#define PIN_CFG_NOGPIO_INT             BIT(17)
> -#define PIN_CFG_NOD                    BIT(18) /* N-ch Open Drain */
> -#define PIN_CFG_SMT                    BIT(19) /* Schmitt-trigger input control */
> -#define PIN_CFG_ELC                    BIT(20)
> -#define PIN_CFG_IOLH_RZV2H             BIT(21)
> +#define PIN_CFG_IOLH_A                 BIT_ULL(0)
> +#define PIN_CFG_IOLH_B                 BIT_ULL(1)
> +#define PIN_CFG_SR                     BIT_ULL(2)
> +#define PIN_CFG_IEN                    BIT_ULL(3)
> +#define PIN_CFG_PUPD                   BIT_ULL(4)
> +#define PIN_CFG_IO_VMC_SD0             BIT_ULL(5)
> +#define PIN_CFG_IO_VMC_SD1             BIT_ULL(6)
> +#define PIN_CFG_IO_VMC_QSPI            BIT_ULL(7)
> +#define PIN_CFG_IO_VMC_ETH0            BIT_ULL(8)
> +#define PIN_CFG_IO_VMC_ETH1            BIT_ULL(9)
> +#define PIN_CFG_FILONOFF               BIT_ULL(10)
> +#define PIN_CFG_FILNUM                 BIT_ULL(11)
> +#define PIN_CFG_FILCLKSEL              BIT_ULL(12)
> +#define PIN_CFG_IOLH_C                 BIT_ULL(13)
> +#define PIN_CFG_SOFT_PS                        BIT_ULL(14)
> +#define PIN_CFG_OEN                    BIT_ULL(15)
> +#define PIN_CFG_VARIABLE               BIT_ULL(16)

PIN_CFG_VARIABLE looks a bit misplaced here, in between all the flags
indicating actual capabilities of a pin.

What about relocating it to the "high" half, and moving it next to
RZG2L_SINGLE_PIN? Perhaps even renaming it to RZG2L_CFG_VARIABLE?

> +#define PIN_CFG_NOGPIO_INT             BIT_ULL(17)
> +#define PIN_CFG_NOD                    BIT_ULL(18)     /* N-ch Open Drain */
> +#define PIN_CFG_SMT                    BIT_ULL(19)     /* Schmitt-trigger input control */
> +#define PIN_CFG_ELC                    BIT_ULL(20)
> +#define PIN_CFG_IOLH_RZV2H             BIT_ULL(21)
>
>  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
>                                         (PIN_CFG_IOLH_##group | \

Then the other PIN_CFG_* definitions can keep on using BIT().
To make that safer, PIN_CFG_MASK should be restricted to 32-bit:

    -#define PIN_CFG_MASK                    GENMASK_ULL(46, 0)
    +#define PIN_CFG_MASK                    GENMASK_ULL(31, 0)

and several u64 variables can be changed to u32 again.

What do you think?

Gr{oetje,eeting}s,

                        Geert
Prabhakar June 17, 2024, 5:05 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Mon, Jun 17, 2024 at 2:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jun 17, 2024 at 3:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Commit 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct
> > rzg2l_variable_pin_cfg") introduced a Smatch static checker warning:
> >
> >     drivers/pinctrl/renesas/pinctrl-rzg2l.c:374 rzg2l_pinctrl_get_variable_pin_cfg()
> >     warn: was expecting a 64 bit value instead of '~((((1))) << (16))'
> >
> > The function `rzg2l_pinctrl_get_variable_pin_cfg` attempts to mask out
> > `PIN_CFG_VARIABLE` using `BIT(16)`. However, since `pincfg` is a `u64`,
> > this inadvertently masks the high 32 bits as well, which is unintended
> > (on non 64-bit platforms). To correct this, `PIN_CFG_VARIABLE` should
> > be defined using `BIT_ULL(16)`, ensuring proper 64-bit masking.
> >
> > To avoid such issues, update `PIN_CFG_*` macros to use `BIT_ULL()`.
> >
> > Fixes: 13a8cae6e561 ("pinctrl: renesas: rzg2l: Drop struct rzg2l_variable_pin_cfg")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/all/5c1bf20b-7e94-4b06-95e5-da9f99750203@moroto.mountain/
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> I would like to brainstorm a bit about this, though. See below...
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -41,28 +41,28 @@
> >  #define MUX_FUNC_MASK          GENMASK(31, 16)
> >
> >  /* PIN capabilities */
> > -#define PIN_CFG_IOLH_A                 BIT(0)
> > -#define PIN_CFG_IOLH_B                 BIT(1)
> > -#define PIN_CFG_SR                     BIT(2)
> > -#define PIN_CFG_IEN                    BIT(3)
> > -#define PIN_CFG_PUPD                   BIT(4)
> > -#define PIN_CFG_IO_VMC_SD0             BIT(5)
> > -#define PIN_CFG_IO_VMC_SD1             BIT(6)
> > -#define PIN_CFG_IO_VMC_QSPI            BIT(7)
> > -#define PIN_CFG_IO_VMC_ETH0            BIT(8)
> > -#define PIN_CFG_IO_VMC_ETH1            BIT(9)
> > -#define PIN_CFG_FILONOFF               BIT(10)
> > -#define PIN_CFG_FILNUM                 BIT(11)
> > -#define PIN_CFG_FILCLKSEL              BIT(12)
> > -#define PIN_CFG_IOLH_C                 BIT(13)
> > -#define PIN_CFG_SOFT_PS                        BIT(14)
> > -#define PIN_CFG_OEN                    BIT(15)
> > -#define PIN_CFG_VARIABLE               BIT(16)
> > -#define PIN_CFG_NOGPIO_INT             BIT(17)
> > -#define PIN_CFG_NOD                    BIT(18) /* N-ch Open Drain */
> > -#define PIN_CFG_SMT                    BIT(19) /* Schmitt-trigger input control */
> > -#define PIN_CFG_ELC                    BIT(20)
> > -#define PIN_CFG_IOLH_RZV2H             BIT(21)
> > +#define PIN_CFG_IOLH_A                 BIT_ULL(0)
> > +#define PIN_CFG_IOLH_B                 BIT_ULL(1)
> > +#define PIN_CFG_SR                     BIT_ULL(2)
> > +#define PIN_CFG_IEN                    BIT_ULL(3)
> > +#define PIN_CFG_PUPD                   BIT_ULL(4)
> > +#define PIN_CFG_IO_VMC_SD0             BIT_ULL(5)
> > +#define PIN_CFG_IO_VMC_SD1             BIT_ULL(6)
> > +#define PIN_CFG_IO_VMC_QSPI            BIT_ULL(7)
> > +#define PIN_CFG_IO_VMC_ETH0            BIT_ULL(8)
> > +#define PIN_CFG_IO_VMC_ETH1            BIT_ULL(9)
> > +#define PIN_CFG_FILONOFF               BIT_ULL(10)
> > +#define PIN_CFG_FILNUM                 BIT_ULL(11)
> > +#define PIN_CFG_FILCLKSEL              BIT_ULL(12)
> > +#define PIN_CFG_IOLH_C                 BIT_ULL(13)
> > +#define PIN_CFG_SOFT_PS                        BIT_ULL(14)
> > +#define PIN_CFG_OEN                    BIT_ULL(15)
> > +#define PIN_CFG_VARIABLE               BIT_ULL(16)
>
> PIN_CFG_VARIABLE looks a bit misplaced here, in between all the flags
> indicating actual capabilities of a pin.
>
> What about relocating it to the "high" half, and moving it next to
> RZG2L_SINGLE_PIN? Perhaps even renaming it to RZG2L_CFG_VARIABLE?
>
OK, I will rename it to RZG2L_CFG_VARIABLE and make it BIT(62) instead.

> > +#define PIN_CFG_NOGPIO_INT             BIT_ULL(17)
> > +#define PIN_CFG_NOD                    BIT_ULL(18)     /* N-ch Open Drain */
> > +#define PIN_CFG_SMT                    BIT_ULL(19)     /* Schmitt-trigger input control */
> > +#define PIN_CFG_ELC                    BIT_ULL(20)
> > +#define PIN_CFG_IOLH_RZV2H             BIT_ULL(21)
> >
> >  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
> >                                         (PIN_CFG_IOLH_##group | \
>
> Then the other PIN_CFG_* definitions can keep on using BIT().
> To make that safer, PIN_CFG_MASK should be restricted to 32-bit:
>
>     -#define PIN_CFG_MASK                    GENMASK_ULL(46, 0)
>     +#define PIN_CFG_MASK                    GENMASK_ULL(31, 0)
>
> and several u64 variables can be changed to u32 again.
>
> What do you think?
>
Agreed.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 32945d4c8dc0..d5eee98f6085 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -41,28 +41,28 @@ 
 #define MUX_FUNC_MASK		GENMASK(31, 16)
 
 /* PIN capabilities */
-#define PIN_CFG_IOLH_A			BIT(0)
-#define PIN_CFG_IOLH_B			BIT(1)
-#define PIN_CFG_SR			BIT(2)
-#define PIN_CFG_IEN			BIT(3)
-#define PIN_CFG_PUPD			BIT(4)
-#define PIN_CFG_IO_VMC_SD0		BIT(5)
-#define PIN_CFG_IO_VMC_SD1		BIT(6)
-#define PIN_CFG_IO_VMC_QSPI		BIT(7)
-#define PIN_CFG_IO_VMC_ETH0		BIT(8)
-#define PIN_CFG_IO_VMC_ETH1		BIT(9)
-#define PIN_CFG_FILONOFF		BIT(10)
-#define PIN_CFG_FILNUM			BIT(11)
-#define PIN_CFG_FILCLKSEL		BIT(12)
-#define PIN_CFG_IOLH_C			BIT(13)
-#define PIN_CFG_SOFT_PS			BIT(14)
-#define PIN_CFG_OEN			BIT(15)
-#define PIN_CFG_VARIABLE		BIT(16)
-#define PIN_CFG_NOGPIO_INT		BIT(17)
-#define PIN_CFG_NOD			BIT(18)	/* N-ch Open Drain */
-#define PIN_CFG_SMT			BIT(19)	/* Schmitt-trigger input control */
-#define PIN_CFG_ELC			BIT(20)
-#define PIN_CFG_IOLH_RZV2H		BIT(21)
+#define PIN_CFG_IOLH_A			BIT_ULL(0)
+#define PIN_CFG_IOLH_B			BIT_ULL(1)
+#define PIN_CFG_SR			BIT_ULL(2)
+#define PIN_CFG_IEN			BIT_ULL(3)
+#define PIN_CFG_PUPD			BIT_ULL(4)
+#define PIN_CFG_IO_VMC_SD0		BIT_ULL(5)
+#define PIN_CFG_IO_VMC_SD1		BIT_ULL(6)
+#define PIN_CFG_IO_VMC_QSPI		BIT_ULL(7)
+#define PIN_CFG_IO_VMC_ETH0		BIT_ULL(8)
+#define PIN_CFG_IO_VMC_ETH1		BIT_ULL(9)
+#define PIN_CFG_FILONOFF		BIT_ULL(10)
+#define PIN_CFG_FILNUM			BIT_ULL(11)
+#define PIN_CFG_FILCLKSEL		BIT_ULL(12)
+#define PIN_CFG_IOLH_C			BIT_ULL(13)
+#define PIN_CFG_SOFT_PS			BIT_ULL(14)
+#define PIN_CFG_OEN			BIT_ULL(15)
+#define PIN_CFG_VARIABLE		BIT_ULL(16)
+#define PIN_CFG_NOGPIO_INT		BIT_ULL(17)
+#define PIN_CFG_NOD			BIT_ULL(18)	/* N-ch Open Drain */
+#define PIN_CFG_SMT			BIT_ULL(19)	/* Schmitt-trigger input control */
+#define PIN_CFG_ELC			BIT_ULL(20)
+#define PIN_CFG_IOLH_RZV2H		BIT_ULL(21)
 
 #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
 					(PIN_CFG_IOLH_##group | \