Re: [Xen-devel] Why xen-pirq chip use startup_irq() for .irq_enable?

[shuo.a.liu@xxxxxxxxx]

On Fri 28.Jul'17 at 17:55:50 -0400, Boris Ostrovsky wrote:
> On 07/27/2017 09:25 PM, shuo.a.liu@xxxxxxxxx wrote:
> > On Thu 27.Jul'17 at 12:06:10 -0400, Boris Ostrovsky wrote:
> > > (Adjusting addressees: David is no longer maintaining Xen code,
> > > Juergen is)
> > Thanks Boris.
> > > On 07/27/2017 09:04 AM, shuo.a.liu@xxxxxxxxx wrote:
> > > > Hi,
> > > > Here is a device has xen-pirq-MSI interrupt. I found dom0 might lost
> > > > interrupt during driver irq_disable/irq_enable.
> > > > There is a pair of irq_disable/enable in driver. Here is the scenario,
> > > >  1. irq_disable(dev_irq) -> disable_dynirq -> mask_evtchn(dev_irq
> > > > channel)
> > > >  2. dev interrupt raised by HW and Xen mark its evtchn as *pending*
> > > > status.
> > > >  3. irq_enable(dev_irq) -> startup_pirq -> eoi_pirq ->
> > > >     clear_evtchn(channel of dev_irq) -> clear *pending* status
> > > >  4. consume_one_event process the dev irq event without pending bit
> > > > assert
> > > >     which result in interrupt lost once.
> > > >  5. No HW interrupt raising anymore.
> > > >
> > > > The first question here is why using startup_irq for .irq_enable
> > > > rather than
> > > > enable_dynirq ? startup_irq will do eoi_pirq who clear the mask bit
> > > > and pending
> > > > bit of the channel while enable_dynirq just only unmask the channel.
> > > Seems like enable_dynirq() would indeed be the right choice. What is a
> > > bit strange is that scenario that you are describing looks pretty common
> > > so we should have hit this problem before.
> > This point confused me also. It seems the code has been here for long
> > time.
> > Anyway, if you think it is the right fix, i can send out a formal patch.
> Yes, I think this shold be done.
OK, will do.
> > > > Second question is that what's the purpose of eoi_pirq in startup_irq?
> > > When we are actually creating new pirq we want to make sure there are no
> > > pending interrupts left over from previous use of the pirq.
> > If interrupt raise just before eoi_pirq in startup_irq, we might face
> > the same issue? Can we make sure pirq is clean when do binding?
> I rather think that
>
>        unmask_evtchn(evtchn);
>        eoi_pirq(irq_get_irq_data(irq));
>
> in __startup_pirq() should be swapped.
I agree. It should be good for new pirq setup.
> -boris
>
> > Thx -
> > Shuo
> >
> > > -boris
> > >
> > > > BTW, i can resolve my problem by below patch. Does it make sence?
> > > >
> > > > ---
> > > > drivers/xen/events/events_base.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/xen/events/events_base.c
> > > > b/drivers/xen/events/events_base.c
> > > > index 4bf7a34..341c456 100644
> > > > --- a/drivers/xen/events/events_base.c
> > > > +++ b/drivers/xen/events/events_base.c
> > > > @@ -582,7 +582,7 @@ static void shutdown_pirq(struct irq_data *data)
> > > >
> > > > static void enable_pirq(struct irq_data *data)
> > > > {
> > > > -    startup_pirq(data);
> > > > +    enable_dynirq(data);
> > > > }
> > > >
> > > > static void disable_pirq(struct irq_data *data)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel