Skip to content

Commit 54a2df5

Browse files
author
Bartosz Golaszewski
committed
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] linuxppc#1: ffff800080963d60 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1bc/0x64c [ 4.432117] linuxppc#2: ffff00008165c8f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x3c/0x198 [ 4.440700] linuxppc#3: ffffda019ba71850 (gpio_shared_lock){+.+.}-{4:4}, at: gpio_device_setup_shared+0x30/0x268 [ 4.450523] linuxppc#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 <[email protected]> Closes: https://lore.kernel.org/all/fimuvblfy2cmn7o4wzcxjzrux5mwhvlvyxfsgeqs6ore2xg75i@ax46d3sfmdux/ Reviewed-by: Dmitry Baryshkov <[email protected]> Tested-by: Dmitry Baryshkov <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Bartosz Golaszewski <[email protected]>
1 parent 64309e4 commit 54a2df5

File tree

1 file changed

+28
-35
lines changed

1 file changed

+28
-35
lines changed

drivers/gpio/gpiolib-shared.c

Lines changed: 28 additions & 35 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;
@@ -170,6 +171,7 @@ static int gpio_shared_of_traverse(struct device_node *curr)
170171
entry->offset = offset;
171172
entry->index = count;
172173
INIT_LIST_HEAD(&entry->refs);
174+
mutex_init(&entry->lock);
173175

174176
list_add_tail(&entry->list, &gpio_shared_list);
175177
}
@@ -246,6 +248,7 @@ static void gpio_shared_adev_release(struct device *dev)
246248
}
247249

248250
static int gpio_shared_make_adev(struct gpio_device *gdev,
251+
struct gpio_shared_entry *entry,
249252
struct gpio_shared_ref *ref)
250253
{
251254
struct auxiliary_device *adev = &ref->adev;
@@ -258,6 +261,7 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
258261
adev->id = ref->dev_id;
259262
adev->name = "proxy";
260263
adev->dev.parent = gdev->dev.parent;
264+
adev->dev.platform_data = entry;
261265
adev->dev.release = gpio_shared_adev_release;
262266

263267
ret = auxiliary_device_init(adev);
@@ -461,7 +465,7 @@ int gpio_device_setup_shared(struct gpio_device *gdev)
461465
pr_debug("Setting up a shared GPIO entry for %s\n",
462466
fwnode_get_name(ref->fwnode));
463467

464-
ret = gpio_shared_make_adev(gdev, ref);
468+
ret = gpio_shared_make_adev(gdev, entry, ref);
465469
if (ret)
466470
return ret;
467471
}
@@ -495,10 +499,11 @@ static void gpio_shared_release(struct kref *kref)
495499
{
496500
struct gpio_shared_entry *entry =
497501
container_of(kref, struct gpio_shared_entry, ref);
498-
struct gpio_shared_desc *shared_desc = entry->shared_desc;
502+
struct gpio_shared_desc *shared_desc;
499503

500-
guard(mutex)(&gpio_shared_lock);
504+
guard(mutex)(&entry->lock);
501505

506+
shared_desc = entry->shared_desc;
502507
gpio_device_put(shared_desc->desc->gdev);
503508
if (shared_desc->can_sleep)
504509
mutex_destroy(&shared_desc->mutex);
@@ -521,6 +526,8 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry)
521526
struct gpio_shared_desc *shared_desc;
522527
struct gpio_device *gdev;
523528

529+
lockdep_assert_held(&entry->lock);
530+
524531
shared_desc = kzalloc(sizeof(*shared_desc), GFP_KERNEL);
525532
if (!shared_desc)
526533
return ERR_PTR(-ENOMEM);
@@ -541,57 +548,42 @@ gpiod_shared_desc_create(struct gpio_shared_entry *entry)
541548
return shared_desc;
542549
}
543550

544-
static struct gpio_shared_entry *gpiod_shared_find(struct auxiliary_device *adev)
551+
struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
545552
{
546553
struct gpio_shared_desc *shared_desc;
547554
struct gpio_shared_entry *entry;
548-
struct gpio_shared_ref *ref;
549-
550-
guard(mutex)(&gpio_shared_lock);
555+
int ret;
551556

552-
list_for_each_entry(entry, &gpio_shared_list, list) {
553-
list_for_each_entry(ref, &entry->refs, list) {
554-
if (adev != &ref->adev)
555-
continue;
557+
lockdep_assert_not_held(&gpio_shared_lock);
556558

557-
if (entry->shared_desc) {
558-
kref_get(&entry->ref);
559-
return entry;
560-
}
559+
entry = dev_get_platdata(dev);
560+
if (WARN_ON(!entry))
561+
/* Programmer bug */
562+
return ERR_PTR(-ENOENT);
561563

564+
scoped_guard(mutex, &entry->lock) {
565+
if (entry->shared_desc) {
566+
kref_get(&entry->ref);
567+
shared_desc = entry->shared_desc;
568+
} else {
562569
shared_desc = gpiod_shared_desc_create(entry);
563570
if (IS_ERR(shared_desc))
564571
return ERR_CAST(shared_desc);
565572

566573
kref_init(&entry->ref);
567574
entry->shared_desc = shared_desc;
568-
569-
pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
570-
dev_name(&adev->dev), gpiod_hwgpio(shared_desc->desc),
571-
gpio_device_get_label(shared_desc->desc->gdev));
572-
573-
574-
return entry;
575575
}
576-
}
577576

578-
return ERR_PTR(-ENOENT);
579-
}
580-
581-
struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
582-
{
583-
struct gpio_shared_entry *entry;
584-
int ret;
585-
586-
entry = gpiod_shared_find(to_auxiliary_dev(dev));
587-
if (IS_ERR(entry))
588-
return ERR_CAST(entry);
577+
pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
578+
dev_name(dev), gpiod_hwgpio(shared_desc->desc),
579+
gpio_device_get_label(shared_desc->desc->gdev));
580+
}
589581

590582
ret = devm_add_action_or_reset(dev, gpiod_shared_put, entry);
591583
if (ret)
592584
return ERR_PTR(ret);
593585

594-
return entry->shared_desc;
586+
return shared_desc;
595587
}
596588
EXPORT_SYMBOL_GPL(devm_gpiod_shared_get);
597589

@@ -607,6 +599,7 @@ static void gpio_shared_drop_ref(struct gpio_shared_ref *ref)
607599
static void gpio_shared_drop_entry(struct gpio_shared_entry *entry)
608600
{
609601
list_del(&entry->list);
602+
mutex_destroy(&entry->lock);
610603
fwnode_handle_put(entry->fwnode);
611604
kfree(entry);
612605
}

0 commit comments

Comments
 (0)