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>
---
Herve: if I say I'll do something, then I'll do it, no need to remind me every
six months. :) Anyway, this is a proposition of fixing the resource leak you
reported with gpiomon in a more generic way so we also address the same issue
for in-kernel users.

 drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c6afbf434366..f336fd608d58 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,31 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 
+/*
+ * 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;
+	struct irq_desc *irqd;
+	void *dev_id;
+	int irq;
+
+	for_each_gpio_desc_with_flag(gc, desc, FLAG_USED_AS_IRQ) {
+		irq = gpiod_to_irq(desc);
+		irqd = irq_to_desc(irq);
+
+		while (irq_desc_has_action(irqd)) {
+			scoped_guard(raw_spinlock_irqsave, &irqd->lock)
+				dev_id = irqd->action->dev_id;
+			free_irq(irq, dev_id);
+		}
+	}
+}
+
 static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
@@ -1125,6 +1151,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);
-- 
2.30.2

On Thu, Sep 19, 2024 at 1:13 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>
> ---
> Herve: if I say I'll do something, then I'll do it, no need to remind me every
> six months. :) Anyway, this is a proposition of fixing the resource leak you
> reported with gpiomon in a more generic way so we also address the same issue
> for in-kernel users.
>

I noticed a problem here so I sent an improved version[1].

Bart

[1] https://lore.kernel.org/linux-gpio/20240919135104.3583-1-brgl@bgdev.pl/