Hi Geert,
Thank you for the review.
On Mon, Jun 17, 2024 at 2:36 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jun 17, 2024 at 3:15 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/5c1bf20b-7e94-4b06-95e5-da9f99750203@moroto.mountain/
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> 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
