Skip to content

Commit ea513dd

Browse files
author
Bartosz Golaszewski
committed
gpio: shared: make locking more fine-grained
The global gpio_shared_lock has caused some issues when recursively removing GPIO shared proxies. The thing is: we don't really need it. Once created from an init-call, the shared GPIO data structures are never removed, there's no need to protect the linked lists of entries and references. All we need to protect is the shared GPIO descriptor (which we already do with a per-entry mutex) and the auxiliary device data in struct gpio_shared_ref. Remove the global gpio_shared_lock and use a per-reference mutex to protect shared references when adding/removing the auxiliary devices and their GPIO lookup entries. Link: https://lore.kernel.org/r/20251206-gpio-shared-teardown-fixes-v1-4-35ac458cfce1@oss.qualcomm.com Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
1 parent d382c76 commit ea513dd

1 file changed

Lines changed: 20 additions & 14 deletions

File tree

drivers/gpio/gpiolib-shared.c

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ struct gpio_shared_ref {
3636
enum gpiod_flags flags;
3737
char *con_id;
3838
int dev_id;
39+
/* Protects the auxiliary device struct and the lookup table. */
40+
struct mutex lock;
3941
struct auxiliary_device adev;
4042
struct gpiod_lookup_table *lookup;
4143
};
@@ -49,14 +51,14 @@ struct gpio_shared_entry {
4951
unsigned int offset;
5052
/* Index in the property value array. */
5153
size_t index;
54+
/* Synchronizes the modification of shared_desc. */
5255
struct mutex lock;
5356
struct gpio_shared_desc *shared_desc;
5457
struct kref ref;
5558
struct list_head refs;
5659
};
5760

5861
static LIST_HEAD(gpio_shared_list);
59-
static DEFINE_MUTEX(gpio_shared_lock);
6062
static DEFINE_IDA(gpio_shared_ida);
6163

6264
#if IS_ENABLED(CONFIG_OF)
@@ -187,6 +189,7 @@ static int gpio_shared_of_traverse(struct device_node *curr)
187189

188190
ref->fwnode = fwnode_handle_get(of_fwnode_handle(curr));
189191
ref->flags = args.args[1];
192+
mutex_init(&ref->lock);
190193

191194
if (strends(prop->name, "gpios"))
192195
suffix = "-gpios";
@@ -258,7 +261,7 @@ static int gpio_shared_make_adev(struct gpio_device *gdev,
258261
struct auxiliary_device *adev = &ref->adev;
259262
int ret;
260263

261-
lockdep_assert_held(&gpio_shared_lock);
264+
guard(mutex)(&ref->lock);
262265

263266
memset(adev, 0, sizeof(*adev));
264267

@@ -373,14 +376,14 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
373376
if (!lookup)
374377
return -ENOMEM;
375378

376-
guard(mutex)(&gpio_shared_lock);
377-
378379
list_for_each_entry(entry, &gpio_shared_list, list) {
379380
list_for_each_entry(ref, &entry->refs, list) {
380381
if (!device_match_fwnode(consumer, ref->fwnode) &&
381382
!gpio_shared_dev_is_reset_gpio(consumer, entry, ref))
382383
continue;
383384

385+
guard(mutex)(&ref->lock);
386+
384387
/* We've already done that on a previous request. */
385388
if (ref->lookup)
386389
return 0;
@@ -413,8 +416,6 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, unsigned long lflags)
413416

414417
static void gpio_shared_remove_adev(struct auxiliary_device *adev)
415418
{
416-
lockdep_assert_held(&gpio_shared_lock);
417-
418419
auxiliary_device_delete(adev);
419420
auxiliary_device_uninit(adev);
420421
}
@@ -426,8 +427,6 @@ int gpio_device_setup_shared(struct gpio_device *gdev)
426427
unsigned long *flags;
427428
int ret;
428429

429-
guard(mutex)(&gpio_shared_lock);
430-
431430
list_for_each_entry(entry, &gpio_shared_list, list) {
432431
list_for_each_entry(ref, &entry->refs, list) {
433432
if (gdev->dev.parent == &ref->adev.dev) {
@@ -484,13 +483,22 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
484483
struct gpio_shared_entry *entry;
485484
struct gpio_shared_ref *ref;
486485

487-
guard(mutex)(&gpio_shared_lock);
488-
489486
list_for_each_entry(entry, &gpio_shared_list, list) {
490487
if (!device_match_fwnode(&gdev->dev, entry->fwnode))
491488
continue;
492489

490+
/*
491+
* For some reason if we call synchronize_srcu() in GPIO core,
492+
* descent here and take this mutex and then recursively call
493+
* synchronize_srcu() again from gpiochip_remove() (which is
494+
* totally fine) called after gpio_shared_remove_adev(),
495+
* lockdep prints a false positive deadlock splat. Disable
496+
* lockdep here.
497+
*/
498+
lockdep_off();
493499
list_for_each_entry(ref, &entry->refs, list) {
500+
guard(mutex)(&ref->lock);
501+
494502
if (ref->lookup) {
495503
gpiod_remove_lookup_table(ref->lookup);
496504
kfree(ref->lookup->table[0].key);
@@ -500,6 +508,7 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
500508

501509
gpio_shared_remove_adev(&ref->adev);
502510
}
511+
lockdep_on();
503512
}
504513
}
505514

@@ -523,8 +532,6 @@ static void gpiod_shared_put(void *data)
523532
{
524533
struct gpio_shared_entry *entry = data;
525534

526-
lockdep_assert_not_held(&gpio_shared_lock);
527-
528535
kref_put(&entry->ref, gpio_shared_release);
529536
}
530537

@@ -562,8 +569,6 @@ struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
562569
struct gpio_shared_entry *entry;
563570
int ret;
564571

565-
lockdep_assert_not_held(&gpio_shared_lock);
566-
567572
entry = dev_get_platdata(dev);
568573
if (WARN_ON(!entry))
569574
/* Programmer bug */
@@ -598,6 +603,7 @@ EXPORT_SYMBOL_GPL(devm_gpiod_shared_get);
598603
static void gpio_shared_drop_ref(struct gpio_shared_ref *ref)
599604
{
600605
list_del(&ref->list);
606+
mutex_destroy(&ref->lock);
601607
kfree(ref->con_id);
602608
ida_free(&gpio_shared_ida, ref->dev_id);
603609
fwnode_handle_put(ref->fwnode);

0 commit comments

Comments
 (0)