Received-SPF: pass (google.com: domain of linux-kernel+bounces-72391-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <400fa03c-bc08-4c32-a1ef-57f227493061@quicinc.com>
Date: Tue, 20 Feb 2024 14:13:06 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4] pinctrl: Add lock to ensure the state atomization
Content-Language: en-US
To: Alexander Stein <alexander.stein@ew.tq-group.com>, <andersson@kernel.org>,
        <linus.walleij@linaro.org>
CC: <kernel@quicinc.com>, <linux-gpio@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>
References: <20240202105854.26446-1-quic_aiquny@quicinc.com>
 <8376074.NyiUUSuA9g@steina-w>
From: "Aiqun Yu (Maria)" <quic_aiquny@quicinc.com>
In-Reply-To: <8376074.NyiUUSuA9g@steina-w>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/9/2024 4:37 PM, Alexander Stein wrote:
> 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@quicinc.com>
>> ---
>>   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.
Thanks for the info. It is a valid scenario of your report. I will also 
re-visit this.
> 
> Best regards,
> Alexander
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu