Skip to content

Commit 3a4bc5d

Browse files
Bartosz Golaszewskivalpackett
authored andcommitted
gpio: shared: fix a deadlock
It's possible that the auxiliary proxy device we add when setting up the GPIO controller exposing shared pins, will get matched and probed immediately. The gpio-proxy-driver will then retrieve the shared descriptor structure. That will cause a recursive mutex locking and a deadlock because we're already holding the gpio_shared_lock in gpio_device_setup_shared() and try to take it again in devm_gpiod_shared_get() like this: [ 4.298346] gpiolib_shared: GPIO 130 owned by f100000.pinctrl is shared by multiple consumers [ 4.307157] gpiolib_shared: Setting up a shared GPIO entry for speaker@0,3 [ 4.314604] [ 4.316146] ============================================ [ 4.321600] WARNING: possible recursive locking detected [ 4.327054] 6.18.0-rc7-next-20251125-g3f300d0674f6-dirty #3887 Not tainted [ 4.334115] -------------------------------------------- [ 4.339566] kworker/u32:3/71 is trying to acquire lock: [ 4.344931] ffffda019ba71850 (gpio_shared_lock){+.+.}-{4:4}, at: devm_gpiod_shared_get+0x34/0x2e0 [ 4.354057] [ 4.354057] but task is already holding lock: [ 4.360041] ffffda019ba71850 (gpio_shared_lock){+.+.}-{4:4}, at: gpio_device_setup_shared+0x30/0x268 [ 4.369421] [ 4.369421] other info that might help us debug this: [ 4.376126] Possible unsafe locking scenario: [ 4.376126] [ 4.382198] CPU0 [ 4.384711] ---- [ 4.387223] lock(gpio_shared_lock); [ 4.390992] lock(gpio_shared_lock); [ 4.394761] [ 4.394761] *** DEADLOCK *** [ 4.394761] [ 4.400832] May be due to missing lock nesting notation [ 4.400832] [ 4.407802] 5 locks held by kworker/u32:3/71: [ 4.412279] #0: ffff000080020948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x194/0x64c [ 4.422650] ColinIanKing#1: ffff800080963d60 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1bc/0x64c [ 4.432117] ColinIanKing#2: ffff00008165c8f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x3c/0x198 [ 4.440700] ColinIanKing#3: ffffda019ba71850 (gpio_shared_lock){+.+.}-{4:4}, at: gpio_device_setup_shared+0x30/0x268 [ 4.450523] ColinIanKing#4: ffff0000810fe918 (&dev->mutex){....}-{4:4}, at: __device_attach+0x3c/0x198 [ 4.459103] [ 4.459103] stack backtrace: [ 4.463581] CPU: 6 UID: 0 PID: 71 Comm: kworker/u32:3 Not tainted 6.18.0-rc7-next-20251125-g3f300d0674f6-dirty #3887 PREEMPT [ 4.463589] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) [ 4.463593] Workqueue: events_unbound deferred_probe_work_func [ 4.463602] Call trace: [ 4.463604] show_stack+0x18/0x24 (C) [ 4.463617] dump_stack_lvl+0x70/0x98 [ 4.463627] dump_stack+0x18/0x24 [ 4.463636] print_deadlock_bug+0x224/0x238 [ 4.463643] __lock_acquire+0xe4c/0x15f0 [ 4.463648] lock_acquire+0x1cc/0x344 [ 4.463653] __mutex_lock+0xb8/0x840 [ 4.463661] mutex_lock_nested+0x24/0x30 [ 4.463667] devm_gpiod_shared_get+0x34/0x2e0 [ 4.463674] gpio_shared_proxy_probe+0x18/0x138 [ 4.463682] auxiliary_bus_probe+0x40/0x78 [ 4.463688] really_probe+0xbc/0x2c0 [ 4.463694] __driver_probe_device+0x78/0x120 [ 4.463701] driver_probe_device+0x3c/0x160 [ 4.463708] __device_attach_driver+0xb8/0x140 [ 4.463716] bus_for_each_drv+0x88/0xe8 [ 4.463723] __device_attach+0xa0/0x198 [ 4.463729] device_initial_probe+0x14/0x20 [ 4.463737] bus_probe_device+0xb4/0xc0 [ 4.463743] device_add+0x578/0x76c [ 4.463747] __auxiliary_device_add+0x40/0xac [ 4.463752] gpio_device_setup_shared+0x1f8/0x268 [ 4.463758] gpiochip_add_data_with_key+0xdac/0x10ac [ 4.463763] devm_gpiochip_add_data_with_key+0x30/0x80 [ 4.463768] msm_pinctrl_probe+0x4b0/0x5e0 [ 4.463779] sm8250_pinctrl_probe+0x18/0x40 [ 4.463784] platform_probe+0x5c/0xa4 [ 4.463793] really_probe+0xbc/0x2c0 [ 4.463800] __driver_probe_device+0x78/0x120 [ 4.463807] driver_probe_device+0x3c/0x160 [ 4.463814] __device_attach_driver+0xb8/0x140 [ 4.463821] bus_for_each_drv+0x88/0xe8 [ 4.463827] __device_attach+0xa0/0x198 [ 4.463834] device_initial_probe+0x14/0x20 [ 4.463841] bus_probe_device+0xb4/0xc0 [ 4.463847] deferred_probe_work_func+0x90/0xcc [ 4.463854] process_one_work+0x214/0x64c [ 4.463860] worker_thread+0x1bc/0x360 [ 4.463866] kthread+0x14c/0x220 [ 4.463871] ret_from_fork+0x10/0x20 [ 77.265041] random: crng init done Fortunately, at the time of creating of the auxiliary device, we already know the correct entry so let's store it as the device's platform data. We don't need to hold gpio_shared_lock in devm_gpiod_shared_get() as we're not removing the entry or traversing the list anymore but we still need to protect it from concurrent modification of its fields so add a more fine-grained mutex. Fixes: a060b8c ("gpiolib: implement low-level, shared GPIO support") Reported-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> Closes: https://lore.kernel.org/all/fimuvblfy2cmn7o4wzcxjzrux5mwhvlvyxfsgeqs6ore2xg75i@ax46d3sfmdux/ Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
1 parent 3cce945 commit 3a4bc5d

File tree

1 file changed

+26
-34
lines changed

1 file changed

+26
-34
lines changed

drivers/gpio/gpiolib-shared.c

Lines changed: 26 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ struct gpio_shared_entry {
4949
unsigned int offset;
5050
/* Index in the property value array. */
5151
size_t index;
52+
struct mutex lock;
5253
struct gpio_shared_desc *shared_desc;
5354
struct kref ref;
5455
struct list_head refs;
@@ -147,6 +148,7 @@ static int gpio_shared_of_traverse(struct device_node *curr)
147148
entry->offset = offset;
148149
entry->index = count;
149150
INIT_LIST_HEAD(&entry->refs);
151+
mutex_init(&entry->lock);
150152

151153
list_add_tail(&entry->list, &gpio_shared_list);
152154
}
@@ -223,6 +225,7 @@ static void gpio_shared_adev_release(struct device *dev)
223225
}
224226

225227
static int gpio_shared_make_adev(struct gpio_device *gdev,
228+
struct gpio_shared_entry *entry,
226229
struct gpio_shared_ref *ref)
227230
{
228231
struct auxiliary_device *adev = &ref->adev;
@@ -235,6 +238,7 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
235238
adev->id = ref->dev_id;
236239
adev->name = "proxy";
237240
adev->dev.parent = gdev->dev.parent;
241+
adev->dev.platform_data = entry;
238242
adev->dev.release = gpio_shared_adev_release;
239243

240244
ret = auxiliary_device_init(adev);
@@ -438,7 +442,7 @@ int gpio_device_setup_shared(struct gpio_device *gdev)
438442
pr_debug("Setting up a shared GPIO entry for %s\n",
439443
fwnode_get_name(ref->fwnode));
440444

441-
ret = gpio_shared_make_adev(gdev, ref);
445+
ret = gpio_shared_make_adev(gdev, entry, ref);
442446
if (ret)
443447
return ret;
444448
}
@@ -474,7 +478,7 @@ static void gpio_shared_release(struct kref *kref)
474478
container_of(kref, struct gpio_shared_entry, ref);
475479
struct gpio_shared_desc *shared_desc = entry->shared_desc;
476480

