Re: [PATCH v5 4/4] pinctrl: qcom: Don't clear pending interrupts when enabling

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Thu, Jan 14, 2021 at 9:15 AM Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
>
> > > @@ -195,6 +201,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > >         if (WARN_ON(i == g->nfuncs))
> > >                 return -EINVAL;
> > >
> > > +       /*
> > > +        * If an GPIO interrupt is setup on this pin then we need special
> > > +        * handling.  Specifically interrupt detection logic will still see
> > > +        * the pin twiddle even when we're muxed away.
> > > +        *
> > > +        * When we see a pin with an interrupt setup on it then we'll disable
> > > +        * (mask) interrupts on it when we mux away until we mux back.  Note
> > > +        * that disable_irq() refcounts and interrupts are disabled as long as
> > > +        * at least one disable_irq() has been called.
> > > +        */
> > > +       if (d && i != gpio_func &&
> > > +           !test_and_set_bit(d->hwirq, pctrl->disabled_for_mux))
> > > +               disable_irq(irq);
> >
> > Does it need to be forced non-lazy so that it is actually disabled at
> > the GIC? I'm trying to understand how the lazy irq disabling plays into
> > this. I think it's a don't care situation because if the line twiddles
> > and triggers an irq then we'll actually disable it at the GIC in the
> > genirq core and mark it pending for resend. I wonder if we wouldn't have
> > to undo the pending state if we actually ignored it at the GIC
> > forcefully. And I also worry that it may cause a random wakeup if the
> > line twiddles, becomes pending at GIC and thus blocks the CPU from
> > running a WFI but it isn't an irq that Linux cares about because it's
> > muxed to UART, and then lazy handling runs and shuts it down. Is that
> > possible?
> >
>
> I was about to write a question about why we should disable the IRQ
> through the irqchip framework, rather than just do it in the hardware
> directly.
>
> Which I think means that I came to the same conclusion as you, that if
> we have a pin masked to non-gpio, it will still wake the system up, just
> to actually disable the IRQ lazily.
>
> Is there a problem with leaving the irq framework to believe the IRQ is
> enabled while we disable the delivery in hardware?

Earlier I had it disabling in hardware, but doing it through the IRQ
framework has the advantage of doing refcounting for us and that saves
us complexity.  If we tweaked the hardware directly we'd have to worry
about this case:

a) Client muxes away the pin: we disable in hardware
b) Client tries to disable/mask the interrupt themselves.
c) Client tries to enable/unmask the interrupt themselves

...when we got the call for c) we'd have to realize that we're still
muxed away and we'd have to ignore their request.  Also, if the mux
back happened in step b) we'd have to know _not_ to unmask the
interrupt.  Trying to solve those corner cases adds complexity.  If we
just rely on the refcounting the complexity goes away.


-Doug