[PATCH v2 0/3] gpiolib: use a read-write semaphore to protect the GPIO device list

[PATCH v2 0/3] gpiolib: use a read-write semaphore to protect the GPIO device list

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I'm still figuring out how to keep GPIO descriptors coherent while
(mostly) lockless. In the meantime, I found a potential race-condition
during GPIO descriptor lookup and also figured that the correct way to
protect the GPIO device list is actually a read-write semaphore as we're
not modifying the list very often and readers should be able to iterate
over it concurrently.

The first patch in this series is new in v2. I realized that we must not
wait until .release() to remove the GPIO device from the list as this is
why pinning down the GPIO device list during lookup would never work -
we always could end up re-taking a reference to an object that was being
released if it got looked up between when the last reference is dropped
and the object is finally removed from the device list.

v1 -> v2:
- add patch 1/3 to fix a release timing issue

Bartosz Golaszewski (3):
  gpiolib: remove the GPIO device from the list when it's unregistered
  gpiolib: replace the GPIO device mutex with a read-write semaphore
  gpiolib: pin GPIO devices in place during descriptor lookup

 drivers/gpio/gpiolib-sysfs.c |  2 +-
 drivers/gpio/gpiolib.c       | 62 ++++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |  2 +-
 3 files changed, 36 insertions(+), 30 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] gpiolib: remove the GPIO device from the list when it's unregistered

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If we wait until the GPIO device's .release() callback gets invoked
before we remove it from the global device list, then we risk that
someone will look it up using gpio_device_find() between where we
dropped the last reference and before .release() is done taking a
reference again to an object that's being released.

The device must be removed when it's being unregistered - just like how
we remove it from the GPIO bus.

Fixes: ff2b13592299 ("gpio: make the gpiochip a real device")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e21497b989a1..e019c4243809 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -651,9 +651,6 @@ static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
 
-	scoped_guard(mutex, &gpio_devices_lock)
-		list_del(&gdev->list);
-
 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
 	kfree(gdev->descs);
@@ -1068,6 +1065,9 @@ void gpiochip_remove(struct gpio_chip *gc)
 		dev_crit(&gdev->dev,
 			 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
+	scoped_guard(mutex, &gpio_devices_lock)
+		list_del(&gdev->list);
+
 	/*
 	 * The gpiochip side puts its use of the device to rest here:
 	 * if there are no userspace clients, the chardev and device will
-- 
2.40.1

[PATCH v2 2/3] gpiolib: replace the GPIO device mutex with a read-write semaphore

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There are only two spots where we modify (add to or remove objects from)
the GPIO device list. Readers should be able to access it concurrently.
Replace the mutex with a read-write semaphore and adjust the locking
operations accordingly.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-sysfs.c |  2 +-
 drivers/gpio/gpiolib.c       | 18 +++++++++---------
 drivers/gpio/gpiolib.h       |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2d29ae37d953..4dbf298bb5dd 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -773,7 +773,7 @@ int gpiochip_sysfs_register_all(void)
 	struct gpio_device *gdev;
 	int ret;
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->mockdev)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e019c4243809..4c93cf73a826 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -85,7 +85,7 @@ static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 
 LIST_HEAD(gpio_devices);
-DEFINE_MUTEX(gpio_devices_lock);
+DECLARE_RWSEM(gpio_devices_sem);
 
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);
@@ -118,7 +118,7 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
 	struct gpio_device *gdev;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
 		list_for_each_entry(gdev, &gpio_devices, list) {
 			if (gdev->base <= gpio &&
 			    gdev->base + gdev->ngpio > gpio)
@@ -402,7 +402,7 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name)
 	if (!name)
 		return NULL;
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		struct gpio_desc *desc;
@@ -871,7 +871,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	gdev->ngpio = gc->ngpio;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_write, &gpio_devices_sem) {
 		/*
 		 * TODO: this allocates a Linux GPIO number base in the global
 		 * GPIO numberspace for this chip. In the long run we want to
@@ -1001,7 +1001,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		goto err_print_message;
 	}
 err_remove_from_list:
-	scoped_guard(mutex, &gpio_devices_lock)
+	scoped_guard(rwsem_write, &gpio_devices_sem)
 		list_del(&gdev->list);
 err_free_label:
 	kfree_const(gdev->label);
@@ -1065,7 +1065,7 @@ void gpiochip_remove(struct gpio_chip *gc)
 		dev_crit(&gdev->dev,
 			 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
 
-	scoped_guard(mutex, &gpio_devices_lock)
+	scoped_guard(rwsem_write, &gpio_devices_sem)
 		list_del(&gdev->list);
 
 	/*
@@ -1114,7 +1114,7 @@ struct gpio_device *gpio_device_find(void *data,
 	 */
 	might_sleep();
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->chip && match(gdev->chip, data))
@@ -4730,7 +4730,7 @@ static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 
 	s->private = "";
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (index-- == 0)
@@ -4745,7 +4745,7 @@ static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
 	struct gpio_device *gdev = v;
 	void *ret = NULL;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
 		if (list_is_last(&gdev->list, &gpio_devices))
 			ret = NULL;
 		else
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 0ce7451a6b24..97df54abf57a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -137,7 +137,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 extern spinlock_t gpio_lock;
 extern struct list_head gpio_devices;
