| Message ID | 20240927174206.602651-1-quic_mojha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinmux: Use sequential access to access desc->pinmux data | expand |
On Sat, Sep 28, 2024 at 01:57:41AM +0530, Wasim Nazir wrote: Hi Wasim, Thanks for the review, there looks to be problem with your email client while replying, please fix. Please find my reply inline.. > From: Mukesh Ojha <quic_mojha@quicinc.com> > > When two client of the same gpio call pinctrl_select_state() for the > same functionality, we are seeing NULL pointer issue while accessing > desc->mux_owner. > > Let's say two processes A, B executing in pin_request() for the same pin > and process A updates the desc->mux_usecount but not yet updated the > desc->mux_owner while process B see the desc->mux_usecount which got > updated by A path and further executes strcmp and while accessing > desc->mux_owner it crashes with NULL pointer. > > Serialize the access to mux related setting with a spin lock. > > cpu0 (process A) cpu1(process B) > > pinctrl_select_state() { pinctrl_select_state() { > pin_request() { pin_request() { > ... > .... > } else { > desc->mux_usecount++; > desc->mux_usecount && strcmp(desc->mux_owner, owner)) { > > if (desc->mux_usecount > 1) > return 0; > desc->mux_owner = owner; > > } } > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > Changes in v2: > - Used scoped_guard and renamed lock name as per suggestion from Linus.W . > > drivers/pinctrl/core.c | 3 + > drivers/pinctrl/core.h | 2 + > drivers/pinctrl/pinmux.c | 150 +++++++++++++++++++++------------------ > 3 files changed, 86 insertions(+), 69 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index 4061890a1748..b00911421cf5 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -220,6 +220,9 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev, > > /* Set owner */ > pindesc->pctldev = pctldev; > +#ifdef CONFIG_PINMUX > + spin_lock_init(&pindesc->mux_lock); > +#endif > > /* Copy basic pin info */ > if (pin->name) { > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h > index 4e07707d2435..179e01dfacc2 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> > @@ -177,6 +178,7 @@ struct pin_desc { > const char *mux_owner; > const struct pinctrl_setting_mux *mux_setting; > const char *gpio_owner; > + spinlock_t mux_lock; > #endif > }; > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 02033ea1c643..e4d535aabbb6 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -14,6 +14,7 @@ > > #include <linux/array_size.h> > #include <linux/ctype.h> > +#include <linux/cleanup.h> > #include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/err.h> > @@ -127,29 +128,31 @@ static int pin_request(struct pinctrl_dev *pctldev, > dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", > pin, desc->name, owner); > > - if ((!gpio_range || ops->strict) && > - desc->mux_usecount && strcmp(desc->mux_owner, owner)) { > - dev_err(pctldev->dev, > - "pin %s already requested by %s; cannot claim for %s\n", > - desc->name, desc->mux_owner, owner); > - goto out; > - } > + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > > Any reason to use spinlock_irqsave and not mutex ? If spinlock is needed > > can we guard only the mux variables and exclude the printk > > as the same lock is used with pin_show API too. Good point, using spinlock_irqsave can make things worse and this can stuck while writing log to console. I remember now, why i did this, v3: https://lore.kernel.org/lkml/20231225082305.12343-1-quic_aiquny@quicinc.com/ Later same patch in v4 was causing sleep while atomic issue., https://lore.kernel.org/lkml/8376074.NyiUUSuA9g@steina-w/ I better be correcting this to mutex here, should also have to increase the range of this lock to cover mux-setting as well. > > > Moreover, is the mux_usecount variable in pinmux_can_be_used_for_gpio() > > needed guarding ? It's a miss, thanks., > + if ((!gpio_range || ops->strict) && > + desc->mux_usecount && strcmp(desc->mux_owner, owner)) { > + dev_err(pctldev->dev, > + "pin %s already requested by %s; cannot claim for %s\n", > + desc->name, desc->mux_owner, owner); > + goto out; > + } > > - if ((gpio_range || ops->strict) && desc->gpio_owner) { > - dev_err(pctldev->dev, > - "pin %s already requested by %s; cannot claim for %s\n", > - desc->name, desc->gpio_owner, owner); > - goto out; > - } > + if ((gpio_range || ops->strict) && desc->gpio_owner) { > + dev_err(pctldev->dev, > + "pin %s already requested by %s; cannot claim for %s\n", > + desc->name, desc->gpio_owner, owner); > + goto out; > + } > > - if (gpio_range) { > - desc->gpio_owner = owner; > - } else { > - desc->mux_usecount++; > - if (desc->mux_usecount > 1) > - return 0; > + if (gpio_range) { > + desc->gpio_owner = owner; > + } else { > + desc->mux_usecount++; > + if (desc->mux_usecount > 1) > + return 0; > > - desc->mux_owner = owner; > + desc->mux_owner = owner; > + } > } > > /* Let each pin increase references to this module */ > @@ -178,12 +181,14 @@ static int pin_request(struct pinctrl_dev *pctldev, > > out_free_pin: > if (status) { > - if (gpio_range) { > - desc->gpio_owner = NULL; > - } else { > - desc->mux_usecount--; > - if (!desc->mux_usecount) > - desc->mux_owner = NULL; > + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > + if (gpio_range) { > + desc->gpio_owner = NULL; > + } else { > + desc->mux_usecount--; > + if (!desc->mux_usecount) > + desc->mux_owner = NULL; > + } > } > } > out: > @@ -223,11 +228,13 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin, > /* > * A pin should not be freed more times than allocated. > */ > - if (WARN_ON(!desc->mux_usecount)) > - return NULL; > - desc->mux_usecount--; > - if (desc->mux_usecount) > - return NULL; > + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > + if (WARN_ON(!desc->mux_usecount)) > + return NULL; > + desc->mux_usecount--; > + if (desc->mux_usecount) > + return NULL; > + } > } > > /* > @@ -239,13 +246,15 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin, > else if (ops->free) > ops->free(pctldev, pin); > > - if (gpio_range) { > - owner = desc->gpio_owner; > - desc->gpio_owner = NULL; > - } else { > - owner = desc->mux_owner; > - desc->mux_owner = NULL; > - desc->mux_setting = NULL; > + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > + if (gpio_range) { > + owner = desc->gpio_owner; > + desc->gpio_owner = NULL; > + } else { > + owner = desc->mux_owner; > + desc->mux_owner = NULL; > + desc->mux_setting = NULL; > + } > } > > module_put(pctldev->owner); > @@ -608,40 +617,43 @@ static int pinmux_pins_show(struct seq_file *s, void *what) > if (desc == NULL) > continue; > > - if (desc->mux_owner && > - !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev))) > - is_hog = true; > - > - if (pmxops->strict) { > - if (desc->mux_owner) > - seq_printf(s, "pin %d (%s): device %s%s", > - pin, desc->name, desc->mux_owner, > + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > + if (desc->mux_owner && > + !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev))) > + is_hog = true; > + > + if (pmxops->strict) { > + if (desc->mux_owner) > + seq_printf(s, "pin %d (%s): device %s%s", > + pin, desc->name, desc->mux_owner, > + is_hog ? " (HOG)" : ""); > + else if (desc->gpio_owner) > + seq_printf(s, "pin %d (%s): GPIO %s", > + pin, desc->name, desc->gpio_owner); > + else > + seq_printf(s, "pin %d (%s): UNCLAIMED", > + pin, desc->name); > + } else { > + /* For non-strict controllers */ > + seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name, > + desc->mux_owner ? desc->mux_owner > + : "(MUX UNCLAIMED)", > + desc->gpio_owner ? desc->gpio_owner > + : "(GPIO UNCLAIMED)", > is_hog ? " (HOG)" : ""); > - else if (desc->gpio_owner) > - seq_printf(s, "pin %d (%s): GPIO %s", > - pin, desc->name, desc->gpio_owner); > + } > + > + /* If mux: print function+group claiming the pin */ > + if (desc->mux_setting) > + seq_printf(s, " function %s group %s\n", > + pmxops->get_function_name(pctldev, > + desc->mux_setting->func), > + pctlops->get_group_name(pctldev, > + desc->mux_setting->group)); > else > - seq_printf(s, "pin %d (%s): UNCLAIMED", > - pin, desc->name); > - } else { > - /* For non-strict controllers */ > - seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name, > - desc->mux_owner ? desc->mux_owner > - : "(MUX UNCLAIMED)", > - desc->gpio_owner ? desc->gpio_owner > - : "(GPIO UNCLAIMED)", > - is_hog ? " (HOG)" : ""); > - } > + seq_putc(s, '\n'); > > - /* If mux: print function+group claiming the pin */ > - if (desc->mux_setting) > - seq_printf(s, " function %s group %s\n", > - pmxops->get_function_name(pctldev, > - desc->mux_setting->func), > - pctlops->get_group_name(pctldev, > - desc->mux_setting->group)); > - else > - seq_putc(s, '\n'); > + } > > Since we are introducing a lock, do we need to guard mux-settings too ? Yes, we should, I would need to take care at other places as well. -Mukesh > } > > mutex_unlock(&pctldev->mutex); > > -- > 2.34.1 > >
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 4061890a1748..b00911421cf5 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -220,6 +220,9 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev, /* Set owner */ pindesc->pctldev = pctldev; +#ifdef CONFIG_PINMUX + spin_lock_init(&pindesc->mux_lock); +#endif /* Copy basic pin info */ if (pin->name) { diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index 4e07707d2435..179e01dfacc2 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> @@ -177,6 +178,7 @@ struct pin_desc { const char *mux_owner; const struct pinctrl_setting_mux *mux_setting; const char *gpio_owner; + spinlock_t mux_lock; #endif }; diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 02033ea1c643..e4d535aabbb6 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -14,6 +14,7 @@ #include <linux/array_size.h> #include <linux/ctype.h> +#include <linux/cleanup.h> #include <linux/debugfs.h> #include <linux/device.h> #include <linux/err.h> @@ -127,29 +128,31 @@ static int pin_request(struct pinctrl_dev *pctldev, dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", pin, desc->name, owner); - if ((!gpio_range || ops->strict) && - desc->mux_usecount && strcmp(desc->mux_owner, owner)) { - dev_err(pctldev->dev, - "pin %s already requested by %s; cannot claim for %s\n", - desc->name, desc->mux_owner, owner); - goto out; - } + scoped_guard(spinlock_irqsave, &desc->mux_lock) { > Any reason to use spinlock_irqsave and not mutex ? If spinlock is needed > can we guard only the mux variables and exclude the printk > as the same lock is used with pin_show API too. > Moreover, is the mux_usecount variable in pinmux_can_be_used_for_gpio() > needed guarding ? + if ((!gpio_range || ops->strict) && + desc->mux_usecount && strcmp(desc->mux_owner, owner)) { + dev_err(pctldev->dev, + "pin %s already requested by %s; cannot claim for %s\n", + desc->name, desc->mux_owner, owner); + goto out; + } - if ((gpio_range || ops->strict) && desc->gpio_owner) { - dev_err(pctldev->dev, - "pin %s already requested by %s; cannot claim for %s\n", - desc->name, desc->gpio_owner, owner); - goto out; - } + if ((gpio_range || ops->strict) && desc->gpio_owner) { + dev_err(pctldev->dev, + "pin %s already requested by %s; cannot claim for %s\n", + desc->name, desc->gpio_owner, owner); + goto out; + } - if (gpio_range) { - desc->gpio_owner = owner; - } else { - desc->mux_usecount++; - if (desc->mux_usecount > 1) - return 0; + if (gpio_range) { + desc->gpio_owner = owner; + } else { + desc->mux_usecount++; + if (desc->mux_usecount > 1) + return 0;