Re: [RFC PATCH V2 09/11] xen: Clear IRQD_IRQ_STARTED flag during shutdown PIRQs

[Anchal Agarwal]

On Wed, Jan 08, 2020 at 04:23:25PM +0100, Thomas Gleixner wrote:
> Anchal Agarwal <anchalag@xxxxxxxxxx> writes:
> 
> > shutdown_pirq is invoked during hibernation path and hence
> > PIRQs should be restarted during resume.
> > Before this commit'020db9d3c1dc0a' xen/events: Fix interrupt lost
> > during irq_disable and irq_enable startup_pirq was automatically
> > called during irq_enable however, after this commit pirq's did not
> > get explicitly started once resumed from hibernation.
> >
> > chip->irq_startup is called only if IRQD_IRQ_STARTED is unset during
> > irq_startup on resume. This flag gets cleared by free_irq->irq_shutdown
> > during suspend. free_irq() never gets explicitly called for ioapic-edge
> > and ioapic-level interrupts as respective drivers do nothing during
> > suspend/resume. So we shut them down explicitly in the first place in
> > syscore_suspend path to clear IRQ<>event channel mapping. shutdown_pirq
> > being called explicitly during suspend does not clear this flags, hence
> > .irq_enable is called in irq_startup during resume instead and pirq's
> > never start up.
> 
> What? 
> 
> > +void irq_state_clr_started(struct irq_desc *desc)
> >  {
> >  	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED);
> >  }
> > +EXPORT_SYMBOL_GPL(irq_state_clr_started);
> 
> This is core internal state and not supposed to be fiddled with by
> drivers.
> 
> irq_chip has irq_suspend/resume/pm_shutdown callbacks for a reason.
>
I agree, as its mentioned in the previous patch {[RFC PATCH V2 08/11]} this is 
one way of explicitly shutting down legacy devices without introducing too much 
code for each of the legacy devices. . for eg. in case of floppy there 
is no suspend/freeze handler which should have done the needful.
.
Either we implement them for all the legacy devices that have them missing or
explicitly shutdown pirqs. I have choosen later for simplicity. I understand
that ideally we should enable/disable devices interrupts in suspend/resume 
devices but that requires adding code for doing that to few drivers[and I may
not know all of them either]

Now I discovered during the flow in hibernation_platform_enter under resume 
devices that for such devices irq_startup is called which checks for 
IRQD_IRQ_STARTED flag and based on that it calls irq_enable or irq_startup.
They are only restarted if the flag is not set which is cleared during shutdown. 
shutdown_pirq does not do that. Only masking/unmasking of evtchn does not work 
as pirq needs to be restarted.
xen-pirq.enable_irq is called rather than stratup_pirq. On resume if these pirqs
are not restarted in this case ACPI SCI interrupts, I do not see receiving 
any interrupts under cat /proc/interrupts even though host keeps generating 
S4 ACPI events. 
Does that makes sense?

Thanks,
Anchal
> Thanks,
> 
>        tglx