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: [email protected]
Co-developed-by: Riwen Lu <[email protected]>
Signed-off-by: Riwen Lu <[email protected]>
Signed-off-by: xiongxin <[email protected]>
---
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
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: [email protected]
> Co-developed-by: Riwen Lu <[email protected]>
> Signed-off-by: Riwen Lu <[email protected]>
> Signed-off-by: xiongxin <[email protected]>
Also note all the tags you've already got must be preserved on the
next patch revisions. One more time:
Acked-by: Serge Semin <[email protected]>
-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
>
On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <[email protected]> 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 <[email protected]>
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
On Tue, Dec 19, 2023 at 04:17:16PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 19, 2023 at 11:14 AM Serge Semin <[email protected]> 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 <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]>
> >
> > 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 <[email protected]>
+ Acked-by: Serge Semin <[email protected]> (✓ 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
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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]>
> > >
> > > 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 [email protected]
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 <[email protected]> (✓ DKIM/gmail.com)
---
Total patches: 1
---
Link: https://lore.kernel.org/r/[email protected]
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/[email protected]/
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 <[email protected]>
> + Acked-by: Serge Semin <[email protected]> (✓ 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
>
>
>
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > 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
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
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