[PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Luo Jiaxing]

The mask and unmask registers are not configured in dwapb_irq_enable() and
dwapb_irq_disable(). In the following situations, the IRQ will be masked by
default after the IRQ is enabled:

mask IRQ -> disable IRQ -> enable IRQ

In this case, the IRQ status of GPIO controller is inconsistent with it's
irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 drivers/gpio/gpio-dwapb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2a9046c..ca654eb 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -270,6 +270,8 @@ static void dwapb_irq_enable(struct irq_data *d)
 	u32 val;
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
+	val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(irqd_to_hwirq(d));
+	dwapb_write(gpio, GPIO_INTMASK, val);
 	val = dwapb_read(gpio, GPIO_INTEN);
 	val |= BIT(irqd_to_hwirq(d));
 	dwapb_write(gpio, GPIO_INTEN, val);
@@ -284,6 +286,8 @@ static void dwapb_irq_disable(struct irq_data *d)
 	u32 val;
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
+	val = dwapb_read(gpio, GPIO_INTMASK) | BIT(irqd_to_hwirq(d));
+	dwapb_write(gpio, GPIO_INTMASK, val);
 	val = dwapb_read(gpio, GPIO_INTEN);
 	val &= ~BIT(irqd_to_hwirq(d));
 	dwapb_write(gpio, GPIO_INTEN, val);
-- 
2.7.4

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Andy Shevchenko]

On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote:
> The mask and unmask registers are not configured in dwapb_irq_enable() and
> dwapb_irq_disable(). In the following situations, the IRQ will be masked by
> default after the IRQ is enabled:
> 
> mask IRQ -> disable IRQ -> enable IRQ
> 
> In this case, the IRQ status of GPIO controller is inconsistent with it's
> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.

Sounds a bit like a papering over the issue which is slightly different.
Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[luojiaxing]

On 2020/11/30 19:22, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote:
>> The mask and unmask registers are not configured in dwapb_irq_enable() and
>> dwapb_irq_disable(). In the following situations, the IRQ will be masked by
>> default after the IRQ is enabled:
>>
>> mask IRQ -> disable IRQ -> enable IRQ
>>
>> In this case, the IRQ status of GPIO controller is inconsistent with it's
>> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
>> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.
> Sounds a bit like a papering over the issue which is slightly different.
> Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?


Sure, The basic software invoking process is as follows:

Release IRQ:
free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable()

Disable IRQ:
disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable 
-> __irq_disable()

As shown before, both will call __irq_disable(). The code of it is as 
follows:

if (irqd_irq_disabled(&desc->irq_data)) {
     if (mask)
         mask_irq(desc);

} else {
         irq_state_set_disabled(desc);
             if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
                 irq_state_set_masked(desc);
             } else if (mask) {
                 mask_irq(desc);
     }
}

Because gpio-dwapb.c provides the hook function of irq_disable, 
__irq_disable() will directly calls chip->irq_disable() instead of 
mask_irq().

For irq_enable(), it's similar and the code is as follows:

if (!irqd_irq_disabled(&desc->irq_data)) {
     unmask_irq(desc);
} else {
     irq_state_clr_disabled(desc);
     if (desc->irq_data.chip->irq_enable) {
desc->irq_data.chip->irq_enable(&desc->irq_data);
         irq_state_clr_masked(desc);
     } else {
         unmask_irq(desc);
     }
}

Similarly, because gpio-dwapb.c provides the hook function of 
irq_enable, irq_enable() will directly calls chip->irq_enable() but does 
not call unmask_irq().


Therefore, the current handle is as follows:

API of IRQ:        |   mask_irq()             | disable_irq()            
|    enable_irq()

gpio-dwapb.c:  |   chip->irq_mask()   | chip->irq_diable()   |    
chip->irq_enable()

I do not know why irq_enable() only calls chip->irq_enable(). However, 
the code shows that irq_enable() clears the disable and masked flags in 
the irq_data state.

Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear 
the disable and masked flags in the hardware register.

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Serge Semin]