-extern struct mutex gpio_devices_lock;
+extern struct rw_semaphore gpio_devices_sem;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
 
-- 
2.40.1

[PATCH v2 3/3] gpiolib: pin GPIO devices in place during descriptor lookup

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

There's time between when we locate the relevant descriptor during
lookup and when we actually take the reference to its parent GPIO
device where - if the GPIO device in question is removed - we'll end up
with a dangling pointer to freed memory. Make sure devices cannot be
removed until we hold a new reference to the device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4c93cf73a826..be57f8d6aeae 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4134,27 +4134,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 	struct gpio_desc *desc;
 	int ret;
 
-	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
-	if (gpiod_not_found(desc) && platform_lookup_allowed) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
+		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
+					    &flags, &lookupflags);
+		if (gpiod_not_found(desc) && platform_lookup_allowed) {
+			/*
+			 * Either we are not using DT or ACPI, or their lookup
+			 * did not return a result. In that case, use platform
+			 * lookup as a fallback.
+			 */
+			dev_dbg(consumer,
+				"using lookup tables for GPIO lookup\n");
+			desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+		}
+
+		if (IS_ERR(desc)) {
+			dev_dbg(consumer, "No GPIO consumer %s found\n",
+				con_id);
+			return desc;
+		}
+
 		/*
-		 * Either we are not using DT or ACPI, or their lookup did not
-		 * return a result. In that case, use platform lookup as a
-		 * fallback.
+		 * If a connection label was passed use that, else attempt to
+		 * use the device name as label
 		 */
-		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
-		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+		ret = gpiod_request(desc, label);
 	}
 
-	if (IS_ERR(desc)) {
-		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
-		return desc;
-	}
-
-	/*
-	 * If a connection label was passed use that, else attempt to use
-	 * the device name as label
-	 */
-	ret = gpiod_request(desc, label);
 	if (ret) {
 		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
 			return ERR_PTR(ret);
-- 
2.40.1

Re: [PATCH v2 1/3] gpiolib: remove the GPIO device from the list when it's unregistered

[Linus Walleij]

On Tue, Jan 2, 2024 at 4:59 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> If we wait until the GPIO device's .release() callback gets invoked
> before we remove it from the global device list, then we risk that
> someone will look it up using gpio_device_find() between where we
> dropped the last reference and before .release() is done taking a
> reference again to an object that's being released.
>
> The device must be removed when it's being unregistered - just like how
> we remove it from the GPIO bus.
>
> Fixes: ff2b13592299 ("gpio: make the gpiochip a real device")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Makes sense!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 2/3] gpiolib: replace the GPIO device mutex with a read-write semaphore

[Linus Walleij]

On Tue, Jan 2, 2024 at 4:59 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There are only two spots where we modify (add to or remove objects from)
> the GPIO device list. Readers should be able to access it concurrently.
> Replace the mutex with a read-write semaphore and adjust the locking
> operations accordingly.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Looks correct to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 3/3] gpiolib: pin GPIO devices in place during descriptor lookup

[Linus Walleij]

