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

[shuo.a.liu@intel.com]

On Fri 28.Jul'17 at 17:55:50 -0400, Boris Ostrovsky wrote:
>On 07/27/2017 09:25 PM, shuo.a.liu@intel.com 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@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.
>
>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@lists.xen.org
https://lists.xen.org/xen-devel