|
| 1 | +From e6d479eeaa03b9073304c0475dd72d781911b196 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Viacheslav Bocharov <v@baodeep.com> |
| 3 | +Date: Tue, 9 Jun 2026 16:37:20 +0300 |
| 4 | +Subject: [PATCH] gpio: shared-proxy: always serialize with a sleeping mutex |
| 5 | + |
| 6 | +The shared GPIO descriptor used either a mutex or a spinlock, chosen at |
| 7 | +runtime from the underlying chip's can_sleep: |
| 8 | + |
| 9 | + shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc); |
| 10 | + ... if (can_sleep) mutex_lock(); else spin_lock_irqsave(); |
| 11 | + |
| 12 | +can_sleep describes only the value path (->get/->set). Under the same |
| 13 | +lock, however, the proxy may call gpiod_set_config() and |
| 14 | +gpiod_direction_*(), which can reach pinctrl paths that take a mutex |
| 15 | +(e.g. gpiod_set_config() -> gpiochip_generic_config() -> |
| 16 | +pinctrl_gpio_set_config()), independent of can_sleep. On a controller |
| 17 | +with non-sleeping MMIO value ops the descriptor lock was a spinlock, so |
| 18 | +the sleeping pinctrl call ran from atomic context. Reproduced on an |
| 19 | +Amlogic A113X board with the workaround from commit 28f240683871 |
| 20 | +("pinctrl: meson: mark the GPIO controller as sleeping") reverted; the |
| 21 | +original Khadas VIM3 report hit the same path: |
| 22 | + |
| 23 | + BUG: sleeping function called from invalid context |
| 24 | + __mutex_lock |
| 25 | + pinctrl_get_device_gpio_range |
| 26 | + pinctrl_gpio_set_config |
| 27 | + gpiochip_generic_config |
| 28 | + gpiod_set_config |
| 29 | + gpio_shared_proxy_set_config <- voting spinlock held |
| 30 | + ... |
| 31 | + mmc_pwrseq_simple_probe |
| 32 | + |
| 33 | +The spinlock existed to take the value vote from atomic context, but the |
| 34 | +vote and the (possibly sleeping) control operations share the same state |
| 35 | +and lock, so this scheme cannot serialize config under a mutex and still |
| 36 | +offer atomic value access. Always serialize the shared descriptor with a |
| 37 | +mutex instead and mark the proxy a sleeping gpiochip, driving the |
| 38 | +underlying GPIO through the cansleep value accessors: those are valid |
| 39 | +for both sleeping and non-sleeping chips, so value access keeps working |
| 40 | +on fast controllers, at the cost of no longer being atomic. |
| 41 | + |
| 42 | +This is observable: consumers gating on gpiod_cansleep() take their |
| 43 | +sleeping branch on a proxied GPIO (mmc-pwrseq-emmc skips its |
| 44 | +emergency-restart reset handler; its normal reset is unaffected), and |
| 45 | +consumers that reject sleeping GPIOs (pwm-gpio, ps2-gpio, ...) would |
| 46 | +fail to probe. Such atomic users do not share a pin through the proxy, |
| 47 | +whose purpose is voting on shared reset/enable lines. The same narrowing |
| 48 | +already applies on Amlogic since that workaround, and rockchip |
| 49 | +addressed the identical splat per-driver in commit 7ca497be0016 ("gpio: |
| 50 | +rockchip: Stop calling pinctrl for set_direction"); fixing the proxy |
| 51 | +addresses the locking error once, for every controller. |
| 52 | + |
| 53 | +The lock type was added by commit a060b8c511ab ("gpiolib: implement |
| 54 | +low-level, shared GPIO support"); the sleeping call under it arrived with |
| 55 | +the proxy driver. |
| 56 | + |
| 57 | +Fixes: e992d54c6f97 ("gpio: shared-proxy: implement the shared GPIO proxy driver") |
| 58 | +Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> |
| 59 | +Closes: https://lore.kernel.org/all/00107523-7737-4b92-a785-14ce4e93b8cb@samsung.com/ |
| 60 | +Signed-off-by: Viacheslav Bocharov <v@baodeep.com> |
| 61 | +--- |
| 62 | + drivers/gpio/gpio-shared-proxy.c | 43 +++++++------------------------- |
| 63 | + drivers/gpio/gpiolib-shared.c | 9 ++----- |
| 64 | + drivers/gpio/gpiolib-shared.h | 31 +++++++++-------------- |
| 65 | + 3 files changed, 23 insertions(+), 60 deletions(-) |
| 66 | + |
| 67 | +diff --git a/drivers/gpio/gpio-shared-proxy.c b/drivers/gpio/gpio-shared-proxy.c |
| 68 | +index 6941e4be6cf1..856e5b9d6163 100644 |
| 69 | +--- a/drivers/gpio/gpio-shared-proxy.c |
| 70 | ++++ b/drivers/gpio/gpio-shared-proxy.c |
| 71 | +@@ -109,7 +109,7 @@ static void gpio_shared_proxy_free(struct gpio_chip *gc, unsigned int offset) |
| 72 | + |
| 73 | + if (proxy->voted_high) { |
| 74 | + ret = gpio_shared_proxy_set_unlocked(proxy, |
| 75 | +- shared_desc->can_sleep ? gpiod_set_value_cansleep : gpiod_set_value, 0); |
| 76 | ++ gpiod_set_value_cansleep, 0); |
| 77 | + if (ret) |
| 78 | + dev_err(proxy->dev, |
| 79 | + "Failed to unset the shared GPIO value on release: %d\n", ret); |
| 80 | +@@ -222,13 +222,6 @@ static int gpio_shared_proxy_direction_output(struct gpio_chip *gc, |
| 81 | + return gpio_shared_proxy_set_unlocked(proxy, gpiod_direction_output, value); |
| 82 | + } |
| 83 | + |
| 84 | +-static int gpio_shared_proxy_get(struct gpio_chip *gc, unsigned int offset) |
| 85 | +-{ |
| 86 | +- struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc); |
| 87 | +- |
| 88 | +- return gpiod_get_value(proxy->shared_desc->desc); |
| 89 | +-} |
| 90 | +- |
| 91 | + static int gpio_shared_proxy_get_cansleep(struct gpio_chip *gc, |
| 92 | + unsigned int offset) |
| 93 | + { |
| 94 | +@@ -237,29 +230,15 @@ static int gpio_shared_proxy_get_cansleep(struct gpio_chip *gc, |
| 95 | + return gpiod_get_value_cansleep(proxy->shared_desc->desc); |
| 96 | + } |
| 97 | + |
| 98 | +-static int gpio_shared_proxy_do_set(struct gpio_shared_proxy_data *proxy, |
| 99 | +- int (*set_func)(struct gpio_desc *desc, int value), |
| 100 | +- int value) |
| 101 | +-{ |
| 102 | +- guard(gpio_shared_desc_lock)(proxy->shared_desc); |
| 103 | +- |
| 104 | +- return gpio_shared_proxy_set_unlocked(proxy, set_func, value); |
| 105 | +-} |
| 106 | +- |
| 107 | +-static int gpio_shared_proxy_set(struct gpio_chip *gc, unsigned int offset, |
| 108 | +- int value) |
| 109 | +-{ |
| 110 | +- struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc); |
| 111 | +- |
| 112 | +- return gpio_shared_proxy_do_set(proxy, gpiod_set_value, value); |
| 113 | +-} |
| 114 | +- |
| 115 | + static int gpio_shared_proxy_set_cansleep(struct gpio_chip *gc, |
| 116 | + unsigned int offset, int value) |
| 117 | + { |
| 118 | + struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc); |
| 119 | + |
| 120 | +- return gpio_shared_proxy_do_set(proxy, gpiod_set_value_cansleep, value); |
| 121 | ++ guard(gpio_shared_desc_lock)(proxy->shared_desc); |
| 122 | ++ |
| 123 | ++ return gpio_shared_proxy_set_unlocked(proxy, gpiod_set_value_cansleep, |
| 124 | ++ value); |
| 125 | + } |
| 126 | + |
| 127 | + static int gpio_shared_proxy_get_direction(struct gpio_chip *gc, |
| 128 | +@@ -302,20 +281,16 @@ static int gpio_shared_proxy_probe(struct auxiliary_device *adev, |
| 129 | + gc->label = dev_name(dev); |
| 130 | + gc->parent = dev; |
| 131 | + gc->owner = THIS_MODULE; |
| 132 | +- gc->can_sleep = shared_desc->can_sleep; |
| 133 | ++ /* Always a sleeping gpiochip: see the lock comment in gpiolib-shared.h. */ |
| 134 | ++ gc->can_sleep = true; |
| 135 | + |
| 136 | + gc->request = gpio_shared_proxy_request; |
| 137 | + gc->free = gpio_shared_proxy_free; |
| 138 | + gc->set_config = gpio_shared_proxy_set_config; |
| 139 | + gc->direction_input = gpio_shared_proxy_direction_input; |
| 140 | + gc->direction_output = gpio_shared_proxy_direction_output; |
| 141 | +- if (gc->can_sleep) { |
| 142 | +- gc->set = gpio_shared_proxy_set_cansleep; |
| 143 | +- gc->get = gpio_shared_proxy_get_cansleep; |
| 144 | +- } else { |
| 145 | +- gc->set = gpio_shared_proxy_set; |
| 146 | +- gc->get = gpio_shared_proxy_get; |
| 147 | +- } |
| 148 | ++ gc->set = gpio_shared_proxy_set_cansleep; |
| 149 | ++ gc->get = gpio_shared_proxy_get_cansleep; |
| 150 | + gc->get_direction = gpio_shared_proxy_get_direction; |
| 151 | + gc->to_irq = gpio_shared_proxy_to_irq; |
| 152 | + |
| 153 | +diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c |
| 154 | +index de72776fb154..495bd3d0ddf0 100644 |
| 155 | +--- a/drivers/gpio/gpiolib-shared.c |
| 156 | ++++ b/drivers/gpio/gpiolib-shared.c |
| 157 | +@@ -627,8 +627,7 @@ static void gpio_shared_release(struct kref *kref) |
| 158 | + |
| 159 | + shared_desc = entry->shared_desc; |
| 160 | + gpio_device_put(shared_desc->desc->gdev); |
| 161 | +- if (shared_desc->can_sleep) |
| 162 | +- mutex_destroy(&shared_desc->mutex); |
| 163 | ++ mutex_destroy(&shared_desc->mutex); |
| 164 | + kfree(shared_desc); |
| 165 | + entry->shared_desc = NULL; |
| 166 | + } |
| 167 | +@@ -659,11 +658,7 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry) |
| 168 | + } |
| 169 | + |
| 170 | + shared_desc->desc = &gdev->descs[entry->offset]; |
| 171 | +- shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc); |
| 172 | +- if (shared_desc->can_sleep) |
| 173 | +- mutex_init(&shared_desc->mutex); |
| 174 | +- else |
| 175 | +- spin_lock_init(&shared_desc->spinlock); |
| 176 | ++ mutex_init(&shared_desc->mutex); |
| 177 | + |
| 178 | + return shared_desc; |
| 179 | + } |
| 180 | +diff --git a/drivers/gpio/gpiolib-shared.h b/drivers/gpio/gpiolib-shared.h |
| 181 | +index 15e72a8dcdb1..5c725118b1af 100644 |
| 182 | +--- a/drivers/gpio/gpiolib-shared.h |
| 183 | ++++ b/drivers/gpio/gpiolib-shared.h |
| 184 | +@@ -6,7 +6,6 @@ |
| 185 | + #include <linux/cleanup.h> |
| 186 | + #include <linux/lockdep.h> |
| 187 | + #include <linux/mutex.h> |
| 188 | +-#include <linux/spinlock.h> |
| 189 | + |
| 190 | + struct gpio_device; |
| 191 | + struct gpio_desc; |
| 192 | +@@ -42,35 +41,29 @@ static inline int gpio_shared_add_proxy_lookup(struct device *consumer, |
| 193 | + |
| 194 | + struct gpio_shared_desc { |
| 195 | + struct gpio_desc *desc; |
| 196 | +- bool can_sleep; |
| 197 | + unsigned long cfg; |
| 198 | + unsigned int usecnt; |
| 199 | + unsigned int highcnt; |
| 200 | +- union { |
| 201 | +- struct mutex mutex; |
| 202 | +- spinlock_t spinlock; |
| 203 | +- }; |
| 204 | ++ struct mutex mutex; /* serializes all proxy operations on this descriptor */ |
| 205 | + }; |
| 206 | + |
| 207 | + struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev); |
| 208 | + |
| 209 | ++/* |
| 210 | ++ * Under this lock the proxy may call gpiod_set_config()/gpiod_direction_*(), |
| 211 | ++ * which can reach pinctrl paths that take a mutex (e.g. gpiod_set_config() -> |
| 212 | ++ * gpiochip_generic_config() -> pinctrl_gpio_set_config()), independent of the |
| 213 | ++ * underlying chip's can_sleep. A spinlock would run that sleeping call from |
| 214 | ++ * atomic context, so the descriptor lock must be a mutex and the proxy |
| 215 | ++ * gpiochip is therefore sleeping (can_sleep=true). |
| 216 | ++ */ |
| 217 | + DEFINE_LOCK_GUARD_1(gpio_shared_desc_lock, struct gpio_shared_desc, |
| 218 | +- if (_T->lock->can_sleep) |
| 219 | +- mutex_lock(&_T->lock->mutex); |
| 220 | +- else |
| 221 | +- spin_lock_irqsave(&_T->lock->spinlock, _T->flags), |
| 222 | +- if (_T->lock->can_sleep) |
| 223 | +- mutex_unlock(&_T->lock->mutex); |
| 224 | +- else |
| 225 | +- spin_unlock_irqrestore(&_T->lock->spinlock, _T->flags), |
| 226 | +- unsigned long flags) |
| 227 | ++ mutex_lock(&_T->lock->mutex), |
| 228 | ++ mutex_unlock(&_T->lock->mutex)) |
| 229 | + |
| 230 | + static inline void gpio_shared_lockdep_assert(struct gpio_shared_desc *shared_desc) |
| 231 | + { |
| 232 | +- if (shared_desc->can_sleep) |
| 233 | +- lockdep_assert_held(&shared_desc->mutex); |
| 234 | +- else |
| 235 | +- lockdep_assert_held(&shared_desc->spinlock); |
| 236 | ++ lockdep_assert_held(&shared_desc->mutex); |
| 237 | + } |
| 238 | + |
| 239 | + #endif /* __LINUX_GPIO_SHARED_H */ |
| 240 | +-- |
| 241 | +2.54.0 |
| 242 | + |
0 commit comments