Messages in this thread | | | Date | Fri, 2 Feb 2024 19:09:39 +0800 | Subject | Re: [PATCH v4] pinctrl: Add lock to ensure the state atomization | From | "Aiqun Yu (Maria)" <> |
| |
On 2/2/2024 6:58 PM, Maria Yu 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. > 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> > --- forget to add change log here: v4-v3 changed: #include <linux/cleanup.h> is removed because of rebase. others is same to v3. > drivers/pinctrl/core.c | 143 +++++++++++++++++++++-------------------- > drivers/pinctrl/core.h | 2 + > 2 files changed, 75 insertions(+), 70 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index ee56856cb80c..1f7d001d4c1e 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -1061,6 +1061,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) { > @@ -1257,93 +1258,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 837fd5bd903d..6844edd38b4a 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: 6613476e225e090cc9aad49be7fa504e290dd33d
-- Thx and BRs, Aiqun(Maria) Yu
| |