| Message ID | 20240919135104.3583-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpio: free irqs that are still requested when the chip is being removed | expand |
Hi Bartosz, On Thu, 19 Sep 2024 15:51:04 +0200 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > If we remove a GPIO chip that is also an interrupt controller with users > not having freed some interrupts, we'll end up leaking resources as > indicated by the following warning: > > remove_proc_entry: removing non-empty directory 'irq/30', leaking at least 'gpio' > > As there's no way of notifying interrupt users about the irqchip going > away and the interrupt subsystem is not plugged into the driver model and > so not all cases can be handled by devlinks, we need to make sure to free > all interrupts before the complete the removal of the provider. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > v1 -> v2: > - we should actually take the request_mutex to protect the irqaction from being > freed while we dereference it and keep the actual dereferencing under the lock > - add some comments to explain what we're doing > > drivers/gpio/gpiolib.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) Thanks for the patch. Tested on my system and it fixes the issue. Reviewed-by: Herve Codina <herve.codina@bootlin.com> Tested-by: Herve Codina <herve.codina@bootlin.com> My old series [1] related to the topic is no more needed and can be thrown away. Best regards, Hervé [1] https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> On Thu, 19 Sep 2024 15:51:04 +0200, Bartosz Golaszewski wrote: > If we remove a GPIO chip that is also an interrupt controller with users > not having freed some interrupts, we'll end up leaking resources as > indicated by the following warning: > > remove_proc_entry: removing non-empty directory 'irq/30', leaking at least 'gpio' > > As there's no way of notifying interrupt users about the irqchip going > away and the interrupt subsystem is not plugged into the driver model and > so not all cases can be handled by devlinks, we need to make sure to free > all interrupts before the complete the removal of the provider. > > [...] Applied, thanks! [1/1] gpio: free irqs that are still requested when the chip is being removed commit: ec8b6f55b98146c41dcf15e8189eb43291e35e89 Best regards,
On Thu, Sep 19, 2024 at 3:51 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > If we remove a GPIO chip that is also an interrupt controller with users > not having freed some interrupts, we'll end up leaking resources as > indicated by the following warning: > > remove_proc_entry: removing non-empty directory 'irq/30', leaking at least 'gpio' > > As there's no way of notifying interrupt users about the irqchip going > away and the interrupt subsystem is not plugged into the driver model and > so not all cases can be handled by devlinks, we need to make sure to free > all interrupts before the complete the removal of the provider. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Late to the show, but it's a great change and something we should have fixed ages ago (as usual...) Thanks! Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index c6afbf434366..16c16414f721 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -14,6 +14,7 @@ #include <linux/idr.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/irqdesc.h> #include <linux/kernel.h> #include <linux/list.h> #include <linux/lockdep.h> @@ -713,6 +714,45 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc, } EXPORT_SYMBOL_GPL(gpiochip_line_is_valid); +static void gpiod_free_irqs(struct gpio_desc *desc) +{ + int irq = gpiod_to_irq(desc); + struct irq_desc *irqd = irq_to_desc(irq); + void *cookie; + + for (;;) { + /* + * Make sure the action doesn't go away while we're + * dereferencing it. Retrieve and store the cookie value. + * If the irq is freed after we release the lock, that's + * alright - the underlying maple tree lookup will return NULL + * and nothing will happen in free_irq(). + */ + scoped_guard(mutex, &irqd->request_mutex) { + if (!irq_desc_has_action(irqd)) + return; + + cookie = irqd->action->dev_id; + } + + free_irq(irq, cookie); + } +} + +/* + * The chip is going away but there may be users who had requested interrupts + * on its GPIO lines who have no idea about its removal and have no way of + * being notified about it. We need to free any interrupts still in use here or + * we'll leak memory and resources (like procfs files). + */ +static void gpiochip_free_remaining_irqs(struct gpio_chip *gc) +{ + struct gpio_desc *desc; + + for_each_gpio_desc_with_flag(gc, desc, FLAG_USED_AS_IRQ) + gpiod_free_irqs(desc); +} + static void gpiodev_release(struct device *dev) { struct gpio_device *gdev = to_gpio_device(dev); @@ -1125,6 +1165,7 @@ void gpiochip_remove(struct gpio_chip *gc) /* FIXME: should the legacy sysfs handling be moved to gpio_device? */ gpiochip_sysfs_unregister(gdev); gpiochip_free_hogs(gc); + gpiochip_free_remaining_irqs(gc); scoped_guard(mutex, &gpio_devices_lock) list_del_rcu(&gdev->list);