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

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

[Maria Yu]

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

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

[Greg KH]

On Thu, Oct 27, 2022 at 02:51:10PM +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;
>  
>  	return 0;
> -- 
> 2.17.1
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

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

[Aiqun Yu (Maria)]

Thx Greg,

Pls ignore this thread. Already correct to the normal  linux-kernel@vger.kernel.org  in another email thread.

Thx and BRs,
Aiqun Yu (Maria)

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org> 
Sent: Thursday, October 27, 2022 3:24 PM
To: Aiqun Yu (QUIC) <quic_aiquny@quicinc.com>
Cc: linus.walleij@linaro.org; linux-gpio@vger.kernel.org; stable@vger.kernel.org
Subject: Re: [PATCH] pinctrl: core: Make p->state in order in pinctrl_commit_state

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

On Thu, Oct 27, 2022 at 02:51:10PM +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;
>
>       return 0;
> --
> 2.17.1
>

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

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

[Linus Walleij]

Hi Maria,

thanks for your patch!

On Thu, Oct 27, 2022 at 8:51 AM Maria Yu <quic_aiquny@quicinc.com> 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

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

[Paul E. McKenney]

On Tue, Nov 08, 2022 at 01:47:15PM +0100, Linus Walleij wrote:
> Hi Maria,
> 
> thanks for your patch!
> 
> On Thu, Oct 27, 2022 at 8:51 AM Maria Yu <quic_aiquny@quicinc.com> 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.

Hello, Linus and Maria!

Insertion of unadorned and uncommented memory barriers does rouse more
than a bit of suspicion, to be sure.  ;-)

Could you please outline what ordering this smp_mb() is intended to
provide?  Yes, my guess is that the p->state change is to be seen as
happening after the prior memory accesses, but:

1.	What is the other side of the interaction doing?  My guess is
	that something is reading p->state and the referencing the same
	memory referenced prior to the pair of smp_mb() calls above.
	For example, are the other relevant memory references referenced
	by the pointer "p"?

	For example, what happens if two of the above updates happen in
	quick succession during the execution of a single instance of
	the other side of the interaction?

2.	Why smp_mb() rather than using smp_store_release() to update
	p->state?

3.	More generally, why unmarked accesses to p->state?  Are the
	other relevant accesses also unmarked?

	Please see these LWN articles for more on the potential dangers
	of unmarked accesses to shared variables:

	Who's afraid of a big bad optimizing compiler?
		https://lwn.net/Articles/793253/

	Calibrating your fear of big bad optimizing compilers
		https://lwn.net/Articles/799218/

4.	There are some tools that can help with this sort of ordering
	code, for example:

	Concurrency bugs should fear the big bad data-race detector (part 1)
		https://lwn.net/Articles/816850/
	Concurrency bugs should fear the big bad data-race detector (part 2)
		https://lwn.net/Articles/816854/

	For this tool (KCSAN) to find a problem, your testing must come
	close to making it happen.

	A design-level full-state-space tool may be found in
	tools/memotry-model.  This tool, as you might expect, is
	restricted to very short code fragments, but does fully handle
	concurrency.  It might take some work to squeeze what you have
	into the confines of this tool.

Again, to evaluate this change, I need to understand what it is trying
to accomplish.

							Thanx, Paul

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

[Aiqun(Maria) Yu]

On 11/9/2022 3:20 AM, Paul E. McKenney wrote:
> On Tue, Nov 08, 2022 at 01:47:15PM +0100, Linus Walleij wrote:
>> Hi Maria,
>>
>> thanks for your patch!
>>
>> On Thu, Oct 27, 2022 at 8:51 AM Maria Yu <quic_aiquny@quicinc.com> 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.
> 
> Hello, Linus and Maria!
> 
> Insertion of unadorned and uncommented memory barriers does rouse more
> than a bit of suspicion, to be sure.  ;-)
> 
Thx Paul and Linus,

welcome for the discussion and thx for the envolvement, that's what 
values a lot through the open source work.

> Could you please outline what ordering this smp_mb() is intended to
> provide?  Yes, my guess is that the p->state change is to be seen as
> happening after the prior memory accesses, but:
> 
> 1.	What is the other side of the interaction doing?  My guess is
> 	that something is reading p->state and the referencing the same
> 	memory referenced prior to the pair of smp_mb() calls above.
> 	For example, are the other relevant memory references referenced
> 	by the pointer "p"?
> 
> 	For example, what happens if two of the above updates happen in
> 	quick succession during the execution of a single instance of
> 	the other side of the interaction?
> 

we've got a dump that the taskA is in call stack of pinctrl_commit_state 
-> pinmux_disable_setting ->dev_warn , while the old_state is not 
consistant with current p->state.

And from current ramdump, the kernel have warn each ~4us of the same 
dev_warn of "xxx-pinctrl xxx.pinctrl: not freeing pin xxx(GPIO_xxx) as 
part of deactivating group gpioxxx - it is already used for some other 
setting". It last for ~20ms and then have a final sysrq triggered crash.

so here is the possible the senario like below:
|TaskA pinctrl_commit_state| TaskB pinctrl_commit_state
| old_state = p->state;    |
|if (p->state)             | if(p->state); //old_state
|                          |list_for_each_entry
|                          |   pinmux_disable_setting(
|                          |      old_state->settings);
|                          |p->state = NULL;
|                          |list_for_each_entry
|                          | pinmux_enable_setting(
|                          |   new_state->settings);
|                          |p->state = new_state; //new state
|list_for_each_entry       |
|pinmux_disable_setting(   |
|  old_state->settings);   |
| dev_warn("not freeing pin")|
|                          |



> 2.	Why smp_mb() rather than using smp_store_release() to update
> 	p->state?
> 
For now I am a little afraid of the current memroy barrier is still not 
enough and need to use a lock to avoid concurency.

> 3.	More generally, why unmarked accesses to p->state?  Are the
> 	other relevant accesses also unmarked?
> 
> 	Please see these LWN articles for more on the potential dangers
> 	of unmarked accesses to shared variables:
> 
> 	Who's afraid of a big bad optimizing compiler?
> 		https://lwn.net/Articles/793253/
> 
> 	Calibrating your fear of big bad optimizing compilers
> 		https://lwn.net/Articles/799218/
> 
Let me have a study and try.
> 4.	There are some tools that can help with this sort of ordering
> 	code, for example:
> 
> 	Concurrency bugs should fear the big bad data-race detector (part 1)
> 		https://lwn.net/Articles/816850/
> 	Concurrency bugs should fear the big bad data-race detector (part 2)
> 		https://lwn.net/Articles/816854/
> 
> 	For this tool (KCSAN) to find a problem, your testing must come
> 	close to making it happen.
> 
> 	A design-level full-state-space tool may be found in
> 	tools/memotry-model.  This tool, as you might expect, is
> 	restricted to very short code fragments, but does fully handle
> 	concurrency.  It might take some work to squeeze what you have
> 	into the confines of this tool.
> 
Let me have a study and try.
> Again, to evaluate this change, I need to understand what it is trying
> to accomplish.
> 
Sure.
> 							Thanx, Paul
> 


-- 
Thx and BRs,
Aiqun(Maria) Yu

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

[Maria Yu]

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

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

[Pavan Kondeti]

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

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