477-
guard(mutex)(&gpio_shared_lock);
481+
guard(mutex)(&entry->lock);
478482

479483
gpio_device_put(shared_desc->desc->gdev);
480484
if (shared_desc->can_sleep)
@@ -498,6 +502,8 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry)
498502
struct gpio_shared_desc *shared_desc;
499503
struct gpio_device *gdev;
500504

505+
lockdep_assert_held(&entry->lock);
506+
501507
shared_desc = kzalloc(sizeof(*shared_desc), GFP_KERNEL);
502508
if (!shared_desc)
503509
return ERR_PTR(-ENOMEM);
@@ -518,57 +524,42 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry)
518524
return shared_desc;
519525
}
520526

521-
static struct gpio_shared_entry *gpiod_shared_find(struct auxiliary_device *adev)
527+
struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
522528
{
523529
struct gpio_shared_desc *shared_desc;
524530
struct gpio_shared_entry *entry;
525-
struct gpio_shared_ref *ref;
526-
527-
guard(mutex)(&gpio_shared_lock);
531+
int ret;
528532

529-
list_for_each_entry(entry, &gpio_shared_list, list) {
530-
list_for_each_entry(ref, &entry->refs, list) {
531-
if (adev != &ref->adev)
532-
continue;
533+
lockdep_assert_not_held(&gpio_shared_lock);
533534

534-
if (entry->shared_desc) {
535-
kref_get(&entry->ref);
536-
return entry;
537-
}
535+
entry = dev_get_platdata(dev);
536+
if (WARN_ON(!entry))
537+
/* Programmer bug */
538+
return ERR_PTR(-ENOENT);
538539

540+
scoped_guard(mutex, &entry->lock) {
541+
if (entry->shared_desc) {
542+
kref_get(&entry->ref);
543+
shared_desc = entry->shared_desc;
544+
} else {
539545
shared_desc = gpiod_shared_desc_create(entry);
540546
if (IS_ERR(shared_desc))
541547
return ERR_CAST(shared_desc);
542548

543549
kref_init(&entry->ref);
544550
entry->shared_desc = shared_desc;
545-
546-
pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
547-
dev_name(&adev->dev), gpiod_hwgpio(shared_desc->desc),
548-
gpio_device_get_label(shared_desc->desc->gdev));
549-
550-
551-
return entry;
552551
}
553552
}
554553

555-
return ERR_PTR(-ENOENT);
556-
}
557-
558-
struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
559-
{
560-
struct gpio_shared_entry *entry;
561-
int ret;
562-
563-
entry = gpiod_shared_find(to_auxiliary_dev(dev));
564-
if (IS_ERR(entry))
565-
return ERR_CAST(entry);
566-
567554
ret = devm_add_action_or_reset(dev, gpiod_shared_put, entry);
568555
if (ret)
569556
return ERR_PTR(ret);
570557

571-
return entry->shared_desc;
558+
pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
559+
dev_name(dev), gpiod_hwgpio(shared_desc->desc),
560+
gpio_device_get_label(shared_desc->desc->gdev));
561+
562+
return shared_desc;
572563
}
573564
EXPORT_SYMBOL_GPL(devm_gpiod_shared_get);
574565

@@ -584,6 +575,7 @@ static void gpio_shared_drop_ref(struct gpio_shared_ref *ref)
584575
static void gpio_shared_drop_entry(struct gpio_shared_entry *entry)
585576
{
586577
list_del(&entry->list);
578+
mutex_destroy(&entry->lock);
587579
fwnode_handle_put(entry->fwnode);
588580
kfree(entry);
589581
}

0 commit comments

Comments
 (0)