On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote:
> 
> On 2020/11/30 19:22, Andy Shevchenko wrote:
> > On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote:
> > > The mask and unmask registers are not configured in dwapb_irq_enable() and
> > > dwapb_irq_disable(). In the following situations, the IRQ will be masked by
> > > default after the IRQ is enabled:
> > > 
> > > mask IRQ -> disable IRQ -> enable IRQ
> > > 
> > > In this case, the IRQ status of GPIO controller is inconsistent with it's
> > > irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
> > > IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.
> > Sounds a bit like a papering over the issue which is slightly different.
> > Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?
> 
> 
> Sure, The basic software invoking process is as follows:
> 
> Release IRQ:
> free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable()
> 
> Disable IRQ:
> disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable ->
> __irq_disable()
> 
> As shown before, both will call __irq_disable(). The code of it is as
> follows:
> 
> if (irqd_irq_disabled(&desc->irq_data)) {
>     if (mask)
>         mask_irq(desc);
> 
> } else {
>         irq_state_set_disabled(desc);
>             if (desc->irq_data.chip->irq_disable) {
> desc->irq_data.chip->irq_disable(&desc->irq_data);
>                 irq_state_set_masked(desc);
>             } else if (mask) {
>                 mask_irq(desc);
>     }
> }
> 
> Because gpio-dwapb.c provides the hook function of irq_disable,
> __irq_disable() will directly calls chip->irq_disable() instead of
> mask_irq().
> 
> For irq_enable(), it's similar and the code is as follows:
> 
> if (!irqd_irq_disabled(&desc->irq_data)) {
>     unmask_irq(desc);
> } else {
>     irq_state_clr_disabled(desc);
>     if (desc->irq_data.chip->irq_enable) {
> desc->irq_data.chip->irq_enable(&desc->irq_data);
>         irq_state_clr_masked(desc);
>     } else {
>         unmask_irq(desc);
>     }
> }
> 
> Similarly, because gpio-dwapb.c provides the hook function of irq_enable,
> irq_enable() will directly calls chip->irq_enable() but does not call
> unmask_irq().
> 
> 
> Therefore, the current handle is as follows:
> 
> API of IRQ:        |   mask_irq()             | disable_irq()           
> |    enable_irq()
> 
> gpio-dwapb.c:  |   chip->irq_mask()   | chip->irq_diable()   |   
> chip->irq_enable()
> 
> I do not know why irq_enable() only calls chip->irq_enable(). However, the
> code shows that irq_enable() clears the disable and masked flags in the
> irq_data state.
> 
> Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the
> disable and masked flags in the hardware register.
> 

Hmm, that sounds like a problem, but the explanation is a bit unclear
to me. AFAICS you are saying that the only callbacks which are
called during the IRQ request/release are the irq_enable(), right? If
so then the only reason why we haven't got a problem reported due to
that so far is that the IRQs actually unmasked by default.

In anyway I'd suggest to join someone from the kernel IRQs-related
subsystem to this discussion to ask their opinion whether the IRQs
setup procedure is supposed to work like you say and the irq_enable
shall actually also unmask IRQs.

Thomas, Jason, Mark, could you give us your comment about the issue?

-Sergey

> 
> 
> 

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Linus Walleij]

On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> Hmm, that sounds like a problem, but the explanation is a bit unclear
> to me. AFAICS you are saying that the only callbacks which are
> called during the IRQ request/release are the irq_enable(), right? If
> so then the only reason why we haven't got a problem reported due to
> that so far is that the IRQs actually unmasked by default.

What we usually do in cases like that (and I have discussed this
with tglx in the past I think) is to simply mask off all IRQs in probe().
Then they will be unmasked when requested by drivers.

See e.g. gpio-pl061 that has this line in probe():
writeb(0, pl061->base + GPIOIE); /* disable irqs */

Yours,
Linus Walleij

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Marc Zyngier]

On 2020-12-06 15:02, Linus Walleij wrote:
> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> 
> wrote:
> 
>> Hmm, that sounds like a problem, but the explanation is a bit unclear
>> to me. AFAICS you are saying that the only callbacks which are
>> called during the IRQ request/release are the irq_enable(), right? If
>> so then the only reason why we haven't got a problem reported due to
>> that so far is that the IRQs actually unmasked by default.
> 
> What we usually do in cases like that (and I have discussed this
> with tglx in the past I think) is to simply mask off all IRQs in 
> probe().
> Then they will be unmasked when requested by drivers.
> 
> See e.g. gpio-pl061 that has this line in probe():
> writeb(0, pl061->base + GPIOIE); /* disable irqs */

