RE: [PATCH v2 3/3] pinctrl: renesas: rza2: Add lock around pinctrl_generic{{add,remove}_group,{add,remove}_function}

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/3] pinctrl: renesas: rza2: Add lock around
> pinctrl_generic{{add,remove}_group,{add,remove}_function}
> 
> Hi Biju,
> 
> On Tue, Aug 15, 2023 at 9:56 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > The pinctrl group and function creation/remove calls expect caller to
> > take care of locking. Add lock around these functions.
> >
> > Fixes: b59d0e782706 ("pinctrl: Add RZ/A2 pin and gpio controller")
> > Cc: stable@xxxxxxxxxx
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> >  * No change.
> 
> Thanks for your patch!
> 
> > --- a/drivers/pinctrl/renesas/pinctrl-rza2.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rza2.c
> 
> > @@ -359,10 +361,13 @@ static int rza2_dt_node_to_map(struct pinctrl_dev
> *pctldev,
> >                 psel_val[i] = MUX_FUNC(value);
> >         }
> >
> > +       mutex_lock(&priv->mutex);
> >         /* Register a single pin group listing all the pins we read from
> DT */
> >         gsel = pinctrl_generic_add_group(pctldev, np->name, pins, npins,
> NULL);
> > -       if (gsel < 0)
> > +       if (gsel < 0) {
> > +               mutex_unlock(&priv->mutex);
> >                 return gsel;
> 
> Please do not mix "cleanup + return" and "goto cleanup" style in the same
> function.  I.e. goto new label below.

OK.

> 
> > +       }
> >
> >         /*
> >          * Register a single group function where the 'data' is an
> > array PSEL
> 
> > @@ -398,6 +404,7 @@ static int rza2_dt_node_to_map(struct pinctrl_dev
> > *pctldev,
> >
> >  remove_group:
> >         pinctrl_generic_remove_group(pctldev, gsel);
> 
> ^^ new label here.
OK.

> 
> > +       mutex_unlock(&priv->mutex);
> >
> >         dev_err(priv->dev, "Unable to parse DT node %s\n", np->name);
> >
> 
> The rest LGTM (unless I'm missing something critical in the real root
> cause of the issue?).

The root cause is race condition here in pctldev->num_groups and
radix_tree_insert adds with wrong group and selector entry.

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/core.c#L656

function 0: usb0, groups = [ usb0 ]
....
.....
function 9: spi1, groups = [ spi1 ]
function (null): COULD NOT GET GROUPS

Cheers,
Biju