diff mbox series

[v2] pinctrl: Add lock to ensure the state atomization

Message ID 20231212090611.950-1-quic_aiquny@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v2] pinctrl: Add lock to ensure the state atomization | expand

Commit Message

Aiqun Yu (Maria) Dec. 12, 2023, 9:06 a.m. UTC 
Currently pinctrl_select_state is an export symbol and don't have
effective re-entrance protect design. During async probing of devices
it's possible to end up in pinctrl_select_state() from multiple
contexts simultaneously, so make it thread safe.
More over, when the real racy happened, the system frequently have
printk message like:
  "not freeing pin xx (xxx) as part of deactivating group xxx - it is
already used for some other setting".
Finally the system crashed after the flood log.
Add per pinctrl lock to ensure the old state and new state transition
atomization.
Also move dev error print message outside the region with interrupts
disabled.

Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
---
 drivers/pinctrl/core.c | 11 +++++++++--
 drivers/pinctrl/core.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)


base-commit: 26aff849438cebcd05f1a647390c4aa700d5c0f1

Comments

Linus Walleij Dec. 20, 2023, 11:02 a.m. UTC | #1 
Hi Maria,

On Tue, Dec 12, 2023 at 10:06 AM Maria Yu <quic_aiquny@quicinc.com> wrote:

> Currently pinctrl_select_state is an export symbol and don't have
> effective re-entrance protect design. During async probing of devices
> it's possible to end up in pinctrl_select_state() from multiple
> contexts simultaneously, so make it thread safe.
> More over, when the real racy happened, the system frequently have
> printk message like:
>   "not freeing pin xx (xxx) as part of deactivating group xxx - it is
> already used for some other setting".
> Finally the system crashed after the flood log.
> Add per pinctrl lock to ensure the old state and new state transition
> atomization.
> Also move dev error print message outside the region with interrupts
> disabled.
>
> Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>

Overall this looks good!

> @@ -1262,9 +1263,12 @@ static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>  static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>  {
>         struct pinctrl_setting *setting, *setting2;
> -       struct pinctrl_state *old_state = READ_ONCE(p->state);
> +       struct pinctrl_state *old_state;
>         int ret;
> +       unsigned long flags;
>
> +       spin_lock_irqsave(&p->lock, flags);
(...)
> +       spin_unlock_irqrestore(&p->lock, flags);
(...)
> +       spin_unlock_irqrestore(&p->lock, flags);

Is it possible to use a scoped guard for pinctrl_commit_state()?

#include <linux/cleanup.h>
guard(spinlock_irqsave)(&p->lock);

It saves some code (and no need for flags) and avoid possible
bugs when people add new errorpaths to the code.

Yours,
Linus Walleij
Aiqun Yu (Maria) Dec. 25, 2023, 6:13 a.m. UTC | #2 
On 12/20/2023 7:02 PM, Linus Walleij wrote:
> Hi Maria,
> 
> On Tue, Dec 12, 2023 at 10:06 AM Maria Yu <quic_aiquny@quicinc.com> wrote:
> 
>> Currently pinctrl_select_state is an export symbol and don't have
>> effective re-entrance protect design. During async probing of devices
>> it's possible to end up in pinctrl_select_state() from multiple
>> contexts simultaneously, so make it thread safe.
>> More over, when the real racy happened, the system frequently have
>> printk message like:
>>    "not freeing pin xx (xxx) as part of deactivating group xxx - it is
>> already used for some other setting".
>> Finally the system crashed after the flood log.
>> Add per pinctrl lock to ensure the old state and new state transition
>> atomization.
>> Also move dev error print message outside the region with interrupts
>> disabled.
>>
>> Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> 
> Overall this looks good!
> 
>> @@ -1262,9 +1263,12 @@ static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>>   static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>>   {
>>          struct pinctrl_setting *setting, *setting2;
>> -       struct pinctrl_state *old_state = READ_ONCE(p->state);
>> +       struct pinctrl_state *old_state;
>>          int ret;
>> +       unsigned long flags;
>>
>> +       spin_lock_irqsave(&p->lock, flags);
> (...)
>> +       spin_unlock_irqrestore(&p->lock, flags);
> (...)
>> +       spin_unlock_irqrestore(&p->lock, flags);
> 
> Is it possible to use a scoped guard for pinctrl_commit_state()?
Good idea.
I will address this in next patchset.
> 
> #include <linux/cleanup.h>
> guard(spinlock_irqsave)(&p->lock);
> 
> It saves some code (and no need for flags) and avoid possible
> bugs when people add new errorpaths to the code.
> 
> Yours,
> Linus Walleij
diff mbox series

Patch

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f2977eb65522..a19c286bf82e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1066,6 +1066,7 @@  static struct pinctrl *create_pinctrl(struct device *dev,
 	p->dev = dev;
 	INIT_LIST_HEAD(&p->states);
 	INIT_LIST_HEAD(&p->dt_maps);
+	spin_lock_init(&p->lock);
 
 	ret = pinctrl_dt_to_map(p, pctldev);
 	if (ret < 0) {
@@ -1262,9 +1263,12 @@  static void pinctrl_link_add(struct pinctrl_dev *pctldev,
 static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
 {
 	struct pinctrl_setting *setting, *setting2;
-	struct pinctrl_state *old_state = READ_ONCE(p->state);
+	struct pinctrl_state *old_state;
 	int ret;
+	unsigned long flags;
 
+	spin_lock_irqsave(&p->lock, flags);
+	old_state = p->state;
 	if (old_state) {
 		/*
 		 * For each pinmux setting in the old state, forget SW's record
@@ -1329,11 +1333,11 @@  static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
 	}
 
 	p->state = state;
+	spin_unlock_irqrestore(&p->lock, flags);
 
 	return 0;
 
 unapply_new_state:
-	dev_err(p->dev, "Error applying setting, reverse things back\n");
 
 	list_for_each_entry(setting2, &state->settings, node) {
 		if (&setting2->node == &setting->node)
@@ -1349,6 +1353,9 @@  static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
 			pinmux_disable_setting(setting2);
 	}
 
+	spin_unlock_irqrestore(&p->lock, flags);
+
+	dev_err(p->dev, "Error applying setting, reverse things back\n");
 	/* There's no infinite recursive loop here because p->state is NULL */
 	if (old_state)
 		pinctrl_select_state(p, old_state);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 530370443c19..86fc41393f7b 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -12,6 +12,7 @@ 
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/radix-tree.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #include <linux/pinctrl/machine.h>
@@ -91,6 +92,7 @@  struct pinctrl {
 	struct pinctrl_state *state;
 	struct list_head dt_maps;
 	struct kref users;
+	spinlock_t lock;
 };
 
 /**