On Tue, Jan 2, 2024 at 4:59 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There's time between when we locate the relevant descriptor during
> lookup and when we actually take the reference to its parent GPIO
> device where - if the GPIO device in question is removed - we'll end up
> with a dangling pointer to freed memory. Make sure devices cannot be
> removed until we hold a new reference to the device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Also looks right to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 0/3] gpiolib: use a read-write semaphore to protect the GPIO device list

[Bartosz Golaszewski]

On Tue, Jan 2, 2024 at 4:59 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> I'm still figuring out how to keep GPIO descriptors coherent while
> (mostly) lockless. In the meantime, I found a potential race-condition
> during GPIO descriptor lookup and also figured that the correct way to
> protect the GPIO device list is actually a read-write semaphore as we're
> not modifying the list very often and readers should be able to iterate
> over it concurrently.
>
> The first patch in this series is new in v2. I realized that we must not
> wait until .release() to remove the GPIO device from the list as this is
> why pinning down the GPIO device list during lookup would never work -
> we always could end up re-taking a reference to an object that was being
> released if it got looked up between when the last reference is dropped
> and the object is finally removed from the device list.
>
> v1 -> v2:
> - add patch 1/3 to fix a release timing issue
>
> Bartosz Golaszewski (3):
>   gpiolib: remove the GPIO device from the list when it's unregistered
>   gpiolib: replace the GPIO device mutex with a read-write semaphore
>   gpiolib: pin GPIO devices in place during descriptor lookup
>
>  drivers/gpio/gpiolib-sysfs.c |  2 +-
>  drivers/gpio/gpiolib.c       | 62 ++++++++++++++++++++----------------
>  drivers/gpio/gpiolib.h       |  2 +-
>  3 files changed, 36 insertions(+), 30 deletions(-)
>
> --
> 2.40.1
>

Series applied.

Bart

Re: [PATCH v2 3/3] gpiolib: pin GPIO devices in place during descriptor lookup

[Marek Szyprowski]

On 02.01.2024 16:59, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> There's time between when we locate the relevant descriptor during
> lookup and when we actually take the reference to its parent GPIO
> device where - if the GPIO device in question is removed - we'll end up
> with a dangling pointer to freed memory. Make sure devices cannot be
> removed until we hold a new reference to the device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This patch landed in linux-next as commit db660b9a9f86 ("gpiolib: pin 
GPIO devices in place during descriptor lookup"). Unfortunately it 
introduces a following lock-dep warning:

============================================
WARNING: possible recursive locking detected
6.7.0-rc7-00062-gdb660b9a9f86 #7819 Not tainted
--------------------------------------------
kworker/u4:2/27 is trying to acquire lock:
c13f4e1c (gpio_devices_sem){++++}-{3:3}, at: gpio_device_find+0x30/0x94

but task is already holding lock:
c13f4e1c (gpio_devices_sem){++++}-{3:3}, at: 
gpiod_find_and_request+0x44/0x594

other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(gpio_devices_sem);
   lock(gpio_devices_sem);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

4 locks held by kworker/u4:2/27:
  #0: c1c06ca8 ((wq_completion)events_unbound){+.+.}-{0:0}, at: 
process_one_work+0x148/0x608
  #1: e093df20 ((work_completion)(&entry->work)){+.+.}-{0:0}, at: 
process_one_work+0x148/0x608
  #2: c1f3048c (&dev->mutex){....}-{3:3}, at: 
__driver_attach_async_helper+0x38/0xec
  #3: c13f4e1c (gpio_devices_sem){++++}-{3:3}, at: 
gpiod_find_and_request+0x44/0x594

stack backtrace:
CPU: 0 PID: 27 Comm: kworker/u4:2 Not tainted 
6.7.0-rc7-00062-gdb660b9a9f86 #7819
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound async_run_entry_fn
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x58/0x70
  dump_stack_lvl from __lock_acquire+0x1300/0x2984
  __lock_acquire from lock_acquire+0x130/0x37c
  lock_acquire from down_read+0x44/0x224
  down_read from gpio_device_find+0x30/0x94
  gpio_device_find from of_get_named_gpiod_flags+0xa4/0x3a8
  of_get_named_gpiod_flags from of_find_gpio+0x80/0x168
  of_find_gpio from gpiod_find_and_request+0x120/0x594
  gpiod_find_and_request from gpiod_get_optional+0x54/0x90
  gpiod_get_optional from reg_fixed_voltage_probe+0x200/0x400
  reg_fixed_voltage_probe from platform_probe+0x5c/0xb8
  platform_probe from really_probe+0xe0/0x400
  really_probe from __driver_probe_device+0x9c/0x1f0
  __driver_probe_device from driver_probe_device+0x30/0xc0
  driver_probe_device from __driver_attach_async_helper+0x54/0xec
  __driver_attach_async_helper from async_run_entry_fn+0x40/0x154
  async_run_entry_fn from process_one_work+0x204/0x608
  process_one_work from worker_thread+0x1e0/0x498
  worker_thread from kthread+0x104/0x138
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xe093dfb0 to 0xe093dff8)
...


