pinmux: Use sequential access to access desc->pinmux data

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 mutex 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>
Link: https://lore.kernel.org/20241014192930.1539673-1-quic_mojha@quicinc.com
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4061890..b3eec63 100644 (file)
--- 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
+       mutex_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 4e07707..d6c2497 100644 (file)
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -177,6 +177,7 @@ struct pin_desc {
        const char *mux_owner;
        const struct pinctrl_setting_mux *mux_setting;
        const char *gpio_owner;
+       struct mutex mux_lock;
 #endif
 };
 

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 02033ea..0743190 100644 (file)
--- 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>
@@ -93,6 +94,7 @@ bool pinmux_can_be_used_for_gpio(struct pinctrl_dev *pctldev, unsigned int pin)
        if (!desc || !ops)
                return true;
 
+       guard(mutex)(&desc->mux_lock);
        if (ops->strict && desc->mux_usecount)
                return false;
 
@@ -127,29 +129,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(mutex, &desc->mux_lock) {
+               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 +182,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(mutex, &desc->mux_lock) {
+                       if (gpio_range) {
+                               desc->gpio_owner = NULL;
+                       } else {
+                               desc->mux_usecount--;
+                               if (!desc->mux_usecount)
+                                       desc->mux_owner = NULL;
+                       }
                }
        }
 out:
@@ -219,15 +225,17 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
                return NULL;
        }
 
-       if (!gpio_range) {
-               /*
-                * 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(mutex, &desc->mux_lock) {
+               if (!gpio_range) {
+                       /*
+                        * 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;
+               }
        }
 
        /*
@@ -239,13 +247,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(mutex, &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);
@@ -458,7 +468,8 @@ int pinmux_enable_setting(const struct pinctrl_setting *setting)
                                 pins[i]);
                        continue;
                }
-               desc->mux_setting = &(setting->data.mux);
+               scoped_guard(mutex, &desc->mux_lock)
+                       desc->mux_setting = &(setting->data.mux);
        }
 
        ret = ops->set_mux(pctldev, setting->data.mux.func,
@@ -472,8 +483,10 @@ int pinmux_enable_setting(const struct pinctrl_setting *setting)
 err_set_mux:
        for (i = 0; i < num_pins; i++) {
                desc = pin_desc_get(pctldev, pins[i]);
-               if (desc)
-                       desc->mux_setting = NULL;
+               if (desc) {
+                       scoped_guard(mutex, &desc->mux_lock)
+                               desc->mux_setting = NULL;
+               }
        }
 err_pin_request:
        /* On error release all taken pins */
@@ -492,6 +505,7 @@ void pinmux_disable_setting(const struct pinctrl_setting *setting)
        unsigned int num_pins = 0;
        int i;
        struct pin_desc *desc;
+       bool is_equal;
 
        if (pctlops->get_group_pins)
                ret = pctlops->get_group_pins(pctldev, setting->data.mux.group,
@@ -517,7 +531,10 @@ void pinmux_disable_setting(const struct pinctrl_setting *setting)
                                 pins[i]);
                        continue;
                }
-               if (desc->mux_setting == &(setting->data.mux)) {
+               scoped_guard(mutex, &desc->mux_lock)
+                       is_equal = (desc->mux_setting == &(setting->data.mux));
+
+               if (is_equal) {
                        pin_free(pctldev, pins[i], NULL);
                } else {
                        const char *gname;
@@ -608,40 +625,42 @@ 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(mutex, &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');
        }
 
        mutex_unlock(&pctldev->mutex);