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

[Maulik Shah <mkshah@codeaurora.org>]

Hi,

On 8/14/2020 4:28 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Aug 13, 2020 at 3:09 PM Thomas Gleixner <tglx@linutronix.de> 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