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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Oct 12, 2026 at 04:50:00PM +0300, Adrian Ambrozewicz wrote:
> gpiochip_remove() force-frees every IRQ still requested on the chip's
> lines with gpiochip_free_remaining_irqs() before anything else, then
> clears the chip pointer, removes the irqchip, disposing of the IRQ
> mappings, and only at the very end, from gpiolib_cdev_unregister(),
> calls the device notifier on which line requests and v1 line events
> stop edge detection and free their own IRQs. That leaves two windows in
> which the character device frees an IRQ it no longer owns:
> 
> - The notifier runs after the mappings are gone. If an IRQ number freed
>   by gpiochip_irqchip_remove() is allocated to another device in the
>   meantime, irq_has_action() finds that device's action and free_irq()
>   warns "Trying to free already-free IRQ". IRQs requested through the
>   character device after gpiochip_free_remaining_irqs() but before the
>   chip pointer is cleared are still installed when their mappings are
>   disposed of.
> 
> - Releasing a request concurrently with gpiochip_remove() races
>   gpiod_free_irqs(), which can free the action between the release
>   path's irq_has_action() check and its free_irq(), and then
>   gpiochip_irqchip_remove(), which can dispose of the descriptor that
>   free_irq() is still using.
> 
> Call the device notifier from gpiochip_remove() right after the chip
> pointer is cleared and the SRCU grace period has elapsed, when no ioctl
> can request new IRQs any more, and only free the remaining IRQs after
> it. Have the release paths free their IRQs before unregistering from the
> notifier, under the lock that the notifier callback also takes, so that
> the notifier does not return while a release is still freeing an IRQ.
> For v1 line events that needs a new mutex; line requests use their
> config_mutex, which now has to be initialized before the first error
> path. As the chip pointer is NULL by the time gpiod_free_irqs() runs,
> have it call gc->to_irq() directly rather than gpiod_to_irq().
> 
> Every IRQ requested through the character device is thus freed exactly
> once, by the notifier or by the release path, and before
> gpiod_free_irqs() and the removal of the irqchip.
> 
> Moving the character device's part of the removal earlier was proposed
> by Herve Codina before gpiochip_free_remaining_irqs() existed; with that
> in place, only the order of the two is left to fix.
> 
> Reproduced with a test-only GPIO chip that uses the gpiolib hierarchical
> irqchip, unbound from its driver with lines requested through the
> character device. For the first window, the chip hands the IRQ number of
> line 0 to a dummy handler while gpiochip_irqchip_remove() disposes of
> the mappings, and the requests are closed after the unbind. For the
> second, it sleeps in its irq_bus_lock() callback once removal has
> started, and the request is closed while gpiochip_remove() is in
> free_irq(). Each of 30 cycles warns "Trying to free already-free IRQ" in
> both cases, for v2 line requests and v1 line events alike; with this
> change none does.

...

> +	mutex_init(&lr->config_mutex);

You might also want to add the respective mutex_destroy() calls.

...

> +	mutex_init(&le->irq_lock);

Ditto,

...

> -	struct irq_desc *irqd = irq_to_desc(irq);
> +	struct irq_desc *irqd;
>  	void *cookie;
> +	int irq;
> +
> +	/* Called with gdev->chip already NULL, so gpiod_to_irq() can't be used. */
> +	if (!gc->to_irq)
> +		return;
> +
> +	irq = gc->to_irq(gc, gpiod_hwgpio(desc));
> +	if (irq <= 0)

Are you sure about 0?

> +		return;

This can be combined with the above, right?

	if (gc->to_irq)
		irq = gc->to_irq(gc, gpiod_hwgpio(desc));
	else
		irq = -ENOENT;
	if (irq < 0)
		return;

> +	irqd = irq_to_desc(irq);
> +	if (!irqd)
> +		return;

-- 
With Best Regards,
Andy Shevchenko