Taking gpio_devices_sem more than once for reading is safe, but it looks 
that it needs some lock-dep annotations to to make it happy and avoid 
the above warning.


> ---
>   drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
>   1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4c93cf73a826..be57f8d6aeae 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4134,27 +4134,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>   	struct gpio_desc *desc;
>   	int ret;
>   
> -	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
> -	if (gpiod_not_found(desc) && platform_lookup_allowed) {
> +	scoped_guard(rwsem_read, &gpio_devices_sem) {
> +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> +					    &flags, &lookupflags);
> +		if (gpiod_not_found(desc) && platform_lookup_allowed) {
> +			/*
> +			 * Either we are not using DT or ACPI, or their lookup
> +			 * did not return a result. In that case, use platform
> +			 * lookup as a fallback.
> +			 */
> +			dev_dbg(consumer,
> +				"using lookup tables for GPIO lookup\n");
> +			desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +		}
> +
> +		if (IS_ERR(desc)) {
> +			dev_dbg(consumer, "No GPIO consumer %s found\n",
> +				con_id);
> +			return desc;
> +		}
> +
>   		/*
> -		 * Either we are not using DT or ACPI, or their lookup did not
> -		 * return a result. In that case, use platform lookup as a
> -		 * fallback.
> +		 * If a connection label was passed use that, else attempt to
> +		 * use the device name as label
>   		 */
> -		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> -		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +		ret = gpiod_request(desc, label);
>   	}
>   
> -	if (IS_ERR(desc)) {
> -		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> -		return desc;
> -	}
> -
> -	/*
> -	 * If a connection label was passed use that, else attempt to use
> -	 * the device name as label
> -	 */
> -	ret = gpiod_request(desc, label);
>   	if (ret) {
>   		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
>   			return ERR_PTR(ret);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 3/3] gpiolib: pin GPIO devices in place during descriptor lookup

[Bartosz Golaszewski]

