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

[Chunyan Zhang <zhang.lyra@xxxxxxxxx>]

Hi Bartosz,

On Mon, 4 Sept 2023 at 20:33, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
Tested-by: Wenhua Lin <wenhua.lin@xxxxxxxxxx>

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@xxxxxxxxxx>
> ---
> 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
>