[RFT PATCH] gpio: eic-sprd: use atomic notifiers to notify all chips about irqs

[RFT PATCH] gpio: eic-sprd: use atomic notifiers to notify all chips about irqs

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Calling gpiochip_find() from interrupt handler in this driver is an
abuse of the GPIO API. It only happens to work because nobody added a
might_sleep() to it and the lock used by GPIOLIB is a spinlock.

Both will soon be changed as we're limiting both the number of
interfaces allowed to be called from atomic context as well as making
struct gpio_chip private to the GPIO code that owns it. We'll also
switch to protecting the global GPIO device list with a mutex as there
is no reason to allow changes to it from interrupt handlers.

Instead of iterating over all SPRD chips and looking up each
corresponding GPIO chip, let's make each SPRD GPIO controller register
with a notifier chain. The chain will be called at interrupt so that
every chip that already probed will be notified. The rest of the
interrupt handling remains the same. This should result in faster code as
we're avoiding iterating over the list of all GPIO devices.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
I only build-tested it. Please take it for a ride, I hope this works.

 drivers/gpio/gpio-eic-sprd.c | 44 ++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index 5320cf1de89c..21a1afe358d6 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
@@ -91,12 +92,20 @@ enum sprd_eic_type {
 
 struct sprd_eic {
 	struct gpio_chip chip;
+	struct notifier_block irq_nb;
 	void __iomem *base[SPRD_EIC_MAX_BANK];
 	enum sprd_eic_type type;
 	spinlock_t lock;
 	int irq;
 };
 
+static ATOMIC_NOTIFIER_HEAD(sprd_eic_irq_notifier);
+
+static struct sprd_eic *to_sprd_eic(struct notifier_block *nb)
+{
+	return container_of(nb, struct sprd_eic, irq_nb);
+}
+
 struct sprd_eic_variant_data {
 	enum sprd_eic_type type;
 	u32 num_eics;
@@ -494,13 +503,6 @@ static void sprd_eic_toggle_trigger(struct gpio_chip *chip, unsigned int irq,
 	sprd_eic_irq_unmask(data);
 }
 
-static int sprd_eic_match_chip_by_type(struct gpio_chip *chip, void *data)
-{
-	enum sprd_eic_type type = *(enum sprd_eic_type *)data;
-
-	return !strcmp(chip->label, sprd_eic_label_name[type]);
-}
-
 static void sprd_eic_handle_one_type(struct gpio_chip *chip)
 {
 	struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
@@ -546,27 +548,29 @@ static void sprd_eic_handle_one_type(struct gpio_chip *chip)
 static void sprd_eic_irq_handler(struct irq_desc *desc)
 {
 	struct irq_chip *ic = irq_desc_get_chip(desc);
-	struct gpio_chip *chip;
-	enum sprd_eic_type type;
 
 	chained_irq_enter(ic, desc);
 
 	/*
 	 * Since the digital-chip EIC 4 sub-modules (debounce, latch, async
-	 * and sync) share one same interrupt line, we should iterate each
-	 * EIC module to check if there are EIC interrupts were triggered.
+	 * and sync) share one same interrupt line, we should notify all of
+	 * them to let them check if there are EIC interrupts were triggered.
 	 */
-	for (type = SPRD_EIC_DEBOUNCE; type < SPRD_EIC_MAX; type++) {
-		chip = gpiochip_find(&type, sprd_eic_match_chip_by_type);
-		if (!chip)
-			continue;
-
-		sprd_eic_handle_one_type(chip);
-	}
+	atomic_notifier_call_chain(&sprd_eic_irq_notifier, 0, NULL);
 
 	chained_irq_exit(ic, desc);
 }
 
+static int sprd_eic_irq_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
+{
+	struct sprd_eic *sprd_eic = to_sprd_eic(nb);
+
+	sprd_eic_handle_one_type(&sprd_eic->chip);
+
+	return NOTIFY_OK;
+}
+
 static const struct irq_chip sprd_eic_irq = {
 	.name		= "sprd-eic",
 	.irq_ack	= sprd_eic_irq_ack,
@@ -653,7 +657,9 @@ static int sprd_eic_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	return 0;
+	sprd_eic->irq_nb.notifier_call = sprd_eic_irq_notify;
+	return atomic_notifier_chain_register(&sprd_eic_irq_notifier,
+					      &sprd_eic->irq_nb);
 }
 
 static const struct of_device_id sprd_eic_of_match[] = {
-- 
2.39.2

Re: [RFT PATCH] gpio: eic-sprd: use atomic notifiers to notify all chips about irqs

[Chunyan Zhang]

Hi Bartosz,

On Mon, 4 Sept 2023 at 20:33, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Calling gpiochip_find() from interrupt handler in this driver is an
> abuse of the GPIO API. It only happens to work because nobody added a
> might_sleep() to it and the lock used by GPIOLIB is a spinlock.

Thanks for the fix.

I back-ported this patch to an internal tree, and my colleague Wenhua
helped make a basic test, there's no problem found. So please feel
free to add:

Reviewed-by: Chunyan Zhang <zhang.lyra@gmail.com>
Tested-by: Wenhua Lin <wenhua.lin@unisoc.com>

Cheers,
Chunyan

>
> Both will soon be changed as we're limiting both the number of
> interfaces allowed to be called from atomic context as well as making
> struct gpio_chip private to the GPIO code that owns it. We'll also
> switch to protecting the global GPIO device list with a mutex as there
> is no reason to allow changes to it from interrupt handlers.
>
> Instead of iterating over all SPRD chips and looking up each
> corresponding GPIO chip, let's make each SPRD GPIO controller register
> with a notifier chain. The chain will be called at interrupt so that
> every chip that already probed will be notified. The rest of the
> interrupt handling remains the same. This should result in faster code as
> we're avoiding iterating over the list of all GPIO devices.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> I only build-tested it. Please take it for a ride, I hope this works.
>
>  drivers/gpio/gpio-eic-sprd.c | 44 ++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index 5320cf1de89c..21a1afe358d6 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
> @@ -9,6 +9,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> @@ -91,12 +92,20 @@ enum sprd_eic_type {
>
>  struct sprd_eic {
>         struct gpio_chip chip;
> +       struct notifier_block irq_nb;
>         void __iomem *base[SPRD_EIC_MAX_BANK];
>         enum sprd_eic_type type;
>         spinlock_t lock;
>         int irq;
>  };
>
> +static ATOMIC_NOTIFIER_HEAD(sprd_eic_irq_notifier);
> +
> +static struct sprd_eic *to_sprd_eic(struct notifier_block *nb)
> +{
> +       return container_of(nb, struct sprd_eic, irq_nb);
> +}
> +
>  struct sprd_eic_variant_data {
>         enum sprd_eic_type type;
>         u32 num_eics;
> @@ -494,13 +503,6 @@ static void sprd_eic_toggle_trigger(struct gpio_chip *chip, unsigned int irq,
>         sprd_eic_irq_unmask(data);
>  }
>
> -static int sprd_eic_match_chip_by_type(struct gpio_chip *chip, void *data)
> -{
> -       enum sprd_eic_type type = *(enum sprd_eic_type *)data;
> -
> -       return !strcmp(chip->label, sprd_eic_label_name[type]);
> -}
> -
>  static void sprd_eic_handle_one_type(struct gpio_chip *chip)
>  {
>         struct sprd_eic *sprd_eic = gpiochip_get_data(chip);
> @@ -546,27 +548,29 @@ static void sprd_eic_handle_one_type(struct gpio_chip *chip)
>  static void sprd_eic_irq_handler(struct irq_desc *desc)
>  {
>         struct irq_chip *ic = irq_desc_get_chip(desc);
> -       struct gpio_chip *chip;
> -       enum sprd_eic_type type;
>
>         chained_irq_enter(ic, desc);
>
>         /*
>          * Since the digital-chip EIC 4 sub-modules (debounce, latch, async
> -        * and sync) share one same interrupt line, we should iterate each
> -        * EIC module to check if there are EIC interrupts were triggered.
> +        * and sync) share one same interrupt line, we should notify all of
> +        * them to let them check if there are EIC interrupts were triggered.
>          */
> -       for (type = SPRD_EIC_DEBOUNCE; type < SPRD_EIC_MAX; type++) {
> -               chip = gpiochip_find(&type, sprd_eic_match_chip_by_type);
> -               if (!chip)
> -                       continue;
> -
> -               sprd_eic_handle_one_type(chip);
> -       }
> +       atomic_notifier_call_chain(&sprd_eic_irq_notifier, 0, NULL);
>
>         chained_irq_exit(ic, desc);
>  }
>
> +static int sprd_eic_irq_notify(struct notifier_block *nb, unsigned long action,
> +                              void *data)
> +{
> +       struct sprd_eic *sprd_eic = to_sprd_eic(nb);
> +
> +       sprd_eic_handle_one_type(&sprd_eic->chip);
> +
> +       return NOTIFY_OK;
> +}
> +
>  static const struct irq_chip sprd_eic_irq = {
>         .name           = "sprd-eic",
>         .irq_ack        = sprd_eic_irq_ack,
> @@ -653,7 +657,9 @@ static int sprd_eic_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> -       return 0;
> +       sprd_eic->irq_nb.notifier_call = sprd_eic_irq_notify;
> +       return atomic_notifier_chain_register(&sprd_eic_irq_notifier,
> +                                             &sprd_eic->irq_nb);
>  }
>
>  static const struct of_device_id sprd_eic_of_match[] = {
> --
> 2.39.2
>