Re: [PATCH v3 6/6] gpiolib: let the character device free its IRQs first on chip removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpiolib: cdev: negative gpiod_get_value_cansleep() treated as rising edge
--

commit 526f1d7ea8a308d5657aec9728c6582d403f73c5
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

gpiolib: let the character device free its IRQs first on chip removal

This commit reorders GPIO chip removal so that the device notifier runs
after gdev->chip is cleared and before remaining IRQs and the irqchip are
freed, while synchronizing character device release paths with the
notifier.

Link: https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e923512febf15..5eb53f8c5d34c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c

[ ... ]

> @@ -1422,7 +1433,6 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
>  	gpiochip_sysfs_unregister(gc);
>  	gpiochip_free_hogs(gc);
> -	gpiochip_free_remaining_irqs(gc);
>  
>  	scoped_guard(mutex, &gpio_devices_lock)
>  		list_del_rcu(&gdev->list);
> @@ -1431,6 +1441,14 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	/* Numb the device, cancelling all outstanding operations */
>  	rcu_assign_pointer(gdev->chip, NULL);
>  	synchronize_srcu(&gdev->srcu);
> +
> +	/*
> +	 * No new interrupts can be requested through the character device now.
> +	 * Have it free the ones it holds before forcibly freeing the rest, and
> +	 * both before the irqchip and its mappings go away.
> +	 */
> +	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
> +	gpiochip_free_remaining_irqs(gc);
>  	gpio_device_teardown_shared(gdev);
>  	gpiochip_irqchip_remove(gc);
>  	acpi_gpiochip_remove(gc);

[Severity: Medium]
With character device IRQs now freed in the notifier callback after
rcu_assign_pointer(gdev->chip, NULL), can an in-flight dual-edge interrupt
handler queue a spurious rising-edge event to userspace?

Previously, gpiochip_free_remaining_irqs() stopped and synchronized all
GPIOD_FLAG_USED_AS_IRQ handlers before gdev->chip was cleared. With this
change, if edge_irq_thread() or lineevent_irq_thread() runs between
rcu_assign_pointer(gdev->chip, NULL) and free_irq() in the notifier,
gpiod_get_value_cansleep() returns -ENODEV.

In drivers/gpio/gpiolib-cdev.c, edge_irq_thread() passes the return value
directly to line_event_id():

	switch (READ_ONCE(line->edflags) & GPIO_V2_LINE_EDGE_FLAGS) {
	case GPIO_V2_LINE_FLAG_EDGE_BOTH:
		le.id = line_event_id(gpiod_get_value_cansleep(line->desc));
		break;

where line_event_id() treats any non-zero value as
GPIO_V2_LINE_EVENT_RISING_EDGE. Similarly, lineevent_irq_thread() checks:

	if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
	    && le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
		int level = gpiod_get_value_cansleep(le->desc);

		if (level)
			/* Emit low-to-high event */
			ge.id = GPIOEVENT_EVENT_RISING_EDGE;

Unlike debounce_work_func(), which drops the event when level < 0, both
threaded IRQ handlers treat -ENODEV as a logical high level and push a
rising-edge event into the kfifo.

Should edge_irq_thread() and lineevent_irq_thread() also check for a
negative return from gpiod_get_value_cansleep() and drop the event?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-gpiolib-chip-removal-v3-0-aambrozewicz@nvidia.com?part=6