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

[Stephen Boyd <swboyd@xxxxxxxxxxxx>]

Quoting Doug Anderson (2021-01-14 09:58:55)
> Hi,
> 
> On Wed, Jan 13, 2021 at 11:14 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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?
> 
> Yes, I think non-lazy is important.  Specifically at the end I assume
> that I can clear the interrupt in hardware and it will go away and
> Linux never saw it.  If it was lazy, it's possible Linux saw the
> interrupt and has it marked with IRQS_PENDING.
> 
> Right now we get non-lazy because we have "disable" implemented, so it
> works fine.  I can be explicit.  Do I add a call to
> msm_gpio_irq_reqres() like:
> 
>   irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);

Yes that should be done explicitly. I suppose when the irq is requested
the first time is when we should do it. Or we can add a flag to gpiolib
to do that all the time for the irq domain? Basically make it so
gpiochip_hierarchy_irq_domain_alloc() sets the flag.

> 
> I'll wait for feedback if you think this is the right way to go before
> sending the next version.
> 
> 
> > 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 think the marking as pending is a problem.  When we finally mux back
> to GPIO we want to clear out anything that showed up while it was
> muxed away and I'm not aware of a way to clear "IRQS_PENDING".

Ok.

> 
> 
> > 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 believe if the interrupt is masked at the GIC then it won't cause
> wakeups.  Specifically to get wakeup stuff working we had to unmask
> the interrupt at the GIC level.

If I understand correctly, the lazy and non-lazy cases will masked at
the GIC either after the line twiddles or immediately here respectfully.
So either way we should be OK because it will be masked, but I'm still
unsure about the lazy case where we are in the process of suspending and
then the line twiddles. That would cause the irq to block suspend,
possibly really late if the CPU is running trusted firmware. So we
really need to make sure that it is non-lazy so this can't happen.

> 
> 
> > > +
> > >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > >         val = msm_readl_ctl(pctrl, g);
> > > @@ -204,6 +224,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > >
> > >         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > >
> > > +       if (d && i == gpio_func &&
> > > +           test_and_clear_bit(d->hwirq, pctrl->disabled_for_mux)) {
> > > +               /*
> > > +                * Clear interrupts detected while not GPIO since we only
> > > +                * masked things.
> > > +                */
> > > +               if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > > +                       irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
> >
> > So if not lazy this could go away?
> 
> I don't think so.  If lazy we could have a pending interrupt tracked
> in two places: in Linux and in the parent if this happened:
> * disable_irq() - do nothing except mark that IRQ is disalbed.
> * IRQ happened - track in Linux and actually disable (mask) the interrupt
> * IRQ happened again - still tracked in Linux but now also latched in
> hardware (but masked)
> 
> ...so if it was lazy we'd need to clear the interrupt in two places.
> With non-lazy we only have to clear the latch that happened in
> hardware, right?

Yes makes sense.

> 
> > Although I think this is to clear out
> > the pending state in the GIC and not the PDC which is the parent.
> 
> Yeah, it clears the state that was latched in the GIC.  It just passes
> through the PDC code on the way there.

Got it. I'm happy with this patch once it explicitly disables the lazy
mode of the gpio irqs.