On Fri, Jan 10, 2020 at 08:13:16PM +0100, Thomas Gleixner wrote:
> Anchal,
>
> Anchal Agarwal <anchalag@xxxxxxxxxx> writes:
> > On Thu, Jan 09, 2020 at 01:07:27PM +0100, Thomas Gleixner wrote:
> >> Anchal Agarwal <anchalag@xxxxxxxxxx> 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