This should definitely be the default behaviour. The code code
expects all interrupt sources to be masked until actively enabled,
usually with the IRQ being requested.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[luojiaxing]

On 2020/12/7 2:50, Marc Zyngier wrote:
> On 2020-12-06 15:02, Linus Walleij wrote:
>> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> 
>> wrote:
>>
>>> Hmm, that sounds like a problem, but the explanation is a bit unclear
>>> to me. AFAICS you are saying that the only callbacks which are
>>> called during the IRQ request/release are the irq_enable(), right? If
>>> so then the only reason why we haven't got a problem reported due to
>>> that so far is that the IRQs actually unmasked by default.
>>
>> What we usually do in cases like that (and I have discussed this
>> with tglx in the past I think) is to simply mask off all IRQs in 
>> probe().
>> Then they will be unmasked when requested by drivers.
>>
>> See e.g. gpio-pl061 that has this line in probe():
>> writeb(0, pl061->base + GPIOIE); /* disable irqs */
>
> This should definitely be the default behaviour. The code code
> expects all interrupt sources to be masked until actively enabled,
> usually with the IRQ being requested.


I think this patch is used for that purpose. I do two things in 
irq_enable(): unmask irq and then enable IRQ;

and for irq_disable(), it's similar; mask IRQ then disable it.


Thanks

Jiaxing


>
> Thanks,
>
>         M.

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Bartosz Golaszewski]

On Mon, Dec 7, 2020 at 2:10 PM luojiaxing <luojiaxing@huawei.com> wrote:
>
>
> On 2020/12/7 2:50, Marc Zyngier wrote:
> > On 2020-12-06 15:02, Linus Walleij wrote:
> >> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com>
> >> wrote:
> >>
> >>> Hmm, that sounds like a problem, but the explanation is a bit unclear
> >>> to me. AFAICS you are saying that the only callbacks which are
> >>> called during the IRQ request/release are the irq_enable(), right? If
> >>> so then the only reason why we haven't got a problem reported due to
> >>> that so far is that the IRQs actually unmasked by default.
> >>
> >> What we usually do in cases like that (and I have discussed this
> >> with tglx in the past I think) is to simply mask off all IRQs in
> >> probe().
> >> Then they will be unmasked when requested by drivers.
> >>
> >> See e.g. gpio-pl061 that has this line in probe():
> >> writeb(0, pl061->base + GPIOIE); /* disable irqs */
> >
> > This should definitely be the default behaviour. The code code
> > expects all interrupt sources to be masked until actively enabled,
> > usually with the IRQ being requested.
>
>
> I think this patch is used for that purpose. I do two things in
> irq_enable(): unmask irq and then enable IRQ;
>
> and for irq_disable(), it's similar; mask IRQ then disable it.

Hi!

Could you please resend this patch rebased on top of v5.11-rc2 and
with the detailed explanation you responded with to Andy as part of
the commit message?

Thanks!
Bart

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Serge Semin]

Hello folks,
My comments are below.

On Wed, Jan 06, 2021 at 01:44:28PM +0200, Andy Shevchenko wrote:
> On Wednesday, January 6, 2021, Bartosz Golaszewski <
> bgolaszewski@baylibre.com> wrote:
> 
> > On Mon, Dec 7, 2020 at 2:10 PM luojiaxing <luojiaxing@huawei.com> wrote:
> > >
> > >
> > > On 2020/12/7 2:50, Marc Zyngier wrote:
> > > > On 2020-12-06 15:02, Linus Walleij wrote:
> > > >> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com>
> > > >> wrote:
> > > >>

> > > >>> Hmm, that sounds like a problem, but the explanation is a bit unclear
> > > >>> to me. AFAICS you are saying that the only callbacks which are
> > > >>> called during the IRQ request/release are the irq_enable(), right? If
> > > >>> so then the only reason why we haven't got a problem reported due to
> > > >>> that so far is that the IRQs actually unmasked by default.

As I said the problem explanation stated in the log is a bit unclear
to me.  It needs elaboration at the very least in v2 with more details
why masking and masking needs to be performed in the IRQ
disable/enable callback. But AFAICS from the code invocation stack and
the Luo further messages the problem with having both mask/unmask and
disable/enable IRQ-chip functionality may indeed exist.

