Re: [PATCH v5 7/7] 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:
- [High] gpiolib: gpiochip_remove() frees remaining IRQs before shared proxy teardown
--

commit 07e6ca00e41fdaf7c8e36bd7d53c2988f88d1da3
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 unregistration
notifier is invoked after clearing gdev->chip and waiting for an SRCU grace
period, allowing the character device to free its IRQs before
gpiochip_free_remaining_irqs() force-frees any remaining ones. It also
serializes character device release paths with the removal notifier so IRQs
are freed before unregistering from the notifier chain.

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 ec5f81f4fd9e6..2aba76f79fd6c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c

[ ... ]

> @@ -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);

[Severity: High]
What happens here when the physical gpio_chip exposes shared lines through
gpio-shared-proxy devices?

When a character device client requests edge detection on a proxy gpio_device
(proxy_gdev), gpio_shared_proxy_to_irq() delegates the IRQ mapping to the
underlying physical descriptor:

drivers/gpio/gpio-shared-proxy.c:gpio_shared_proxy_to_irq() {
	struct gpio_shared_proxy_data *proxy = gpiochip_get_data(gc);

	return gpiod_to_irq(proxy->shared_desc->desc);
}

Requesting the IRQ sets GPIOD_FLAG_USED_AS_IRQ on the physical chip's
descriptor via gpiochip_reqres_irq() -> gpiochip_lock_as_irq(), while the
character device registers its device_unregistered_nb notifier on
proxy_gdev->device_notifier.

During gpiochip_remove() on the physical chip, calling
blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL) only notifies
users of the physical gdev. Because gpiochip_free_remaining_irqs(gc) runs
before gpio_device_teardown_shared(gdev) tears down the child proxy devices
and triggers proxy_gdev->device_notifier, gpiod_free_irqs() will force-free
the IRQ still owned by the proxy character device while proxy_gdev is active:

drivers/gpio/gpiolib-cdev.c:free_line_irq() {
	const char *name = *label;

	if (irq && irq_has_action(irq))
		free_irq(irq, dev_id);

	free_irq_label(name);
	*label = NULL;
}

If linereq_free(), lineevent_free(), or linereq_set_config() on proxy_gdev
runs concurrently with gpiod_free_irqs(), could both paths call free_irq() and
trigger the "Trying to free already-free IRQ" warning, or could
free_line_irq() see irq_has_action(irq) return false right after __free_irq()
clears desc->action (before __synchronize_irq() and irq_stop_thread() finish)
and free irq_label and the line state while the IRQ teardown or threaded
handler is still using them?

Should gpio_device_teardown_shared(gdev) be called before
gpiochip_free_remaining_irqs(gc) so that proxy character devices also release
their IRQs via their notifier before the remaining IRQs are force-freed?

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