gpio: shared-proxy: always serialize with a sleeping mutex

The shared GPIO descriptor used either a mutex or a spinlock, chosen at
runtime from the underlying chip's can_sleep:

	shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc);
	... if (can_sleep) mutex_lock(); else spin_lock_irqsave();

can_sleep describes only the value path (->get/->set). Under the same
lock, however, the proxy may call gpiod_set_config() and
gpiod_direction_*(), which can reach pinctrl paths that take a mutex
(e.g. gpiod_set_config() -> gpiochip_generic_config() ->
pinctrl_gpio_set_config()), independent of can_sleep. On a controller
with non-sleeping MMIO value ops the descriptor lock was a spinlock, so
the sleeping pinctrl call ran from atomic context. Reproduced on an
Amlogic A113X board with the workaround from commit 28f240683871
("pinctrl: meson: mark the GPIO controller as sleeping") reverted; the
original Khadas VIM3 report hit the same path:

	BUG: sleeping function called from invalid context
	  __mutex_lock
	  pinctrl_get_device_gpio_range
	  pinctrl_gpio_set_config
	  gpiochip_generic_config
	  gpiod_set_config
	  gpio_shared_proxy_set_config   <- voting spinlock held
	  ...
	  mmc_pwrseq_simple_probe

The spinlock existed to take the value vote from atomic context, but the
vote and the (possibly sleeping) control operations share the same state
and lock, so this scheme cannot serialize config under a mutex and still
offer atomic value access. Always serialize the shared descriptor with a
mutex instead and mark the proxy a sleeping gpiochip, driving the
underlying GPIO through the cansleep value accessors: those are valid
for both sleeping and non-sleeping chips, so value access keeps working
on fast controllers, at the cost of no longer being atomic.

With every vote edge now driven through the cansleep value setter,
gpio_shared_proxy_set_unlocked() no longer needs a per-call setter: drop
its set_func callback and call gpiod_set_value_cansleep() directly. The
shared direction_output path reaches it only once the line is already an
output, so driving the value there is equivalent to re-issuing
gpiod_direction_output(), without the redundant per-edge re-assertion of
drive config and bias.

