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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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@xxxxxxxxxxxx> wrote:
> 
> > On Mon, Dec 7, 2020 at 2:10 PM luojiaxing <luojiaxing@xxxxxxxxxx> 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@xxxxxxxxx>
> > > >> 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