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

[Anchal Agarwal <anchalag@amazon.com>]

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)