[v2] gpio: dwapb: mask/unmask IRQ when disable/enale it

[v2] gpio: dwapb: mask/unmask IRQ when disable/enale it

[xiongxin]

In the hardware implementation of the i2c hid driver based on dwapb gpio
irq, 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 dwapb 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")
Signed-off-by: xiongxin <xiongxin@kylinos.cn>
Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
Tested-by: xiongxin <xiongxin@kylinos.cn>
---
 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

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

[Serge Semin]

On Mon, Dec 18, 2023 at 04:12:46PM +0800, xiongxin wrote:
> In the hardware implementation of the i2c hid driver based on dwapb gpio
> irq, 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 dwapb 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")
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> Tested-by: xiongxin <xiongxin@kylinos.cn>
> ---
>  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);

Thanks for submitting the patch. I wasn't sure which way was better:
define a "mask" or "hwirq" local vars with respective semantics. From
my point of view both were correct with the first version being more
optimized and the second one making enable()/disable() methods looking
alike the mask()/unmask() functions. No objections against you
implementing the second version. So

Acked-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

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

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

[Andy Shevchenko]

On Mon, Dec 18, 2023 at 04:12:46PM +0800, xiongxin wrote:
> In the hardware implementation of the i2c hid driver based on dwapb gpio
> irq, 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 dwapb 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")
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>

Your SoB should go last.

> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>

Then at all means what this SoB for? Either it's missing Co-developed-by,
or simply wrong.

> Tested-by: xiongxin <xiongxin@kylinos.cn>

This is assumed to be done by the contributor, but it's harmless to have it.

With the above being sorted out,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

...

To Serge, I give my vote to hwirq as it is aligned with the documentation.

-- 
With Best Regards,
Andy Shevchenko

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

[Serge Semin]

On Mon, Dec 18, 2023 at 01:33:05PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 18, 2023 at 04:12:46PM +0800, xiongxin wrote:
> > In the hardware implementation of the i2c hid driver based on dwapb gpio
> > irq, 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 dwapb 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")
> > Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> 
> Your SoB should go last.
> 
> > Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> Then at all means what this SoB for? Either it's missing Co-developed-by,
> or simply wrong.
> 
> > Tested-by: xiongxin <xiongxin@kylinos.cn>
> 
> This is assumed to be done by the contributor, but it's harmless to have it.
> 
> With the above being sorted out,
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> 
> ...
> 

> To Serge, I give my vote to hwirq as it is aligned with the documentation.

Right. Thanks for noting. It's now even more justified to use 'hwirq'
then.

-Serge(y)

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>