On Mon, Jan 8, 2024 at 2:03 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 02.01.2024 16:59, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > There's time between when we locate the relevant descriptor during
> > lookup and when we actually take the reference to its parent GPIO
> > device where - if the GPIO device in question is removed - we'll end up
> > with a dangling pointer to freed memory. Make sure devices cannot be
> > removed until we hold a new reference to the device.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> This patch landed in linux-next as commit db660b9a9f86 ("gpiolib: pin
> GPIO devices in place during descriptor lookup"). Unfortunately it
> introduces a following lock-dep warning:
>
> ============================================
> WARNING: possible recursive locking detected
> 6.7.0-rc7-00062-gdb660b9a9f86 #7819 Not tainted
> --------------------------------------------
> kworker/u4:2/27 is trying to acquire lock:
> c13f4e1c (gpio_devices_sem){++++}-{3:3}, at: gpio_device_find+0x30/0x94
>
> but task is already holding lock:
> c13f4e1c (gpio_devices_sem){++++}-{3:3}, at:
> gpiod_find_and_request+0x44/0x594
>
> other info that might help us debug this:
>   Possible unsafe locking scenario:
>
>         CPU0
>         ----
>    lock(gpio_devices_sem);
>    lock(gpio_devices_sem);
>
>   *** DEADLOCK ***
>
>   May be due to missing lock nesting notation
>
> 4 locks held by kworker/u4:2/27:
>   #0: c1c06ca8 ((wq_completion)events_unbound){+.+.}-{0:0}, at:
> process_one_work+0x148/0x608
>   #1: e093df20 ((work_completion)(&entry->work)){+.+.}-{0:0}, at:
> process_one_work+0x148/0x608
>   #2: c1f3048c (&dev->mutex){....}-{3:3}, at:
> __driver_attach_async_helper+0x38/0xec
>   #3: c13f4e1c (gpio_devices_sem){++++}-{3:3}, at:
> gpiod_find_and_request+0x44/0x594
>
> stack backtrace:
> CPU: 0 PID: 27 Comm: kworker/u4:2 Not tainted
> 6.7.0-rc7-00062-gdb660b9a9f86 #7819
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events_unbound async_run_entry_fn
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x58/0x70
>   dump_stack_lvl from __lock_acquire+0x1300/0x2984
>   __lock_acquire from lock_acquire+0x130/0x37c
>   lock_acquire from down_read+0x44/0x224
>   down_read from gpio_device_find+0x30/0x94
>   gpio_device_find from of_get_named_gpiod_flags+0xa4/0x3a8
>   of_get_named_gpiod_flags from of_find_gpio+0x80/0x168
>   of_find_gpio from gpiod_find_and_request+0x120/0x594
>   gpiod_find_and_request from gpiod_get_optional+0x54/0x90
>   gpiod_get_optional from reg_fixed_voltage_probe+0x200/0x400
>   reg_fixed_voltage_probe from platform_probe+0x5c/0xb8
>   platform_probe from really_probe+0xe0/0x400
>   really_probe from __driver_probe_device+0x9c/0x1f0
>   __driver_probe_device from driver_probe_device+0x30/0xc0
>   driver_probe_device from __driver_attach_async_helper+0x54/0xec
>   __driver_attach_async_helper from async_run_entry_fn+0x40/0x154
>   async_run_entry_fn from process_one_work+0x204/0x608
>   process_one_work from worker_thread+0x1e0/0x498
>   worker_thread from kthread+0x104/0x138
>   kthread from ret_from_fork+0x14/0x28
> Exception stack(0xe093dfb0 to 0xe093dff8)
> ...
>
>
> Taking gpio_devices_sem more than once for reading is safe, but it looks
> that it needs some lock-dep annotations to to make it happy and avoid
> the above warning.
>
>
> > ---
> >   drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
> >   1 file changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 4c93cf73a826..be57f8d6aeae 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -4134,27 +4134,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
> >       struct gpio_desc *desc;
> >       int ret;
> >
> > -     desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
> > -     if (gpiod_not_found(desc) && platform_lookup_allowed) {
> > +     scoped_guard(rwsem_read, &gpio_devices_sem) {
> > +             desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> > +                                         &flags, &lookupflags);
> > +             if (gpiod_not_found(desc) && platform_lookup_allowed) {
> > +                     /*
> > +                      * Either we are not using DT or ACPI, or their lookup
> > +                      * did not return a result. In that case, use platform
> > +                      * lookup as a fallback.
> > +                      */
> > +                     dev_dbg(consumer,
> > +                             "using lookup tables for GPIO lookup\n");
> > +                     desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> > +             }
> > +
> > +             if (IS_ERR(desc)) {
> > +                     dev_dbg(consumer, "No GPIO consumer %s found\n",
> > +                             con_id);
> > +                     return desc;
> > +             }
> > +
> >               /*
> > -              * Either we are not using DT or ACPI, or their lookup did not
> > -              * return a result. In that case, use platform lookup as a
> > -              * fallback.
> > +              * If a connection label was passed use that, else attempt to
> > +              * use the device name as label
> >                */
> > -             dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> > -             desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> > +             ret = gpiod_request(desc, label);
> >       }
> >
> > -     if (IS_ERR(desc)) {
> > -             dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> > -             return desc;
> > -     }
> > -
> > -     /*
> > -      * If a connection label was passed use that, else attempt to use
> > -      * the device name as label
> > -      */
> > -     ret = gpiod_request(desc, label);
> >       if (ret) {
> >               if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
> >                       return ERR_PTR(ret);
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Thanks for the report. I think it may have come too late in the
release cycle as it has the potential to break a lot of things. I will
back it out of my for-next branch. I'll resend it for v6.9.

Bart