Re: [RFC PATCH V2 09/11] xen: Clear IRQD_IRQ_STARTED flag during shutdown PIRQs
From: Anchal Agarwal
Date: Thu Jan 09 2020 - 18:41:31 EST
On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> Anchal Agarwal <anchalag@xxxxxxxxxx> writes:
> > On Wed, Jan 08, 2020 at 04:23:25PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@xxxxxxxxxx> writes:
> >> > +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?
>
> No. You still violate all abstraction boundaries. On one hand you use a
> XEN specific suspend function to shut down interrupts, but then you want
> the core code to reestablish them on resume. That's just bad hackery which
> abuses partial knowledge of core internals. The state flag is only one
> part of the core internal state and just clearing it does not make the
> rest consistent. It just works by chance and not by design and any
> change of the core code will break it in colourful ways.
>
> So either you can handle it purely on the XEN side without touching any
> core state or you need to come up with some way of letting the core code
> know that it should invoke shutdown instead of disable.
>
> Something like the completely untested patch below.
>
> Thanks,
>
> tglx
Understandable. Really appreciate the patch suggestion below and i will test it
for sure and see if things can be fixed properly in irq core if thats the only
option. In the meanwhile, I tried to fix it on xen side unless it gives you the
same feeling as above? MSI-x are just fine, just ioapic ones don't get any event
channel asssigned hence enable_dynirq does nothing. Those needs to be restarted.
Thanks,
Anchal
<-----------------------
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1bb0b522d004..2ed152f35816 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -575,6 +575,11 @@ static void shutdown_pirq(struct irq_data *data)
static void enable_pirq(struct irq_data *data)
{
+/*ioapic interrupts don't get event channel assigned
+ * after being explicitly shutdown during guest
+ * hibernation. They need to be restarted*/
+ if(!evtchn_from_irq(data->irq))
+ startup_pirq(data);
enable_dynirq(data);
}
>
> 8<----------------
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 7853eb9301f2..50f2057bc339 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -511,6 +511,7 @@ struct irq_chip {
> * IRQCHIP_EOI_THREADED: Chip requires eoi() on unmask in threaded mode
> * IRQCHIP_SUPPORTS_LEVEL_MSI Chip can provide two doorbells for Level MSIs
> * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips
> + * IRQCHIP_SHUTDOWN_ON_SUSPEND: Shutdown non wake irqs in the suspend path
> */
> enum {
> IRQCHIP_SET_TYPE_MASKED = (1 << 0),
> @@ -522,6 +523,7 @@ enum {
> IRQCHIP_EOI_THREADED = (1 << 6),
> IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7),
> IRQCHIP_SUPPORTS_NMI = (1 << 8),
> + IRQCHIP_SHUTDOWN_ON_SUSPEND = (1 << 9),
> };
>
> #include <linux/irqdesc.h>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b3fa2d87d2f3..0fe355f72a15 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -233,7 +233,7 @@ __irq_startup_managed(struct irq_desc *desc, struct cpumask *aff, bool force)
> }
> #endif
>
> -static int __irq_startup(struct irq_desc *desc)
> +int __irq_startup(struct irq_desc *desc)
> {
> struct irq_data *d = irq_desc_get_irq_data(desc);
> int ret = 0;
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 3924fbe829d4..11c7c55bda63 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -80,6 +80,7 @@ extern void __enable_irq(struct irq_desc *desc);
> extern int irq_activate(struct irq_desc *desc);
> extern int irq_activate_and_startup(struct irq_desc *desc, bool resend);
> extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
> +extern int __irq_startup(struct irq_desc *desc);
>
> extern void irq_shutdown(struct irq_desc *desc);
> extern void irq_shutdown_and_deactivate(struct irq_desc *desc);
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 8f557fa1f4fe..597f0602510a 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -85,16 +85,22 @@ static bool suspend_device_irq(struct irq_desc *desc)
> }
>
> desc->istate |= IRQS_SUSPENDED;
> - __disable_irq(desc);
> -
> - /*
> - * Hardware which has no wakeup source configuration facility
> - * requires that the non wakeup interrupts are masked at the
> - * chip level. The chip implementation indicates that with
> - * IRQCHIP_MASK_ON_SUSPEND.
> - */
> - if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
> - mask_irq(desc);
> +
> + /* Some irq chips (e.g. XEN PIRQ) require a full shutdown on suspend */
> + if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND) {
> + irq_shutdown(desc);
> + } else {
> + __disable_irq(desc);
> +
> + /*
> + * Hardware which has no wakeup source configuration facility
> + * requires that the non wakeup interrupts are masked at the
> + * chip level. The chip implementation indicates that with
> + * IRQCHIP_MASK_ON_SUSPEND.
> + */
> + if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
> + mask_irq(desc);
> + }
> return true;
> }
>
> @@ -152,7 +158,10 @@ static void resume_irq(struct irq_desc *desc)
> irq_state_set_masked(desc);
> resume:
> desc->istate &= ~IRQS_SUSPENDED;
> - __enable_irq(desc);
> + if (irq_desc_get_chip(desc)->flags & IRQCHIP_SHUTDOWN_ON_SUSPEND)
> + __irq_startup(desc);
> + else
> + __enable_irq(desc);
> }
>
> static void resume_irqs(bool want_early)