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

[Alexander Stein]

Am Freitag, 2. Februar 2024, 11:58:54 CET schrieb 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@xxxxxxxxxxx>
> ---
>  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

This breaks pinctrl-imx on imx8qxp:

[    1.170727] imx8qxp-pinctrl system-controller:pinctrl: initialized IMX 
pinctrl driver
[    1.283968] BUG: sleeping function called from invalid context at kernel/
locking/mutex.c:283
[    1.292089] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 70, 
name: kworker/u16:4
[    1.300341] preempt_count: 1, expected: 0
[    1.304337] RCU nest depth: 0, expected: 0
[    1.308423] CPU: 2 PID: 70 Comm: kworker/u16:4 Not tainted 6.8.0-rc3-
next-20240209+ #2267 0b2aeebc4d64f1aef3abdd5fede2a9b5162eb867
[    1.320148] Hardware name: TQ-Systems i.MX8QXP TQMa8XQP on MBa8Xx (DT)
[    1.326667] Workqueue: events_unbound deferred_probe_work_func
[    1.332486] Call trace:
[    1.334918]  dump_backtrace+0x90/0x10c
[    1.338653]  show_stack+0x14/0x1c
[    1.341954]  dump_stack_lvl+0x6c/0x80
[    1.345603]  dump_stack+0x14/0x1c
[    1.348904]  __might_resched+0x108/0x160
[    1.352813]  __might_sleep+0x58/0xb0
[    1.356375]  mutex_lock+0x20/0x74
[    1.359676]  imx_scu_call_rpc+0x44/0x2e8
[    1.363586]  imx_pinconf_set_scu+0x84/0x150
[    1.367756]  imx_pinconf_set+0x48/0x7c
[    1.371491]  pinconf_apply_setting+0x90/0x110
[    1.375835]  pinctrl_commit_state+0xcc/0x28c
[    1.380092]  pinctrl_select_state+0x18/0x28
[    1.384262]  pinctrl_bind_pins+0x1e4/0x26c
[    1.388345]  really_probe+0x60/0x3e0
[    1.391907]  __driver_probe_device+0x84/0x198
[    1.396251]  driver_probe_device+0x38/0x150
[    1.400421]  __device_attach_driver+0xcc/0x194
[    1.404851]  bus_for_each_drv+0x80/0xdc
[    1.408674]  __device_attach+0x9c/0x1d0
[    1.412496]  device_initial_probe+0x10/0x18
[    1.416666]  bus_probe_device+0xa4/0xa8
[    1.420489]  deferred_probe_work_func+0x9c/0xe8
[    1.425006]  process_one_work+0x14c/0x40c
[    1.429002]  worker_thread+0x304/0x414
[    1.432738]  kthread+0xf4/0x100
[    1.435866]  ret_from_fork+0x10/0x20

With this commit pin_config_set callbacks need to be atomic suddenly which is 
a no-go for any device attached to i2c or spi and in this case IPC RPC.
Once reverted systems start normally again.

Best regards,
Alexander

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/