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

[Bjorn Andersson <andersson@xxxxxxxxxx>]

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
> 
>