Judging by the irq_enable() and irq_disable() functions code both of them
use only one pair of the IRQ switchers with giving more favor to the
IRQ disable/enable methods. So if the later are specified for an IRQ
chip, then the IRQ mask/unmask functions just won't be called. (Though
I might be wrong in this matter. Marc, please correct me if I am.) In our
case if for some reason any of the GPIO lane IRQ has been masked for
instance by a bootloader or the default state has been changed on
IP-core level, then the corresponding IRQ just won't be activated by
the kernel IRQs subsystem. In case of my DW APB core and most likely in
the most of the cases all the IRQs are unmasked, but disabled by default.
That's why we haven't got any report about the problem until now.

> > > >>

> > > >> What we usually do in cases like that (and I have discussed this
> > > >> with tglx in the past I think) is to simply mask off all IRQs in
> > > >> probe().
> > > >> Then they will be unmasked when requested by drivers.
> > > >>
> > > >> See e.g. gpio-pl061 that has this line in probe():
> > > >> writeb(0, pl061->base + GPIOIE); /* disable irqs */
> > > >
> > > > This should definitely be the default behaviour. The code code
> > > > expects all interrupt sources to be masked until actively enabled,
> > > > usually with the IRQ being requested.

What DW APB driver has different with respect to the others is that it
provides both IRQ mask/unmask and disable/enable functionality. So
if we get to mask all the IRQs in the probe method, then according to
the irq_enable()/irq_disable() code semantics and as Luo said the
corresponding IRQ won't be unmasked in request_irq(), but will be just
enabled. So effectively the IRQ will be left masked, which of course
we don't want to happen after the successful request_irq() method
return.

As I see it the problem either needs to be worked around in the local
driver (for instance in a way Luo suggests) or fixed in the IRQ subsystem
level.

> > >
> > >

> > > I think this patch is used for that purpose. I do two things in
> > > irq_enable(): unmask irq and then enable IRQ;
> > >
> > > and for irq_disable(), it's similar; mask IRQ then disable it.

Yeah, this patch provides a work around for the problem. So the
chip->irq_enable() callback enables and unmasks the corresponding
GPIO IRQ, while chip->irq_disable() masks and disables it. In this
case the irq_mask()/irq_unmask() methods just get to be redundant
since won't be used by the core anyway.

But before accepting the solution in my opinion it would be better to
at least discuss whether it is possible to fix the
irq_enable()/irq_disable() methods so the similar problem wouldn't
happen for the hardware like DW APB.

Marc, Linus, Andy, Bartosz, Luo what do you think? Wouldn't that be
better to fix the IRQ subsystem code instead seeing it doesn't cover
all the possible hardware like with both types of IRQ enable/mask
callback provided?

> >
> > Hi!
> >
> > Could you please resend this patch rebased on top of v5.11-rc2 and
> > with the detailed explanation you responded with to Andy as part of
> > the commit message?
> >
> >

> I guess it’s more than that. What’s the driver maintainer position here?

Andy, thanks for sending a notification about this patch. Please see my
comments above.

-Sergey

> 
> 
> 
> > Thanks!
> > Bart
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[luojiaxing]

On 2020/12/6 23:02, Linus Walleij wrote:
> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
>> Hmm, that sounds like a problem, but the explanation is a bit unclear
>> to me. AFAICS you are saying that the only callbacks which are
>> called during the IRQ request/release are the irq_enable(), right? If
>> so then the only reason why we haven't got a problem reported due to
>> that so far is that the IRQs actually unmasked by default.
> What we usually do in cases like that (and I have discussed this
> with tglx in the past I think) is to simply mask off all IRQs in probe().
> Then they will be unmasked when requested by drivers.


Yes, I agree. but in this case I mean that they will not unmasked when 
drivers request IRQ.

Drivers request IRQ will only call irq_enable(), so if we mask all IRQ 
in gpio-dwab.'s probe(),

no one will unmask the IRQ for drivers.


Thanks

Jiaxing


>
> See e.g. gpio-pl061 that has this line in probe():
> writeb(0, pl061->base + GPIOIE); /* disable irqs */
>
> Yours,
> Linus Walleij
>
> .
>

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[luojiaxing]

