2022-10-27 06:57:22

by Aiqun Yu (Maria)

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

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;
--
2.17.1

2022-11-01 05:09:09

by Pavan Kondeti

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

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?

Thanks,
Pavan

2022-11-02 01:33:54

by Aiqun Yu (Maria)

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

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