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

[PATCH v3] 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.

v3:
	* Modify the submitter's information
v2:
	* Resubmit the patch to fix this exception from the dwapb gpio
	  driver side.
v1:
	* Resolve the exception from the IRQ core layer. (key point not
	  found correctly)

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>
---
 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: [PATCH v3] gpio: dwapb: mask/unmask IRQ when disable/enale it

[Serge Semin]

On Tue, Dec 19, 2023 at 09:37:51AM +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.
> 

> v3:
> 	* Modify the submitter's information
> v2:
> 	* Resubmit the patch to fix this exception from the dwapb gpio
> 	  driver side.
> v1:
> 	* Resolve the exception from the IRQ core layer. (key point not
> 	  found correctly)

This should have been placed below the '---' marker:
Documentation/process/submitting-patches.rst

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

Also note all the tags you've already got must be preserved on the
next patch revisions. One more time:

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

-Serge(y)

> ---
>  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: [PATCH v3] gpio: dwapb: mask/unmask IRQ when disable/enale it

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Tue, Dec 19, 2023 at 09:37:51AM +0800, xiongxin wrote:

...

> Also note all the tags you've already got must be preserved on the
> next patch revisions. One more time:

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

I recommend using `b4` for that.

it harvests tags from the email thread, so no need to care about
possible misses.

-- 
With Best Regards,
Andy Shevchenko

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

[Serge Semin]

On Tue, Dec 19, 2023 at 04:17:16PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Tue, Dec 19, 2023 at 09:37:51AM +0800, xiongxin wrote:
> 
> ...
> 
> > Also note all the tags you've already got must be preserved on the
> > next patch revisions. One more time:
> 
> > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> 
> I recommend using `b4` for that.
> 
> it harvests tags from the email thread, so no need to care about
> possible misses.

AFAICS it doesn't pick up the tags from the previous revisions at
least if the new patch wasn't submitted as in-reply-to the prev one.
Just tested it on v3. b4 found my new ab-tag only and no yours rb-tag.
Did you mean something other than I thought you did?

-Serge(y)

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

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

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 05:31:38PM +0300, Serge Semin wrote:
> On Tue, Dec 19, 2023 at 04:17:16PM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Tue, Dec 19, 2023 at 09:37:51AM +0800, xiongxin wrote:
> > 
> > ...
> > 
> > > Also note all the tags you've already got must be preserved on the
> > > next patch revisions. One more time:
> > 
> > > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > I recommend using `b4` for that.
> > 
> > it harvests tags from the email thread, so no need to care about
> > possible misses.
> 
> AFAICS it doesn't pick up the tags from the previous revisions at
> least if the new patch wasn't submitted as in-reply-to the prev one.

???

> Just tested it on v3. b4 found my new ab-tag only and no yours rb-tag.
> Did you mean something other than I thought you did?

Grabbing thread from lore.kernel.org/all/mdogxxro42ymeaykrgqpld2kqbppopbywcm76osskuf3df72sl@5jalt26vzcv4/t.mbox.gz
Analyzing 4 messages in the thread
Checking attestation on all messages, may take a moment...
---
  [PATCH v2] gpio: dwapb: mask/unmask IRQ when disable/enale it
    + Reviewed-by: Andy Shevchenko <andy@kernel.org>
    + Acked-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)

The flow you need to follow is that

	git checkout $BASE # gpio/for-next in this case
	b4 am ... # as above
	git am ...
	...address comments...
	git commit -a -s --amend
	git format-patch ... -1 HEAD~0
	git send-email ...

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Dec 19, 2023 at 04:44:22PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 05:31:38PM +0300, Serge Semin wrote:
> > On Tue, Dec 19, 2023 at 04:17:16PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > On Tue, Dec 19, 2023 at 09:37:51AM +0800, xiongxin wrote:

...

> > > > Also note all the tags you've already got must be preserved on the
> > > > next patch revisions. One more time:
> > > 
> > > I recommend using `b4` for that.
> > > 
> > > it harvests tags from the email thread, so no need to care about
> > > possible misses.
> > 
> > AFAICS it doesn't pick up the tags from the previous revisions at
> > least if the new patch wasn't submitted as in-reply-to the prev one.
> 
> ???

Ah, I see what you mean now. Yes, the flow I suggested has to always be
followed, gaps are not permitted.

-- 
With Best Regards,
Andy Shevchenko

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

[Serge Semin]

On Tue, Dec 19, 2023 at 04:44:21PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 05:31:38PM +0300, Serge Semin wrote:
> > On Tue, Dec 19, 2023 at 04:17:16PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > On Tue, Dec 19, 2023 at 09:37:51AM +0800, xiongxin wrote:
> > > 
> > > ...
> > > 
> > > > Also note all the tags you've already got must be preserved on the
> > > > next patch revisions. One more time:
> > > 
> > > > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > > 
> > > I recommend using `b4` for that.
> > > 
> > > it harvests tags from the email thread, so no need to care about
> > > possible misses.
> > 
> > AFAICS it doesn't pick up the tags from the previous revisions at
> > least if the new patch wasn't submitted as in-reply-to the prev one.
> 
> ???
> 
> > Just tested it on v3. b4 found my new ab-tag only and no yours rb-tag.
> > Did you mean something other than I thought you did?
> 

> Grabbing thread from lore.kernel.org/all/mdogxxro42ymeaykrgqpld2kqbppopbywcm76osskuf3df72sl@5jalt26vzcv4/t.mbox.gz

It's _v2_. I was talking about _v3_:

