Re: [PATCH v4 3/7] genirq: Introduce irq_suspend_one() and irq_resume_one() callbacks

[Maulik Shah]

Hi,

On 8/14/2020 4:28 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Aug 13, 2020 at 3:09 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > > Specifically the problem we're trying to address is when an IRQ is
> > > marked as "disabled" (driver called disable_irq()) but also marked as
> > > "wakeup" (driver called enable_irq_wake()).  As per my understanding,
> > > this means:
> > >
> > > * Don't call the interrupt handler for this interrupt until I call
> > > enable_irq() but keep tracking it (either in hardware or in software).
> > > Specifically it's a requirement that if the interrupt fires one or
> > > more times while masked the interrupt handler should be called as soon
> > > as enable_irq() is called.
> > irq_disable() has two operating modes:
> >
> >      1) Immediately mask the interrupt at the irq chip level
> >
> >      2) Software disable it. If an interrupt is raised while disabled
> >         then the flow handler observes disabled state, masks it, marks it
> >         pending and returns without invoking any device handler.
> >
> > On a subsequent irq_enable() the interrupt is unmasked if it was masked
> > and if the interrupt is marked pending and the interrupt is not level
> > type then it's attempted to retrigger it. Either in hardware or by a
> > software replay mechanism.
> >
> > > * If this interrupt fires while the system is suspended then please
> > > wake the system up.
> > Well, that's kinda contradicting itself. If the interrupt is masked then
> > what is the point? I'm surely missing something subtle here.
> This is how I've always been told that the API works and there are at
> least a handful of drivers in the kernel whose suspend routines both
> enable wakeup and call disable_irq().  Isn't this also documented as
> of commit f9f21cea3113 ("genirq: Clarify that irq wake state is
> orthogonal to enable/disable")?
>
>
> > > On some (many?) interrupt controllers a masked interrupt won't wake
> > > the system up.  Thus we need some point in time where the interrupt
> > > controller can unmask interrupts in hardware so that they can act as
> > > wakeups.
> > So far nobody told me about this until now, but why exactly do we need
> > yet another unspecified callback instead of simply telling the core via
> > an irq chip flag that it should always unmask the interrupt if it is a
> > wakeup source?
> >
> > > Also: if an interrupt was masked lazily this could be a good
> > > time to ensure that these interrupts _won't_ wake the system up.
> > Setting IRQCHIP_MASK_ON_SUSPEND does exactly that. No need for a chip
> > driver to do any magic. You just have to use it.
> >
> > So the really obvious counterpart for this is to have:
> >
> >         IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND
> >
> > and then do:
> >
> > @@ -81,6 +81,8 @@ static bool suspend_device_irq(struct ir
> >                   * IRQD_WAKEUP_ARMED is visible before we return from
> >                   * suspend_device_irqs().
> >                   */
> > +               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
> > +                       unmask_irq(desc);
> >                  return true;
> >          }
> >
> > plus the counterpart in the resume path. This also ensures that state is
> > consistent.
> This sounds wonderful to me.  Maulik: I think you could replace quite
> a few of the patches in the series and just use that.

Sure.

+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
+                       unmask_irq(desc);

I tried this patch and it didnot work as is.

Calling unmask_irq() only invoke's chip's .irq_unmask callback but the 
underlying irq_chip have .irq_enable also present.

Replacing the call with irq_enable() internally takes care of either 
invoking chip's .irq_enable (if its present) else it invokes unmask_irq().

+
+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
+                       irq_enable(desc);

probably IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND should also be renamed to 
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND.

Thanks,
Maulik

> > The magic behind the back of the core code unmask brings core state and
> > hardware state out of sync. So if for whatever reason the interrupt is
> > raised in the CPU before the resume path can mask it again, then the
> > flow handler will see disabled state, invoke mask_irq() which does
> > nothing because core state is masked and if that's a level irq it will
> > come back forever.
> >
> > > Thus the point of these callbacks is to provide a hook for IRQ chips
> > > to do this.  Now that you understand the motivation perhaps you can
> > > suggest a better way to accomplish this if the approach in this patch
> > > is not OK.
> > See above.
> >
> > > I will note that a quick audit of existing users of the gernic-chip's
> > > irq_suspend() show that they are doing exactly this.  So the point of
> > > my patch is to actually allow other IRQ chips (ones that aren't using
> > > generic-chip) to do this type of thing.  At the same time my patch
> > > provides a way for current users of generic-chip to adapt their
> > > routines so they work without syscore (which, I guess, isn't
> > > compatible with s2idle).
> > If that's the main problem which is solved in these callbacks, then I
> > really have to ask why this has not been raised years ago. Why can't
> > people talk?
> Not all of us have the big picture that you do to know how things
> ought to work, I guess.  If nothing else someone looking at this
> problem would think: "this must be a common problem, let's go see how
> all the other places do it" and then they find how everyone else is
> doing it and do it that way.  It requires the grander picture that a
> maintainer has in order to say: whoa, everyone's copying the same
> hack--let's come up with a better solution.
>
>
> > IIRC back then when the callbacks for GC were added the reason was that
> > the affected chips needed a way to save and restore the full chip state
> > because the hardware lost it during suspend. S2idle did not exist back
> > then at least not in it's current form. Oh well...
> >
> > But gust replacing them by something which is yet another sinkhole for
> > horrible hacks behind the core code is not making it any better.
> >
> > I fear another sweep through the unpleasantries of chip drivers is due
> > sooner than later. Aside of finding time, I need to find my eyecancer
> > protection glasses and check my schnaps stock.
> >
> > > > So what happens in this case:
> > > >
> > > >     CPU0                         CPU1
> > > >     interrupt                    suspend_device_irq()
> > > >       handle()                     chip->suspend_one()
> > > >         action()                 ...
> > > >         chip->fiddle();
> > > >
> > > > ????
> > > Ah, so I guess we need to move the call to suspend_one_irq() till
> > > after the (potential) call to synchronize_irq() in in
> > > suspend_device_irqs()?
> > For what you are trying to achieve, no. IRQCHIP_MASK_ON_SUSPEND is
> > already safe.
> >
> > If we add IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND then there is no sync
> > problem either.
> >
> > > Hopefully with the above explanation this makes more sense?
> > At least the explanation helped to understand the problem, while the
> > changelog was pretty useless in that regard:
> >
> >    "These two callbacks are interesting because sometimes an irq chip
> >     needs to know about suspend/resume."
> >
> > Really valuable and precise technical information.
> Funny to get yelled at for not providing a detailed enough changelog.
> Usually people complain that my changelogs are too detailed.  Sigh.
>
>
> > But aside of the confusion, even with your explanation of what you are
> > trying to solve, I really want a coherent explanation why this should be
> > done for any of those:
> >
> >    1) an interrupt which has no action, i.e. an interrupt which has no
> >       active users and is in the worst case completely deactivated or was
> >       never activated to begin with.
> >
> >       In the inactive case it might be in a state where unmask issues an
> >       invalid vector, causes hardware malfunction or hits undefined
> >       software state in the chip drivers in the hierarchy.
> >
> >       If you want to be woken up by irq X, then request irq X which
> >       ensures that irq X is in a usable state at all levels of the
> >       stack. If you call disable_irq() or mark the interrupt with
> >       IRQ_NOAUTOEN, fine, it's still consistent state.
> >
> >    2) interrupts which have no_suspend_depth > 0 which means that
> >       there is an action requested which explicitely says: don't touch me
> >       on suspend.
> >
> >       If that driver invokes disable_irq() then it can keep the pieces.
> >
> >    3) chained interrupts
> >
> >       They are never disabled and never masked. So why would anything
> >       need to be done here?
> >
> >       Side note: they should not exist at all, but that's a different
> >       story.
> >
> > If you don't have coherent explanations, then please just don't touch
> > that condition at all.
> >
> > Hint: "Sometimes a chip needs to know" does not qualify :)
> Clearly I am not coherent.  ;-)  My only goal was to help enable
> interrupts that were disabled / marked as wakeup (as per above,
> documented to be OK) to work on Qualcomm chips.  This specifically
> affects me because a driver that I need to work (cros_ec) does this.
> If IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND is good to add then it sounds like
> a great plan to me.
>
>
> -Doug

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation