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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Maria,

thanks for your patch!

On Thu, Oct 27, 2022 at 8:51 AM Maria Yu <quic_aiquny@xxxxxxxxxxx> 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;
>
>         return 0;

Ow!

It's not often that I loop in Paul McKenney on patches, but this is in the core
of the subsystem used across all architectures so if this is a generic problem
of concurrency, I really want some professional concurrency person to
look at it before I apply it.

Yours,
Linus Walleij