This is observable: consumers gating on gpiod_cansleep() take their
sleeping branch on a proxied GPIO (mmc-pwrseq-emmc skips its
emergency-restart reset handler; its normal reset is unaffected), and
consumers that reject sleeping GPIOs (pwm-gpio, ps2-gpio, ...) would
fail to probe. Such atomic users do not share a pin through the proxy,
whose purpose is voting on shared reset/enable lines. The same narrowing
already applies on Amlogic since that workaround, and rockchip
addressed the identical splat per-driver in commit 7ca497be0016 ("gpio:
rockchip: Stop calling pinctrl for set_direction"); fixing the proxy
addresses the locking error once, for every controller.

The lock type was added by commit a060b8c511ab ("gpiolib: implement
low-level, shared GPIO support"); the sleeping call under it arrived with
the proxy driver.

Fixes: e992d54c6f97 ("gpio: shared-proxy: implement the shared GPIO proxy driver")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/all/00107523-7737-4b92-a785-14ce4e93b8cb@samsung.com/
Signed-off-by: Viacheslav Bocharov <v@baodeep.com>
Link: https://patch.msgid.link/20260630101545.800625-2-v@baodeep.com
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

diff --git a/drivers/gpio/gpio-shared-proxy.c b/drivers/gpio/gpio-shared-proxy.c
index 6941e4be6cf1871b134c55c05877c9793de52ac3..10ca2ef77ef3410b7be46a6657ebea8cd80c4491 100644 (file)
--- a/drivers/gpio/gpio-shared-proxy.c
+++ b/drivers/gpio/gpio-shared-proxy.c
@@ -9,8 +9,10 @@
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
+#include <linux/lockdep.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/string_choices.h>
 #include <linux/types.h>
 
@@ -24,15 +26,13 @@ struct gpio_shared_proxy_data {
 };
 
 static int
-gpio_shared_proxy_set_unlocked(struct gpio_shared_proxy_data *proxy,
-                              int (*set_func)(struct gpio_desc *desc, int value),
-                              int value)
+gpio_shared_proxy_set_unlocked(struct gpio_shared_proxy_data *proxy, int value)
 {
        struct gpio_shared_desc *shared_desc = proxy->shared_desc;
        struct gpio_desc *desc = shared_desc->desc;
        int ret = 0;
 
-       gpio_shared_lockdep_assert(shared_desc);
+       lockdep_assert_held(&shared_desc->mutex);
 
        if (value) {
               /* User wants to set value to high. */
@@ -46,7 +46,7 @@ gpio_shared_proxy_set_unlocked(struct gpio_shared_proxy_data *proxy,
                         * Current value is low, need to actually set value
                         * to high.
                         */
-                       ret = set_func(desc, 1);
+                       ret = gpiod_set_value_cansleep(desc, 1);
                        if (ret)
                                goto out;
                }
@@ -65,7 +65,7 @@ gpio_shared_proxy_set_unlocked(struct gpio_shared_proxy_data *proxy,
        /* We previously voted for high. */
        if (shared_desc->highcnt == 1) {
                /* This is the last remaining vote for high, set value  to low. */
-               ret = set_func(desc, 0);
+               ret = gpiod_set_value_cansleep(desc, 0);
                if (ret)
                        goto out;
        }
@@ -89,7 +89,7 @@ static int gpio_shared_proxy_request(struct gpio_chip *gc, unsigned int offset)
        struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);
        struct gpio_shared_desc *shared_desc = proxy->shared_desc;
 
-       guard(gpio_shared_desc_lock)(shared_desc);
+       guard(mutex)(&shared_desc->mutex);
 
        proxy->shared_desc->usecnt++;
 
@@ -105,11 +105,10 @@ static void gpio_shared_proxy_free(struct gpio_chip *gc, unsigned int offset)
        struct gpio_shared_desc *shared_desc = proxy->shared_desc;
        int ret;
 
-       guard(gpio_shared_desc_lock)(shared_desc);
+       guard(mutex)(&shared_desc->mutex);
 
        if (proxy->voted_high) {
-               ret = gpio_shared_proxy_set_unlocked(proxy,
-                       shared_desc->can_sleep ? gpiod_set_value_cansleep : gpiod_set_value, 0);
+               ret = gpio_shared_proxy_set_unlocked(proxy, 0);
                if (ret)
                        dev_err(proxy->dev,
                                "Failed to unset the shared GPIO value on release: %d\n", ret);
@@ -129,7 +128,7 @@ static int gpio_shared_proxy_set_config(struct gpio_chip *gc,
        struct gpio_desc *desc = shared_desc->desc;
        int ret;
 
-       guard(gpio_shared_desc_lock)(shared_desc);
+       guard(mutex)(&shared_desc->mutex);
 
        if (shared_desc->usecnt > 1) {
                if (shared_desc->cfg != cfg) {
@@ -157,7 +156,7 @@ static int gpio_shared_proxy_direction_input(struct gpio_chip *gc,
        struct gpio_desc *desc = shared_desc->desc;
        int dir;
 
-       guard(gpio_shared_desc_lock)(shared_desc);
+       guard(mutex)(&shared_desc->mutex);
 
        if (shared_desc->usecnt == 1) {
                dev_dbg(proxy->dev,
@@ -187,7 +186,7 @@ static int gpio_shared_proxy_direction_output(struct gpio_chip *gc,
        struct gpio_desc *desc = shared_desc->desc;
        int ret, dir;
 
-       guard(gpio_shared_desc_lock)(shared_desc);
+       guard(mutex)(&shared_desc->mutex);
 
        if (shared_desc->usecnt == 1) {
                dev_dbg(proxy->dev,
@@ -219,14 +218,7 @@ static int gpio_shared_proxy_direction_output(struct gpio_chip *gc,
                return -EPERM;
        }
 
-       return gpio_shared_proxy_set_unlocked(proxy, gpiod_direction_output, value);
-}
-
-static int gpio_shared_proxy_get(struct gpio_chip *gc, unsigned int offset)
-{
-       struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);
-
-       return gpiod_get_value(proxy->shared_desc->desc);
+       return gpio_shared_proxy_set_unlocked(proxy, value);
 }
 
 static int gpio_shared_proxy_get_cansleep(struct gpio_chip *gc,
@@ -237,29 +229,14 @@ static int gpio_shared_proxy_get_cansleep(struct gpio_chip *gc,
        return gpiod_get_value_cansleep(proxy->shared_desc->desc);
 }
 
-static int gpio_shared_proxy_do_set(struct gpio_shared_proxy_data *proxy,
-                                   int (*set_func)(struct gpio_desc *desc, int value),
-                                   int value)
-{
-       guard(gpio_shared_desc_lock)(proxy->shared_desc);
-
-       return gpio_shared_proxy_set_unlocked(proxy, set_func, value);
-}
-
-static int gpio_shared_proxy_set(struct gpio_chip *gc, unsigned int offset,
-                                int value)
-{
-       struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);
-
-       return gpio_shared_proxy_do_set(proxy, gpiod_set_value, value);
-}
-
 static int gpio_shared_proxy_set_cansleep(struct gpio_chip *gc,
                                          unsigned int offset, int value)
 {
        struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);
 
-       return gpio_shared_proxy_do_set(proxy, gpiod_set_value_cansleep, value);
+       guard(mutex)(&proxy->shared_desc->mutex);
+
+       return gpio_shared_proxy_set_unlocked(proxy, value);
 }
 
 static int gpio_shared_proxy_get_direction(struct gpio_chip *gc,
@@ -302,20 +279,25 @@ static int gpio_shared_proxy_probe(struct auxiliary_device *adev,
        gc->label = dev_name(dev);
        gc->parent = dev;
        gc->owner = THIS_MODULE;
-       gc->can_sleep = shared_desc->can_sleep;
+       /*
+        * Under the descriptor mutex the proxy may call
+        * gpiod_set_config()/gpiod_direction_*(), which can reach pinctrl
+        * paths that take a mutex (e.g. gpiod_set_config() ->
+        * gpiochip_generic_config() -> pinctrl_gpio_set_config()), independent
+        * of the underlying chip's can_sleep. So the descriptor lock must be a
+        * mutex and the proxy gpiochip is therefore always sleeping; drive the
+        * underlying GPIO through the cansleep value accessors, which are valid
+        * for both sleeping and non-sleeping chips.
+        */
+       gc->can_sleep = true;
 
        gc->request = gpio_shared_proxy_request;
        gc->free = gpio_shared_proxy_free;
        gc->set_config = gpio_shared_proxy_set_config;
        gc->direction_input = gpio_shared_proxy_direction_input;
        gc->direction_output = gpio_shared_proxy_direction_output;
-       if (gc->can_sleep) {
-               gc->set = gpio_shared_proxy_set_cansleep;
-               gc->get = gpio_shared_proxy_get_cansleep;
-       } else {
-               gc->set = gpio_shared_proxy_set;
-               gc->get = gpio_shared_proxy_get;
-       }
+       gc->set = gpio_shared_proxy_set_cansleep;
+       gc->get = gpio_shared_proxy_get_cansleep;
        gc->get_direction = gpio_shared_proxy_get_direction;
        gc->to_irq = gpio_shared_proxy_to_irq;
 

diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
index de72776fb154f1f2ec97a3e186dc96366f3cee8b..495bd3d0ddf02cf47f6eacbf66d0215f9dd8fe58 100644 (file)
--- a/drivers/gpio/gpiolib-shared.c
+++ b/drivers/gpio/gpiolib-shared.c
@@ -627,8 +627,7 @@ static void gpio_shared_release(struct kref *kref)
 
        shared_desc = entry->shared_desc;
        gpio_device_put(shared_desc->desc->gdev);
-       if (shared_desc->can_sleep)
-               mutex_destroy(&shared_desc->mutex);
+       mutex_destroy(&shared_desc->mutex);
        kfree(shared_desc);
        entry->shared_desc = NULL;
 }
@@ -659,11 +658,7 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry)
        }
 
        shared_desc->desc = &gdev->descs[entry->offset];
-       shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc);
-       if (shared_desc->can_sleep)
-               mutex_init(&shared_desc->mutex);
-       else
-               spin_lock_init(&shared_desc->spinlock);
+       mutex_init(&shared_desc->mutex);
 
        return shared_desc;
 }

diff --git a/drivers/gpio/gpiolib-shared.h b/drivers/gpio/gpiolib-shared.h
index 15e72a8dcdb138f19ce000a33d3f53cb8f140bce..bbdc0ab7b647a44fca714bed0a7ff75101da7460 100644 (file)
--- a/drivers/gpio/gpiolib-shared.h
+++ b/drivers/gpio/gpiolib-shared.h
@@ -3,10 +3,7 @@
 #ifndef __LINUX_GPIO_SHARED_H
 #define __LINUX_GPIO_SHARED_H
 
-#include <linux/cleanup.h>
-#include <linux/lockdep.h>
 #include <linux/mutex.h>
-#include <linux/spinlock.h>
 
 struct gpio_device;
 struct gpio_desc;
@@ -42,35 +39,12 @@ static inline int gpio_shared_add_proxy_lookup(struct device *consumer,
 
 struct gpio_shared_desc {
        struct gpio_desc *desc;
-       bool can_sleep;
        unsigned long cfg;
        unsigned int usecnt;
        unsigned int highcnt;
-       union {
-               struct mutex mutex;
-               spinlock_t spinlock;
-       };
+       struct mutex mutex; /* serializes all proxy operations on this descriptor */
 };
 
 struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev);
 
-DEFINE_LOCK_GUARD_1(gpio_shared_desc_lock, struct gpio_shared_desc,
-       if (_T->lock->can_sleep)
-               mutex_lock(&_T->lock->mutex);
-       else
-               spin_lock_irqsave(&_T->lock->spinlock, _T->flags),
-       if (_T->lock->can_sleep)
-               mutex_unlock(&_T->lock->mutex);
-       else
-               spin_unlock_irqrestore(&_T->lock->spinlock, _T->flags),
-       unsigned long flags)
-
-static inline void gpio_shared_lockdep_assert(struct gpio_shared_desc *shared_desc)
-{
-       if (shared_desc->can_sleep)
-               lockdep_assert_held(&shared_desc->mutex);
-       else
-               lockdep_assert_held(&shared_desc->spinlock);
-}
-
 #endif /* __LINUX_GPIO_SHARED_H */