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

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

[Anchal Agarwal]

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.

Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
---
 drivers/xen/events/events_base.c | 1 +
 include/linux/irq.h              | 1 +
 kernel/irq/chip.c                | 3 ++-
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index b893536d8af4..aae7c4997b51 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1606,6 +1606,7 @@ void xen_shutdown_pirqs(void)
 			continue;
 
 		shutdown_pirq(irq_get_irq_data(info->irq));
+		irq_state_clr_started(irq_to_desc(info->irq));
 	}
 }
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf29148..1e125cd22cf0 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -745,6 +745,7 @@ extern int irq_set_msi_desc(unsigned int irq, struct msi_desc *entry);
 extern int irq_set_msi_desc_off(unsigned int irq_base, unsigned int irq_offset,
 				struct msi_desc *entry);
 extern struct irq_data *irq_get_irq_data(unsigned int irq);
+extern void irq_state_clr_started(struct irq_desc *desc);
 
 static inline struct irq_chip *irq_get_chip(unsigned int irq)
 {
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b2c0af..3e8a36c673d6 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -173,10 +173,11 @@ static void irq_state_clr_masked(struct irq_desc *desc)
 	irqd_clear(&desc->irq_data, IRQD_IRQ_MASKED);
 }
 
-static void irq_state_clr_started(struct irq_desc *desc)
+void irq_state_clr_started(struct irq_desc *desc)
 {
 	irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED);
 }
+EXPORT_SYMBOL_GPL(irq_state_clr_started);
 
 static void irq_state_set_started(struct irq_desc *desc)
 {
-- 
2.15.3.AMZN

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

[Thomas Gleixner]

Anchal Agarwal <anchalag@amazon.com> 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.

Thanks,

       tglx

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@amazon.com> 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

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

[Thomas Gleixner]

Anchal Agarwal <anchalag@amazon.com> writes:
> On Wed, Jan 08, 2020 at 04:23:25PM +0100, Thomas Gleixner wrote:
>> Anchal Agarwal <anchalag@amazon.com> 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

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)

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

[Anchal Agarwal]

On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> Anchal Agarwal <anchalag@amazon.com> writes:
> > On Wed, Jan 08, 2020 at 04:23:25PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@amazon.com> 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)

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

[Thomas Gleixner]

Anchal,

Anchal Agarwal <anchalag@amazon.com> writes:
> On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
>> Anchal Agarwal <anchalag@amazon.com> writes:
>> 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.
>
> 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.
>
> 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);
>  }

Interesting patch format :)

Doing the shutdown from syscore_ops and the startup conditionally in a
totaly unrelated function is not really intuitive.

So either you do it symmetrically in XEN via syscore_ops callbacks or
you let the irq core code help you out with the patch I provided

Thanks,

        tglx

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

[Anchal Agarwal]

On Fri, Jan 10, 2020 at 08:13:16PM +0100, Thomas Gleixner wrote:
> Anchal,
> 
> Anchal Agarwal <anchalag@amazon.com> writes:
> > On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@amazon.com> writes:
> >> 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.
> >
> > 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.
> >
> > 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);
> >  }
> 
> Interesting patch format :)
Apparently vim and me rushing through the email [did not format the patch]
were the culprit and I only caught it after sending an email
> 
> Doing the shutdown from syscore_ops and the startup conditionally in a
> totaly unrelated function is not really intuitive.
> 
I agree to the point that still the startup is not as synchronous 
to shutdown however, enable_pirq is still invoked during irq_startup
for xen specific code and I was trying to reuse the code path to fix 
within xen. Basically borrowing from what this commit [commit 020db9d3]
changed. Not sure if this could have broken under any other environment
though :(

But anyways I think the patch you suggested is much more clean and 
intuitive.

> So either you do it symmetrically in XEN via syscore_ops callbacks or
> you let the irq core code help you out with the patch I provided
> 
In my understanding, it may not be the right thing as syscore stuff runs
with one cpu online and disabled interrupts. Also I did try it in the past 
and failed horribly unless there is any smarter way of doing it.
It should correctly be done in suspend/resume devices as are other device 
interrupts.

I did test the patch you suggested and it works.
I haven't done large scale testing but it looks like it may just work fine.
I will send out an updated patch for shutdown/startup of pirq after I do some
more testing and will drop patches related to shutdown/startup of pirqs from 
the original series.

Thanks,

Anchal

> Thanks,
> 
>         tglx