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/
