Re: Why xen-pirq chip use startup_irq() for .irq_enable?

[shuo.a.liu@intel.com]

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@intel.com 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.
>>
>> 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?

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@lists.xen.org
https://lists.xen.org/xen-devel