On 2020/12/6 6:15, Serge Semin wrote:
> On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote:
>> On 2020/11/30 19:22, Andy Shevchenko wrote:
>>> On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote:
>>>> The mask and unmask registers are not configured in dwapb_irq_enable() and
>>>> dwapb_irq_disable(). In the following situations, the IRQ will be masked by
>>>> default after the IRQ is enabled:
>>>>
>>>> mask IRQ -> disable IRQ -> enable IRQ
>>>>
>>>> In this case, the IRQ status of GPIO controller is inconsistent with it's
>>>> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
>>>> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.
>>> Sounds a bit like a papering over the issue which is slightly different.
>>> Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?
>>
>> Sure, The basic software invoking process is as follows:
>>
>> Release IRQ:
>> free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable()
>>
>> Disable IRQ:
>> disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable ->
>> __irq_disable()
>>
>> As shown before, both will call __irq_disable(). The code of it is as
>> follows:
>>
>> if (irqd_irq_disabled(&desc->irq_data)) {
>>      if (mask)
>>          mask_irq(desc);
>>
>> } else {
>>          irq_state_set_disabled(desc);
>>              if (desc->irq_data.chip->irq_disable) {
>> desc->irq_data.chip->irq_disable(&desc->irq_data);
>>                  irq_state_set_masked(desc);
>>              } else if (mask) {
>>                  mask_irq(desc);
>>      }
>> }
>>
>> Because gpio-dwapb.c provides the hook function of irq_disable,
>> __irq_disable() will directly calls chip->irq_disable() instead of
>> mask_irq().
>>
>> For irq_enable(), it's similar and the code is as follows:
>>
>> if (!irqd_irq_disabled(&desc->irq_data)) {
>>      unmask_irq(desc);
>> } else {
>>      irq_state_clr_disabled(desc);
>>      if (desc->irq_data.chip->irq_enable) {
>> desc->irq_data.chip->irq_enable(&desc->irq_data);
>>          irq_state_clr_masked(desc);
>>      } else {
>>          unmask_irq(desc);
>>      }
>> }
>>
>> Similarly, because gpio-dwapb.c provides the hook function of irq_enable,
>> irq_enable() will directly calls chip->irq_enable() but does not call
>> unmask_irq().
>>
>>
>> Therefore, the current handle is as follows:
>>
>> API of IRQ:        |   mask_irq()             | disable_irq()
>> |    enable_irq()
>>
>> gpio-dwapb.c:  |   chip->irq_mask()   | chip->irq_diable()   |
>> chip->irq_enable()
>>
>> I do not know why irq_enable() only calls chip->irq_enable(). However, the
>> code shows that irq_enable() clears the disable and masked flags in the
>> irq_data state.
>>
>> Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the
>> disable and masked flags in the hardware register.
>>
> Hmm, that sounds like a problem, but the explanation is a bit unclear
> to me. AFAICS you are saying that the only callbacks which are
> called during the IRQ request/release are the irq_enable(), right?


Yes, but one point needs to be clarified, for IRQ requests, it calls 
irq_enable(); for IRQ release, it calls irq_disable().

Actually I am thinking that why only irq_enable()/irq_disable() is 
called since the mask and enable flags of irq_data are both set.

Does IRQ subsystem expect irq_enable to set both mask and enable? If we 
didn't do that, the state machine of the software is different from 
hardware, at least for mask bit.


> If
> so then the only reason why we haven't got a problem reported due to
> that so far is that the IRQs actually unmasked by default.


yes, I think so, Common drivers do not mask the IRQ before releasing it. 
But that's possible.


>
> In anyway I'd suggest to join someone from the kernel IRQs-related
> subsystem to this discussion to ask their opinion whether the IRQs
> setup procedure is supposed to work like you say and the irq_enable
> shall actually also unmask IRQs.
>
> Thomas, Jason, Mark, could you give us your comment about the issue?
>
> -Sergey
>
>>
>>
> .
>

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Linus Walleij]

Sorry for top posting but I need the help of the irqchip maintainer
Marc Z to hash this out.

The mask/unmask/disable/enable semantics is something that
you need to work with every day to understand right.

Yours,
Linus Walleij

