From 0102fbf52b93e609fec0dab53b1fb4fe69113f5e Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Tue, 11 Mar 2025 18:56:31 +0100 Subject: [PATCH 1/2] gpiolib: don't check the retval of get_direction() when registering a chip During chip registration we should neither check the return value of gc->get_direction() nor hold the SRCU lock when calling it. The former is because pin controllers may have pins set to alternate functions and return errors from their get_direction() callbacks. That's alright - we should default to the safe INPUT state and not bail-out. The latter is not needed because we haven't registered the chip yet so there's nothing to protect against dynamic removal. In fact: we currently hit a lockdep splat. Revert to calling the gc->get_direction() callback directly and *not* checking its value. Fixes: 9d846b1aebbe ("gpiolib: check the return value of gpio_chip::get_direction()") Reported-by: Marek Szyprowski Closes: https://lore.kernel.org/all/81f890fc-6688-42f0-9756-567efc8bb97a@samsung.com/ Reviewed-by: Andy Shevchenko Tested-by: Marek Szyprowski Link: https://lore.kernel.org/r/20250226-retval-fixes-v2-1-c8dc57182441@linaro.org Tested-by: Gene C Link: https://lore.kernel.org/r/20250311175631.83779-1-brgl@bgdev.pl Signed-off-by: Bartosz Golaszewski --- drivers/gpio/gpiolib.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8741600af7ef..de708d081858 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1056,24 +1056,19 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, desc->gdev = gdev; - if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) { - ret = gc->get_direction(gc, desc_index); - if (ret < 0) - /* - * FIXME: Bail-out here once all GPIO drivers - * are updated to not return errors in - * situations that can be considered normal - * operation. - */ - dev_warn(&gdev->dev, - "%s: get_direction failed: %d\n", - __func__, ret); - - assign_bit(FLAG_IS_OUT, &desc->flags, !ret); - } else { + /* + * We would typically want to check the return value of + * get_direction() here but we must not check the return value + * and bail-out as pin controllers can have pins configured to + * alternate functions and return -EINVAL. Also: there's no + * need to take the SRCU lock here. + */ + if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) + assign_bit(FLAG_IS_OUT, &desc->flags, + !gc->get_direction(gc, desc_index)); + else assign_bit(FLAG_IS_OUT, &desc->flags, !gc->direction_input); - } } ret = of_gpiochip_add(gc); From dcb73cbaaeb39c9fd00bf2e019f911725945e2fe Mon Sep 17 00:00:00 2001 From: Bartosz Golaszewski Date: Tue, 11 Mar 2025 15:31:43 +0100 Subject: [PATCH 2/2] gpio: cdev: use raw notifier for line state events We use a notifier to implement the mechanism of informing the user-space about changes in GPIO line status. We register with the notifier when the GPIO character device file is opened and unregister when the last reference to the associated file descriptor is dropped. Since commit fcc8b637c542 ("gpiolib: switch the line state notifier to atomic") we use the atomic notifier variant. Atomic notifiers call rcu_synchronize in atomic_notifier_chain_unregister() which caused a significant performance regression in some circumstances, observed by user-space when calling close() on the GPIO device file descriptor. Replace the atomic notifier with the raw variant and provide synchronization with a read-write spinlock. Fixes: fcc8b637c542 ("gpiolib: switch the line state notifier to atomic") Reported-by: David Jander Closes: https://lore.kernel.org/all/20250311110034.53959031@erd003.prtnl/ Tested-by: David Jander Tested-by: Kent Gibson Link: https://lore.kernel.org/r/20250311-gpiolib-line-state-raw-notifier-v2-1-138374581e1e@linaro.org Signed-off-by: Bartosz Golaszewski --- drivers/gpio/gpiolib-cdev.c | 15 +++++++++------ drivers/gpio/gpiolib.c | 8 +++++--- drivers/gpio/gpiolib.h | 5 ++++- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 40f76a90fd7d..107d75558b5a 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -2729,8 +2729,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) cdev->gdev = gpio_device_get(gdev); cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify; - ret = atomic_notifier_chain_register(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + scoped_guard(write_lock_irqsave, &gdev->line_state_lock) + ret = raw_notifier_chain_register(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); if (ret) goto out_free_bitmap; @@ -2754,8 +2755,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file) blocking_notifier_chain_unregister(&gdev->device_notifier, &cdev->device_unregistered_nb); out_unregister_line_notifier: - atomic_notifier_chain_unregister(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + scoped_guard(write_lock_irqsave, &gdev->line_state_lock) + raw_notifier_chain_unregister(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); out_free_bitmap: gpio_device_put(gdev); bitmap_free(cdev->watched_lines); @@ -2779,8 +2781,9 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file) blocking_notifier_chain_unregister(&gdev->device_notifier, &cdev->device_unregistered_nb); - atomic_notifier_chain_unregister(&gdev->line_state_notifier, - &cdev->lineinfo_changed_nb); + scoped_guard(write_lock_irqsave, &gdev->line_state_lock) + raw_notifier_chain_unregister(&gdev->line_state_notifier, + &cdev->lineinfo_changed_nb); bitmap_free(cdev->watched_lines); gpio_device_put(gdev); kfree(cdev); diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index de708d081858..0c00ed2ab431 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1025,7 +1025,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, } } - ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier); + rwlock_init(&gdev->line_state_lock); + RAW_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier); BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier); ret = init_srcu_struct(&gdev->srcu); @@ -4188,8 +4189,9 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep); void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action) { - atomic_notifier_call_chain(&desc->gdev->line_state_notifier, - action, desc); + guard(read_lock_irqsave)(&desc->gdev->line_state_lock); + + raw_notifier_call_chain(&desc->gdev->line_state_notifier, action, desc); } /** diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 147156ec502b..c129a03e2040 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -45,6 +46,7 @@ * @list: links gpio_device:s together for traversal * @line_state_notifier: used to notify subscribers about lines being * requested, released or reconfigured + * @line_state_lock: RW-spinlock protecting the line state notifier * @line_state_wq: used to emit line state events from a separate thread in * process context * @device_notifier: used to notify character device wait queues about the GPIO @@ -72,7 +74,8 @@ struct gpio_device { const char *label; void *data; struct list_head list; - struct atomic_notifier_head line_state_notifier; + struct raw_notifier_head line_state_notifier; + rwlock_t line_state_lock; struct workqueue_struct *line_state_wq; struct blocking_notifier_head device_notifier; struct srcu_struct srcu;