[user@pc] $ b4 am 20231219013751.20386-1-xiongxin@kylinos.cn
Grabbing thread from lore.kernel.org/all/20231219013751.20386-1-xiongxin%40kylinos.cn/t.mbox.gz
Analyzing 5 messages in the thread
Checking attestation on all messages, may take a moment...
---
  [PATCH v3] gpio: dwapb: mask/unmask IRQ when disable/enale it
    + Acked-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
---
Total patches: 1
---
 Link: https://lore.kernel.org/r/20231219013751.20386-1-xiongxin@kylinos.cn
 Base: applies clean to current tree
       git checkout -b v3_20231219_xiongxin_kylinos_cn HEAD
       git am ./v3_20231219_xiongxin_gpio_dwapb_mask_unmask_irq_when_disable_enale_it.mbx

In anyway, Xiong already submitted v4 with all the tags added:
https://lore.kernel.org/linux-gpio/20231219101620.4617-1-xiongxin@kylinos.cn/
which you've already noticed.

-Serge(y)

> Analyzing 4 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>   [PATCH v2] gpio: dwapb: mask/unmask IRQ when disable/enale it
>     + Reviewed-by: Andy Shevchenko <andy@kernel.org>
>     + Acked-by: Serge Semin <fancer.lancer@gmail.com> (✓ DKIM/gmail.com)
> 
> The flow you need to follow is that
> 
> 	git checkout $BASE # gpio/for-next in this case
> 	b4 am ... # as above
> 	git am ...
> 	...address comments...
> 	git commit -a -s --amend
> 	git format-patch ... -1 HEAD~0
> 	git send-email ...
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

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

[Konstantin Ryabitsev]

On Tue, Dec 19, 2023 at 05:31:38PM +0300, Serge Semin wrote:
> > > Also note all the tags you've already got must be preserved on the
> > > next patch revisions. One more time:
> > 
> > > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > I recommend using `b4` for that.
> > 
> > it harvests tags from the email thread, so no need to care about
> > possible misses.
> 
> AFAICS it doesn't pick up the tags from the previous revisions at
> least if the new patch wasn't submitted as in-reply-to the prev one.

It's a known limitation at this time, but it will be improved in the near
future and we'll be able to grab trailers across revisions as long as the
patch-id remains the same.

-K

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

[Serge Semin]

Hi Konstantin

On Tue, Dec 19, 2023 at 01:56:47PM -0500, Konstantin Ryabitsev wrote:
> On Tue, Dec 19, 2023 at 05:31:38PM +0300, Serge Semin wrote:
> > > > Also note all the tags you've already got must be preserved on the
> > > > next patch revisions. One more time:
> > > 
> > > > Acked-by: Serge Semin <fancer.lancer@gmail.com>
> > > 
> > > I recommend using `b4` for that.
> > > 
> > > it harvests tags from the email thread, so no need to care about
> > > possible misses.
> > 
> > AFAICS it doesn't pick up the tags from the previous revisions at
> > least if the new patch wasn't submitted as in-reply-to the prev one.
> 

> It's a known limitation at this time, but it will be improved in the near
> future and we'll be able to grab trailers across revisions as long as the
> patch-id remains the same.

Ok. Thanks for the note.

I am sure you are well aware of that, but in some cases the tags are
intentionally omitted in the new patch revisions for instance due to
significant patch body change. How are you going to handle that? Just
make the tags picking up optional? Perhaps making the tags handling
interactive with printing a text/context around the tag?

-Serge(y)

> 
> -K

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

[Konstantin Ryabitsev]

On Wed, Dec 20, 2023 at 12:58:16AM +0300, Serge Semin wrote:
> > It's a known limitation at this time, but it will be improved in the near
> > future and we'll be able to grab trailers across revisions as long as the
> > patch-id remains the same.
> 
> Ok. Thanks for the note.
> 
> I am sure you are well aware of that, but in some cases the tags are
> intentionally omitted in the new patch revisions for instance due to
> significant patch body change. How are you going to handle that?

The patch-id (generated with `git patch-id --stable`), would change if the
patch body is changed (except things like whitespace). This is exactly the
behaviour that we need -- if a tag was sent to a different version of the
patch, then we no longer want to retrieve it.

> Just make the tags picking up optional? Perhaps making the tags handling
> interactive with printing a text/context around the tag?

We may indeed be able to do --interactive for some of the commands in the
future, but the goal is to automate things away.

-K

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

[Serge Semin]

On Tue, Dec 19, 2023 at 05:10:02PM -0500, Konstantin Ryabitsev wrote:
> On Wed, Dec 20, 2023 at 12:58:16AM +0300, Serge Semin wrote:
> > > It's a known limitation at this time, but it will be improved in the near
> > > future and we'll be able to grab trailers across revisions as long as the
> > > patch-id remains the same.
> > 
> > Ok. Thanks for the note.
> > 
> > I am sure you are well aware of that, but in some cases the tags are
> > intentionally omitted in the new patch revisions for instance due to
> > significant patch body change. How are you going to handle that?
> 

> The patch-id (generated with `git patch-id --stable`), would change if the
> patch body is changed (except things like whitespace). This is exactly the
> behaviour that we need -- if a tag was sent to a different version of the
> patch, then we no longer want to retrieve it.
> 
> > Just make the tags picking up optional? Perhaps making the tags handling
> > interactive with printing a text/context around the tag?
> 
> We may indeed be able to do --interactive for some of the commands in the
> future, but the goal is to automate things away.

Got it. Thanks. Can't wait to see that in action.

-Serge(y)

> 
> -K