On Fri, Dec 01, 2023 at 11:29:31PM +0800, Maria Yu wrote:
> Currently pinctrl_select_state is an export symbol and don't have
> effective re-entrance protect design. And possible of pinctrl state
> changed during pinctrl_commit_state handling. Add per pinctrl lock to
> ensure the old state and new state transition atomization.
> Move dev error print message right before old_state pinctrl_select_state
> and out of lock protection to avoid console related driver call
> pinctrl_select_state recursively.
I'm uncertain about the validity of having client code call this api in
a racy manner. I'm likely just missing something here... It would be
nice if this scenario was described in a little bit more detail.
The recursive error print sounds like a distinct problem of its own,
that warrants being introduced in a patch of its own. But as with the
other part, I'm not able to spot a code path in the upstream kernel
where this hppens, so please properly describe the scenario where
touching the console would result back in another pinctrl_select_state().
Thanks,
Bjorn
>
> Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
> Signed-off-by: Maria Yu <quic_aiquny@xxxxxxxxxxx>
> ---
> drivers/pinctrl/core.c | 11 +++++++++--
> drivers/pinctrl/core.h | 2 ++
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> 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;
> };
>
> /**
>
> base-commit: 994d5c58e50e91bb02c7be4a91d5186292a895c8
> --
> 2.17.1
>
>