On Mon, Nov 30, 2020 at 10:36 AM Luo Jiaxing <luojiaxing@huawei.com> wrote:
>
> The mask and unmask registers are not configured in dwapb_irq_enable() and
> dwapb_irq_disable(). In the following situations, the IRQ will be masked by
> default after the IRQ is enabled:
>
> mask IRQ -> disable IRQ -> enable IRQ
>
> In this case, the IRQ status of GPIO controller is inconsistent with it's
> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and
> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask.
>
> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 2a9046c..ca654eb 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -270,6 +270,8 @@ static void dwapb_irq_enable(struct irq_data *d)
>         u32 val;
>
>         spin_lock_irqsave(&gc->bgpio_lock, flags);
> +       val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(irqd_to_hwirq(d));
> +       dwapb_write(gpio, GPIO_INTMASK, val);
>         val = dwapb_read(gpio, GPIO_INTEN);
>         val |= BIT(irqd_to_hwirq(d));
>         dwapb_write(gpio, GPIO_INTEN, val);
> @@ -284,6 +286,8 @@ static void dwapb_irq_disable(struct irq_data *d)
>         u32 val;
>
>         spin_lock_irqsave(&gc->bgpio_lock, flags);
> +       val = dwapb_read(gpio, GPIO_INTMASK) | BIT(irqd_to_hwirq(d));
> +       dwapb_write(gpio, GPIO_INTMASK, val);
>         val = dwapb_read(gpio, GPIO_INTEN);
>         val &= ~BIT(irqd_to_hwirq(d));
>         dwapb_write(gpio, GPIO_INTEN, val);
> --
> 2.7.4
>

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Thomas Gleixner]

On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote:
> Sorry for top posting but I need the help of the irqchip maintainer
> Marc Z to hash this out.
>
> The mask/unmask/disable/enable semantics is something that
> you need to work with every day to understand right.

The patch is correct.

The irq_enable() callback is required to be a superset of
irq_unmask(). I.e. the core code expects it to do:

  1) Some preparatory work to enable the interrupt line

  2) Unmask the interrupt, which is why the masked state is cleared
     by the core after invoking the irq_enable() callback.

#2 is pretty obvious because if an interrupt chip does not implement the
irq_enable() callback the core defaults to irq_unmask()

Correspondingly the core expects from the irq_disable() callback:

   1) To mask the interrupt

   2) To do some extra work to disable the interrupt line

Same reasoning as above vs. #1 as the core fallback is to invoke the
irq_unmask() callback when the irq_disable() callback is not
implemented.

Thanks,

        tglx

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Serge Semin]

Hi Thomas

On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote:
> On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote:
> > Sorry for top posting but I need the help of the irqchip maintainer
> > Marc Z to hash this out.
> >
> > The mask/unmask/disable/enable semantics is something that
> > you need to work with every day to understand right.
> 
> The patch is correct.
> 
> The irq_enable() callback is required to be a superset of
> irq_unmask(). I.e. the core code expects it to do:
> 
>   1) Some preparatory work to enable the interrupt line
> 
>   2) Unmask the interrupt, which is why the masked state is cleared
>      by the core after invoking the irq_enable() callback.
> 
> #2 is pretty obvious because if an interrupt chip does not implement the
> irq_enable() callback the core defaults to irq_unmask()
> 
> Correspondingly the core expects from the irq_disable() callback:
> 
>    1) To mask the interrupt
> 
>    2) To do some extra work to disable the interrupt line
> 
> Same reasoning as above vs. #1 as the core fallback is to invoke the
> irq_unmask() callback when the irq_disable() callback is not
> implemented.

Just curious. Wouldn't that be more correct/portable for the core to
call both callbacks when it's required and if both are provided? So
the supersetness requirement would be no longer applied to the
IRQ enable/disable callbacks implementation thus avoiding the code
duplications in the low-level drivers.

-Serge(y)

> 
> Thanks,
> 
>         tglx
> 

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Thomas Gleixner]

