diff mbox series

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

Message ID 1606728979-44259-1-git-send-email-luojiaxing@huawei.com
State New
Headers show
Series [v1] gpio: dwapb: mask/unmask IRQ when disable/enable it | expand

Commit Message

luojiaxing Nov. 30, 2020, 9:36 a.m. UTC
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(+)

Comments

Andy Shevchenko Nov. 30, 2020, 11:22 a.m. UTC | #1
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?
luojiaxing Dec. 1, 2020, 8:59 a.m. UTC | #2
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.
Linus Walleij Dec. 5, 2020, 9:58 p.m. UTC | #3
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
>
Serge Semin Dec. 5, 2020, 10:15 p.m. UTC | #4
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

> 
> 
>
Linus Walleij Dec. 6, 2020, 3:02 p.m. UTC | #5
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
Marc Zyngier Dec. 6, 2020, 6:50 p.m. UTC | #6
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.
luojiaxing Dec. 7, 2020, 12:44 p.m. UTC | #7
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
>
>>
>>
> .
>
luojiaxing Dec. 7, 2020, 1:04 p.m. UTC | #8
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
>
> .
>
luojiaxing Dec. 7, 2020, 1:10 p.m. UTC | #9
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.
Bartosz Golaszewski Jan. 6, 2021, 10:24 a.m. UTC | #10
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
Serge Semin Jan. 7, 2021, 12:20 p.m. UTC | #11
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
diff mbox series

Patch

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