Re: [PATCH] pinctrl: core: Make p->state in order in pinctrl_commit_state

[Aiqun(Maria) Yu]

Hi Pavan,

On 11/1/2022 12:30 PM, Pavan Kondeti wrote:
> Hi Maria,
>
> On Thu, Oct 27, 2022 at 02:54:08PM +0800, Maria Yu wrote:
> > We've got a dump that current cpu is in pinctrl_commit_state, the
> > old_state != p->state while the stack is still in the process of
> > pinmux_disable_setting. So it means even if the current p->state is
> > changed in new state, the settings are not yet up-to-date enabled
> > complete yet.
> >
> > Currently p->state in different value to synchronize the
> > pinctrl_commit_state behaviors. The p->state will have transaction like
> > old_state -> NULL -> new_state. When in old_state, it will try to
> > disable all the all state settings. And when after new state settings
> > enabled, p->state will changed to the new state after that. So use
> > smp_mb to synchronize the p->state variable and the settings in order.
> > ---
> >   drivers/pinctrl/core.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> > index 9e57f4c62e60..cd917a5b1a0a 100644
> > --- a/drivers/pinctrl/core.c
> > +++ b/drivers/pinctrl/core.c
> > @@ -1256,6 +1256,7 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> >   		}
> >   	}
> >   
> > +	smp_mb();
> >   	p->state = NULL;
> >   
> >   	/* Apply all the settings for the new state - pinmux first */
> > @@ -1305,6 +1306,7 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> >   			pinctrl_link_add(setting->pctldev, p->dev);
> >   	}
> >   
> > +	smp_mb();
> >   	p->state = state;
> >   
>
>  From your commit description, are you inferring that this p->state assignment
> re-ordered wrt pinmux_disable_setting()? btw, I don't see any locking that
> protects concurrent access to p->state modifications. For whatever reasons, if
> a client makes concurrent calls to pinctrl_select_state(), we can land up in
> the situation, you are seeing. correct?
correct.
> Thanks,
> Pavan


--
Thx and BRs,
Aiqun(Maria) Yu