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

["Aiqun Yu (Maria)" <quic_aiquny@xxxxxxxxxxx>]

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