[PATCH v3] pinctrl: Add lock to ensure the state atomization

[PATCH v3] pinctrl: Add lock to ensure the state atomization

[Maria Yu]

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.
Use scoped guard to simplify the lock protection needed code.

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

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f2977eb65522..ab158b745486 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -13,6 +13,7 @@
 #define pr_fmt(fmt) "pinctrl core: " fmt
 
 #include <linux/array_size.h>
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -1066,6 +1067,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,93 +1264,95 @@ 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;
 
-	if (old_state) {
-		/*
-		 * For each pinmux setting in the old state, forget SW's record
-		 * of mux owner for that pingroup. Any pingroups which are
-		 * still owned by the new state will be re-acquired by the call
-		 * to pinmux_enable_setting() in the loop below.
-		 */
-		list_for_each_entry(setting, &old_state->settings, node) {
-			if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
-				continue;
-			pinmux_disable_setting(setting);
+	scoped_guard(spinlock_irqsave, &p->lock) {
+		old_state = p->state;
+		if (old_state) {
+			/*
+			 * For each pinmux setting in the old state, forget SW's record
+			 * of mux owner for that pingroup. Any pingroups which are
+			 * still owned by the new state will be re-acquired by the call
+			 * to pinmux_enable_setting() in the loop below.
+			 */
+			list_for_each_entry(setting, &old_state->settings, node) {
+				if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
+					continue;
+				pinmux_disable_setting(setting);
+			}
 		}
-	}
-
-	p->state = NULL;
 
-	/* Apply all the settings for the new state - pinmux first */
-	list_for_each_entry(setting, &state->settings, node) {
-		switch (setting->type) {
-		case PIN_MAP_TYPE_MUX_GROUP:
-			ret = pinmux_enable_setting(setting);
-			break;
-		case PIN_MAP_TYPE_CONFIGS_PIN:
-		case PIN_MAP_TYPE_CONFIGS_GROUP:
-			ret = 0;
-			break;
-		default:
-			ret = -EINVAL;
-			break;
-		}
+		p->state = NULL;
 
-		if (ret < 0)
-			goto unapply_new_state;
+		/* Apply all the settings for the new state - pinmux first */
+		list_for_each_entry(setting, &state->settings, node) {
+			switch (setting->type) {
+			case PIN_MAP_TYPE_MUX_GROUP:
+				ret = pinmux_enable_setting(setting);
+				break;
+			case PIN_MAP_TYPE_CONFIGS_PIN:
+			case PIN_MAP_TYPE_CONFIGS_GROUP:
+				ret = 0;
+				break;
+			default:
+				ret = -EINVAL;
+				break;
+			}
 
-		/* Do not link hogs (circular dependency) */
-		if (p != setting->pctldev->p)
-			pinctrl_link_add(setting->pctldev, p->dev);
-	}
+			if (ret < 0)
+				goto unapply_new_state;
 
-	/* Apply all the settings for the new state - pinconf after */
-	list_for_each_entry(setting, &state->settings, node) {
-		switch (setting->type) {
-		case PIN_MAP_TYPE_MUX_GROUP:
-			ret = 0;
-			break;
-		case PIN_MAP_TYPE_CONFIGS_PIN:
-		case PIN_MAP_TYPE_CONFIGS_GROUP:
-			ret = pinconf_apply_setting(setting);
-			break;
-		default:
-			ret = -EINVAL;
-			break;
+			/* Do not link hogs (circular dependency) */
+			if (p != setting->pctldev->p)
+				pinctrl_link_add(setting->pctldev, p->dev);
 		}
 
-		if (ret < 0) {
-			goto unapply_new_state;
-		}
+		/* Apply all the settings for the new state - pinconf after */
+		list_for_each_entry(setting, &state->settings, node) {
+			switch (setting->type) {
+			case PIN_MAP_TYPE_MUX_GROUP:
+				ret = 0;
+				break;
+			case PIN_MAP_TYPE_CONFIGS_PIN:
+			case PIN_MAP_TYPE_CONFIGS_GROUP:
+				ret = pinconf_apply_setting(setting);
+				break;
+			default:
+				ret = -EINVAL;
+				break;
+			}
 
-		/* Do not link hogs (circular dependency) */
-		if (p != setting->pctldev->p)
-			pinctrl_link_add(setting->pctldev, p->dev);
-	}
+			if (ret < 0)
+				goto unapply_new_state;
 
-	p->state = state;
+			/* Do not link hogs (circular dependency) */
+			if (p != setting->pctldev->p)
+				pinctrl_link_add(setting->pctldev, p->dev);
+		}
 
-	return 0;
+		p->state = state;
+
+		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)
-			break;
-		/*
-		 * All we can do here is pinmux_disable_setting.
-		 * That means that some pins are muxed differently now
-		 * than they were before applying the setting (We can't
-		 * "unmux a pin"!), but it's not a big deal since the pins
-		 * are free to be muxed by another apply_setting.
-		 */
-		if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
-			pinmux_disable_setting(setting2);
+		list_for_each_entry(setting2, &state->settings, node) {
+			if (&setting2->node == &setting->node)
+				break;
+			/*
+			 * All we can do here is pinmux_disable_setting.
+			 * That means that some pins are muxed differently now
+			 * than they were before applying the setting (We can't
+			 * "unmux a pin"!), but it's not a big deal since the pins
+			 * are free to be muxed by another apply_setting.
+			 */
+			if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
+				pinmux_disable_setting(setting2);
+		}
 	}
 
+	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;
 };
 
 /**

base-commit: 861deac3b092f37b2c5e6871732f3e11486f7082
-- 
2.17.1

Re: [PATCH v3] pinctrl: Add lock to ensure the state atomization

[Linus Walleij]

Hi Maria,

I wanted to apply this v3 patch, but I can't figure out what it is based on?

Could you rebase the patch on v6.8-rc1 and resend?

Yours,
Linus Walleij

Re: [PATCH v3] pinctrl: Add lock to ensure the state atomization

[Aiqun Yu (Maria)]

On 1/28/2024 6:43 AM, Linus Walleij wrote:
> Hi Maria,
> 
> I wanted to apply this v3 patch, but I can't figure out what it is based on?
> 
> Could you rebase the patch on v6.8-rc1 and resend?
Hi Linus,

Here you go:
https://lore.kernel.org/all/20240202105854.26446-1-quic_aiquny@quicinc.com/T/#u

When I rebase to v6.8-rc1, there is little conflict about header file, 
so the cleanup.h is already added from other commits.
So I just mark it as v4 instead of Resend.

Hope this information is clear to you.
> 
> Yours,
> Linus Walleij

-- 
Thx and BRs,
Aiqun(Maria) Yu