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

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

[shuo.a.liu]

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.

Second question is that what's the purpose of eoi_pirq in startup_irq?

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


Thanks.
Shuo A


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

(Adjusting addressees: David is no longer maintaining Xen code, Juergen is)

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.

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

-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

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

[shuo.a.liu]

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

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

[Boris Ostrovsky]

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.

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

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

[shuo.a.liu]

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