On Fri, Dec 15 2023 at 13:24, Serge Semin wrote:
> On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote:
>> On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote:
>> > Sorry for top posting but I need the help of the irqchip maintainer
>> > Marc Z to hash this out.
>> >
>> > The mask/unmask/disable/enable semantics is something that
>> > you need to work with every day to understand right.
>> 
>> The patch is correct.
>> 
>> The irq_enable() callback is required to be a superset of
>> irq_unmask(). I.e. the core code expects it to do:
>> 
>>   1) Some preparatory work to enable the interrupt line
>> 
>>   2) Unmask the interrupt, which is why the masked state is cleared
>>      by the core after invoking the irq_enable() callback.
>> 
>> #2 is pretty obvious because if an interrupt chip does not implement the
>> irq_enable() callback the core defaults to irq_unmask()
>> 
>> Correspondingly the core expects from the irq_disable() callback:
>> 
>>    1) To mask the interrupt
>> 
>>    2) To do some extra work to disable the interrupt line
>> 
>> Same reasoning as above vs. #1 as the core fallback is to invoke the
>> irq_unmask() callback when the irq_disable() callback is not
>> implemented.
>
> Just curious. Wouldn't that be more correct/portable for the core to
> call both callbacks when it's required and if both are provided? So
> the supersetness requirement would be no longer applied to the
> IRQ enable/disable callbacks implementation thus avoiding the code
> duplications in the low-level drivers.

We could do that, but there are chips which require atomicity of the
operations (#1/#2). Not sure whether it safes much.

Thanks,

        tglx

Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Serge Semin]

On Fri, Dec 15, 2023 at 11:56:02AM +0100, Thomas Gleixner wrote:
> On Fri, Dec 15 2023 at 13:24, Serge Semin wrote:
> > On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote:
> >> On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote:
> >> > Sorry for top posting but I need the help of the irqchip maintainer
> >> > Marc Z to hash this out.
> >> >
> >> > The mask/unmask/disable/enable semantics is something that
> >> > you need to work with every day to understand right.
> >> 
> >> The patch is correct.
> >> 
> >> The irq_enable() callback is required to be a superset of
> >> irq_unmask(). I.e. the core code expects it to do:
> >> 
> >>   1) Some preparatory work to enable the interrupt line
> >> 
> >>   2) Unmask the interrupt, which is why the masked state is cleared
> >>      by the core after invoking the irq_enable() callback.
> >> 
> >> #2 is pretty obvious because if an interrupt chip does not implement the
> >> irq_enable() callback the core defaults to irq_unmask()
> >> 
> >> Correspondingly the core expects from the irq_disable() callback:
> >> 
> >>    1) To mask the interrupt
> >> 
> >>    2) To do some extra work to disable the interrupt line
> >> 
> >> Same reasoning as above vs. #1 as the core fallback is to invoke the
> >> irq_unmask() callback when the irq_disable() callback is not
> >> implemented.
> >
> > Just curious. Wouldn't that be more correct/portable for the core to
> > call both callbacks when it's required and if both are provided? So
> > the supersetness requirement would be no longer applied to the
> > IRQ enable/disable callbacks implementation thus avoiding the code
> > duplications in the low-level drivers.
> 
> We could do that, but there are chips which require atomicity of the
> operations (#1/#2). Not sure whether it safes much.

I see. Thanks for the answer. Right, seeing there are only three GPIO
drivers have such problem:
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-dwapb.c
drivers/gpio/gpio-hisi.c
it's better to leave the semantics as is. It just isn't worth it to
risk breaking so many platforms due to several drivers.

Regarding this patch implementation. It can be optimized a bit:
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4a4f61bf6c58..15943f67758c 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);
+	u32 mask = BIT(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) | mask;
 	dwapb_write(gpio, GPIO_INTEN, val);
+	val = dwapb_read(gpio, GPIO_INTMASK) & ~mask;
+	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);
+	u32 mask = BIT(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) | mask;
+	dwapb_write(gpio, GPIO_INTMASK, val);
+	val = dwapb_read(gpio, GPIO_INTEN) & ~mask;
 	dwapb_write(gpio, GPIO_INTEN, val);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }

Seeing Luo' email bounces back, Xiang (sorry if I spell your name
incorrectly), could you please test the patch above out whether it
fixes your problem, and then resubmit it?  Please don't forget to add
a Fixes tag. I guess the problem has been here since the driver
birthday:
Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block")

-Serge(y)


> 
> Thanks,
> 
>         tglx