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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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.

>>>
>>> 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.


-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