Re: [PATCH v5] gpio: dwapb: mask/unmask IRQ when disable/enale it

[Bartosz Golaszewski <brgl@bgdev.pl>]

On Wed, Dec 20, 2023 at 3:29 AM xiongxin <xiongxin@kylinos.cn> wrote:
>
> In the hardware implementation of the I2C HID driver based on DesignWare
> GPIO IRQ chip, when the user continues to use the I2C HID device in the
> suspend process, the I2C HID interrupt will be masked after the resume
> process is finished.
>
> This is because the disable_irq()/enable_irq() of the DesignWare GPIO
> driver does not synchronize the IRQ mask register state. In normal use
> of the I2C HID procedure, the GPIO IRQ irq_mask()/irq_unmask() functions
> are called in pairs. In case of an exception, i2c_hid_core_suspend()
> calls disable_irq() to disable the GPIO IRQ. With low probability, this
> causes irq_unmask() to not be called, which causes the GPIO IRQ to be
> masked and not unmasked in enable_irq(), raising an exception.
>
> Add synchronization to the masked register state in the
> dwapb_irq_enable()/dwapb_irq_disable() function. mask the GPIO IRQ
> before disabling it. After enabling the GPIO IRQ, unmask the IRQ.
>
> Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block")
> Cc: stable@kernel.org
> Co-developed-by: Riwen Lu <luriwen@kylinos.cn>
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> Acked-by: Serge Semin <fancer.lancer@gmail.com>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> ---
> v5:
>         * fix typo in patch description
> v4:
>         * Add patch tag information
> v3:
>         * Modify the submitter's information
> v2:
>         * Resubmit the patch to fix this exception from the DesignWare
>           GPIO driver side
> v1:
>         * Resolve the exception from the IRQ core layer. (key point not
>           found correctly)
> ---
>  drivers/gpio/gpio-dwapb.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 4a4f61bf6c58..8c59332429c2 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -282,13 +282,15 @@ static void dwapb_irq_enable(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
>         unsigned long flags;
>         u32 val;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       val = dwapb_read(gpio, GPIO_INTEN);
> -       val |= BIT(irqd_to_hwirq(d));
> +       val = dwapb_read(gpio, GPIO_INTEN) | BIT(hwirq);
>         dwapb_write(gpio, GPIO_INTEN, val);
> +       val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(hwirq);
> +       dwapb_write(gpio, GPIO_INTMASK, val);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>  }
>
> @@ -296,12 +298,14 @@ static void dwapb_irq_disable(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
>         unsigned long flags;
>         u32 val;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       val = dwapb_read(gpio, GPIO_INTEN);
> -       val &= ~BIT(irqd_to_hwirq(d));
> +       val = dwapb_read(gpio, GPIO_INTMASK) | BIT(hwirq);
> +       dwapb_write(gpio, GPIO_INTMASK, val);
> +       val = dwapb_read(gpio, GPIO_INTEN) & ~BIT(hwirq);
>         dwapb_write(gpio, GPIO_INTEN, val);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>  }
> --
> 2.34.1
>

Queued for fixes, thanks!

Bartosz