[Xenomai] one-shot fasteoi irqs

[Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

Hi Jan,

I seem to recall you sent patches to fix threaded irqs some time ago. I 
am having a problem here without SMP, whereas the system works with 
SMP. A fasteoi irq handler triggers repeatedly while its threaded 
counterpart never gets triggered. I believe this is because 
handle_fasteoi_irq unconditionally releases the irq line. The following 
patch seems to fix the issue though I would prefer a cleaner solution.

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 11e75d1..2ff8d3a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 
 #ifdef CONFIG_IPIPE
 	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
-	if (desc->irq_data.chip->irq_release)
-		desc->irq_data.chip->irq_release(&desc->irq_data);
+	if (desc->irq_data.chip->irq_release) {
+		if ((!(desc->istate & IRQS_ONESHOT) ||
+		     !desc->threads_oneshot))
+			desc->irq_data.chip->irq_release(&desc->irq_data);
+	}
 out_eoi:
 #else  /* !CONFIG_IPIPE */
 	if (desc->istate & IRQS_ONESHOT)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index e49a288..485c2c4 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -715,9 +715,15 @@ again:
 
 	desc->threads_oneshot &= ~action->thread_mask;
 
-	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
-	    irqd_irq_masked(&desc->irq_data))
-		unmask_irq(desc);
+	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) {
+#ifdef CONFIG_IPIPE
+		if (desc->irq_data.chip->irq_release)
+			desc->irq_data.chip->irq_release(&desc->irq_data);
+		else 
+#endif
+			if (irqd_irq_masked(&desc->irq_data))
+				unmask_irq(desc);
+	}
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);


-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Jan Kiszka]

On 2013-03-27 00:05, Gilles Chanteperdrix wrote:
> 
> Hi Jan,
> 
> I seem to recall you sent patches to fix threaded irqs some time ago. I 

In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for
2.6.38 back then.

> am having a problem here without SMP, whereas the system works with 
> SMP. A fasteoi irq handler triggers repeatedly while its threaded 
> counterpart never gets triggered. I believe this is because 
> handle_fasteoi_irq unconditionally releases the irq line.

You mean it doesn't respect the conditions in cond_unmask_irq?

> The following 
> patch seems to fix the issue though I would prefer a cleaner solution.
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 11e75d1..2ff8d3a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>  
>  #ifdef CONFIG_IPIPE
>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
> -	if (desc->irq_data.chip->irq_release)
> -		desc->irq_data.chip->irq_release(&desc->irq_data);
> +	if (desc->irq_data.chip->irq_release) {
> +		if ((!(desc->istate & IRQS_ONESHOT) ||
> +		     !desc->threads_oneshot))
> +			desc->irq_data.chip->irq_release(&desc->irq_data);
> +	}
>  out_eoi:
>  #else  /* !CONFIG_IPIPE */
>  	if (desc->istate & IRQS_ONESHOT)
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index e49a288..485c2c4 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -715,9 +715,15 @@ again:
>  
>  	desc->threads_oneshot &= ~action->thread_mask;
>  
> -	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
> -	    irqd_irq_masked(&desc->irq_data))
> -		unmask_irq(desc);
> +	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) {
> +#ifdef CONFIG_IPIPE
> +		if (desc->irq_data.chip->irq_release)
> +			desc->irq_data.chip->irq_release(&desc->irq_data);
> +		else 
> +#endif
> +			if (irqd_irq_masked(&desc->irq_data))
> +				unmask_irq(desc);
> +	}
>  
>  out_unlock:
>  	raw_spin_unlock_irq(&desc->lock);
> 
> 

So this basically extends I-pipe's held phase of the IRQ to the threaded
handler if we are working in oneshot mode, right?

I will have to dig into this again, but without remembering all details,
there are some things that do not look OK:
 - Shouldn't we just replace unmask_irq with irq_release for the I-pipe
   case? IOW: There is too much code under #ifdef CONFIG_IPIPE.
 - It seems we do not respect masking done by Linux, rather
   unconditionally release (AKA unmask) the line.

The latter was right in 2.6.38 but apparently got broken during to port
from 3.2 to 3.4.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130327/d590dc7c/attachment.pgp>

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 03/27/2013 07:50 AM, Jan Kiszka wrote:

> On 2013-03-27 00:05, Gilles Chanteperdrix wrote:
>>
>> Hi Jan,
>>
>> I seem to recall you sent patches to fix threaded irqs some time ago. I 
> 
> In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for
> 2.6.38 back then.
> 
>> am having a problem here without SMP, whereas the system works with 
>> SMP. A fasteoi irq handler triggers repeatedly while its threaded 
>> counterpart never gets triggered. I believe this is because 
>> handle_fasteoi_irq unconditionally releases the irq line.
> 
> You mean it doesn't respect the conditions in cond_unmask_irq?
> 
>> The following 
>> patch seems to fix the issue though I would prefer a cleaner solution.
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 11e75d1..2ff8d3a 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>  
>>  #ifdef CONFIG_IPIPE
>>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>> -	if (desc->irq_data.chip->irq_release)
>> -		desc->irq_data.chip->irq_release(&desc->irq_data);
>> +	if (desc->irq_data.chip->irq_release) {
>> +		if ((!(desc->istate & IRQS_ONESHOT) ||
>> +		     !desc->threads_oneshot))
>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>> +	}
>>  out_eoi:
>>  #else  /* !CONFIG_IPIPE */
>>  	if (desc->istate & IRQS_ONESHOT)
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index e49a288..485c2c4 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -715,9 +715,15 @@ again:
>>  
>>  	desc->threads_oneshot &= ~action->thread_mask;
>>  
>> -	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>> -	    irqd_irq_masked(&desc->irq_data))
>> -		unmask_irq(desc);
>> +	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) {
>> +#ifdef CONFIG_IPIPE
>> +		if (desc->irq_data.chip->irq_release)
>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>> +		else 
>> +#endif
>> +			if (irqd_irq_masked(&desc->irq_data))
>> +				unmask_irq(desc);
>> +	}
>>  
>>  out_unlock:
>>  	raw_spin_unlock_irq(&desc->lock);
>>
>>
> 
> So this basically extends I-pipe's held phase of the IRQ to the threaded
> handler if we are working in oneshot mode, right?
> 
> I will have to dig into this again, but without remembering all details,
> there are some things that do not look OK:
>  - Shouldn't we just replace unmask_irq with irq_release for the I-pipe
>    case? IOW: There is too much code under #ifdef CONFIG_IPIPE.


No, unmask_irq still has to use ->irq_unmask, while the masking done by
the I-pipe has to use ->irq_release.

>  - It seems we do not respect masking done by Linux, rather
>    unconditionally release (AKA unmask) the line.


You mean masking which could be done by the interrupt handler?

> 
> The latter was right in 2.6.38 but apparently got broken during to port
> from 3.2 to 3.4.


fasteoi irqs were reworked with the introduction of ->irq_hold and
->irq_release, because the 2.6.38 was found not to work either...

-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Jan Kiszka]

On 2013-03-27 10:23, Gilles Chanteperdrix wrote:
> On 03/27/2013 07:50 AM, Jan Kiszka wrote:
> 
>> On 2013-03-27 00:05, Gilles Chanteperdrix wrote:
>>>
>>> Hi Jan,
>>>
>>> I seem to recall you sent patches to fix threaded irqs some time ago. I 
>>
>> In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for
>> 2.6.38 back then.
>>
>>> am having a problem here without SMP, whereas the system works with 
>>> SMP. A fasteoi irq handler triggers repeatedly while its threaded 
>>> counterpart never gets triggered. I believe this is because 
>>> handle_fasteoi_irq unconditionally releases the irq line.
>>
>> You mean it doesn't respect the conditions in cond_unmask_irq?
>>
>>> The following 
>>> patch seems to fix the issue though I would prefer a cleaner solution.
>>>
>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>> index 11e75d1..2ff8d3a 100644
>>> --- a/kernel/irq/chip.c
>>> +++ b/kernel/irq/chip.c
>>> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>>  
>>>  #ifdef CONFIG_IPIPE
>>>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>> -	if (desc->irq_data.chip->irq_release)
>>> -		desc->irq_data.chip->irq_release(&desc->irq_data);
>>> +	if (desc->irq_data.chip->irq_release) {
>>> +		if ((!(desc->istate & IRQS_ONESHOT) ||
>>> +		     !desc->threads_oneshot))
>>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>>> +	}
>>>  out_eoi:
>>>  #else  /* !CONFIG_IPIPE */
>>>  	if (desc->istate & IRQS_ONESHOT)
>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>> index e49a288..485c2c4 100644
>>> --- a/kernel/irq/manage.c
>>> +++ b/kernel/irq/manage.c
>>> @@ -715,9 +715,15 @@ again:
>>>  
>>>  	desc->threads_oneshot &= ~action->thread_mask;
>>>  
>>> -	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>>> -	    irqd_irq_masked(&desc->irq_data))
>>> -		unmask_irq(desc);
>>> +	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) {
>>> +#ifdef CONFIG_IPIPE
>>> +		if (desc->irq_data.chip->irq_release)
>>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>>> +		else 
>>> +#endif
>>> +			if (irqd_irq_masked(&desc->irq_data))
>>> +				unmask_irq(desc);
>>> +	}
>>>  
>>>  out_unlock:
>>>  	raw_spin_unlock_irq(&desc->lock);
>>>
>>>
>>
>> So this basically extends I-pipe's held phase of the IRQ to the threaded
>> handler if we are working in oneshot mode, right?
>>
>> I will have to dig into this again, but without remembering all details,
>> there are some things that do not look OK:
>>  - Shouldn't we just replace unmask_irq with irq_release for the I-pipe
>>    case? IOW: There is too much code under #ifdef CONFIG_IPIPE.
> 
> 
> No, unmask_irq still has to use ->irq_unmask, while the masking done by
> the I-pipe has to use ->irq_release.

No, I mean that unmask should be replaced by release, but we must not
apply different conditions on the unmask, at least the logical one. If
Linux wants the line to remain masked, we have to respect this. That is
currently (also) broken after the last refactoring.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 03/27/2013 10:30 AM, Jan Kiszka wrote:

> On 2013-03-27 10:23, Gilles Chanteperdrix wrote:
>> On 03/27/2013 07:50 AM, Jan Kiszka wrote:
>>
>>> On 2013-03-27 00:05, Gilles Chanteperdrix wrote:
>>>>
>>>> Hi Jan,
>>>>
>>>> I seem to recall you sent patches to fix threaded irqs some time ago. I 
>>>
>>> In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for
>>> 2.6.38 back then.
>>>
>>>> am having a problem here without SMP, whereas the system works with 
>>>> SMP. A fasteoi irq handler triggers repeatedly while its threaded 
>>>> counterpart never gets triggered. I believe this is because 
>>>> handle_fasteoi_irq unconditionally releases the irq line.
>>>
>>> You mean it doesn't respect the conditions in cond_unmask_irq?
>>>
>>>> The following 
>>>> patch seems to fix the issue though I would prefer a cleaner solution.
>>>>
>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>>> index 11e75d1..2ff8d3a 100644
>>>> --- a/kernel/irq/chip.c
>>>> +++ b/kernel/irq/chip.c
>>>> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>>>  
>>>>  #ifdef CONFIG_IPIPE
>>>>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>>> -	if (desc->irq_data.chip->irq_release)
>>>> -		desc->irq_data.chip->irq_release(&desc->irq_data);
>>>> +	if (desc->irq_data.chip->irq_release) {
>>>> +		if ((!(desc->istate & IRQS_ONESHOT) ||
>>>> +		     !desc->threads_oneshot))
>>>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>>>> +	}
>>>>  out_eoi:
>>>>  #else  /* !CONFIG_IPIPE */
>>>>  	if (desc->istate & IRQS_ONESHOT)
>>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>>> index e49a288..485c2c4 100644
>>>> --- a/kernel/irq/manage.c
>>>> +++ b/kernel/irq/manage.c
>>>> @@ -715,9 +715,15 @@ again:
>>>>  
>>>>  	desc->threads_oneshot &= ~action->thread_mask;
>>>>  
>>>> -	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>>>> -	    irqd_irq_masked(&desc->irq_data))
>>>> -		unmask_irq(desc);
>>>> +	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) {
>>>> +#ifdef CONFIG_IPIPE
>>>> +		if (desc->irq_data.chip->irq_release)
>>>> +			desc->irq_data.chip->irq_release(&desc->irq_data);
>>>> +		else 
>>>> +#endif
>>>> +			if (irqd_irq_masked(&desc->irq_data))
>>>> +				unmask_irq(desc);
>>>> +	}
>>>>  
>>>>  out_unlock:
>>>>  	raw_spin_unlock_irq(&desc->lock);
>>>>
>>>>
>>>
>>> So this basically extends I-pipe's held phase of the IRQ to the threaded
>>> handler if we are working in oneshot mode, right?
>>>
>>> I will have to dig into this again, but without remembering all details,
>>> there are some things that do not look OK:
>>>  - Shouldn't we just replace unmask_irq with irq_release for the I-pipe
>>>    case? IOW: There is too much code under #ifdef CONFIG_IPIPE.
>>
>>
>> No, unmask_irq still has to use ->irq_unmask, while the masking done by
>> the I-pipe has to use ->irq_release.
> 
> No, I mean that unmask should be replaced by release, but we must not
> apply different conditions on the unmask, at least the logical one. If
> Linux wants the line to remain masked, we have to respect this. That is
> currently (also) broken after the last refactoring.


Ok, how about this one:

iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 11e75d1..47d1d27 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc)
 void unmask_irq(struct irq_desc *desc)
 {
 	unsigned long flags;
-
+	
+	flags = hard_cond_local_irq_save();
+#ifdef CONFIG_IPIPE
+	if (desc->irq_data.chip->irq_release) {
+		desc->irq_data.chip->irq_release(&desc->irq_data);
+		irq_state_clr_masked(desc);
+	} else
+#endif /* CONFIG_IPIPE */
 	if (desc->irq_data.chip->irq_unmask) {
-		flags = hard_cond_local_irq_save();
 		desc->irq_data.chip->irq_unmask(&desc->irq_data);
 		irq_state_clr_masked(desc);
-		hard_cond_local_irq_restore(flags);
 	}
+	hard_cond_local_irq_restore(flags);
 }
 
 /*
@@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 
 #ifdef CONFIG_IPIPE
 	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
-	if (desc->irq_data.chip->irq_release)
-		desc->irq_data.chip->irq_release(&desc->irq_data);
+	cond_unmask_irq(desc);
 out_eoi:
 #else  /* !CONFIG_IPIPE */
 	if (desc->istate & IRQS_ONESHOT)
@@ -682,12 +687,15 @@ void __ipipe_end_level_irq(unsigned irq, struct irq_desc *desc)
 void __ipipe_ack_fasteoi_irq(unsigned irq, struct irq_desc *desc)
 {
 	desc->irq_data.chip->irq_hold(&desc->irq_data);
+	irq_state_set_masked(desc);	
 }
 
 void __ipipe_end_fasteoi_irq(unsigned irq, struct irq_desc *desc)
 {
-	if (desc->irq_data.chip->irq_release)
+	if (desc->irq_data.chip->irq_release) {
+		irq_state_clr_masked(desc);		
 		desc->irq_data.chip->irq_release(&desc->irq_data);
+	}
 }
 
 void __ipipe_ack_edge_irq(unsigned irq, struct irq_desc *desc)



-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Jan Kiszka]

On 2013-03-27 13:55, Gilles Chanteperdrix wrote:
> Ok, how about this one:
> 
> iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 11e75d1..47d1d27 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc)
>  void unmask_irq(struct irq_desc *desc)
>  {
>  	unsigned long flags;
> -
> +	
> +	flags = hard_cond_local_irq_save();
> +#ifdef CONFIG_IPIPE
> +	if (desc->irq_data.chip->irq_release) {
> +		desc->irq_data.chip->irq_release(&desc->irq_data);
> +		irq_state_clr_masked(desc);

Are you sure that every call to unmask_irq implies (under I-pipe) that
the IRQ was held before so that release_irq is appropriate here? Or did
you rather want to patch cond_unmask_irq this way? But that has
unfortunately also more users than our fast_eoi path.

> +	} else
> +#endif /* CONFIG_IPIPE */
>  	if (desc->irq_data.chip->irq_unmask) {
> -		flags = hard_cond_local_irq_save();
>  		desc->irq_data.chip->irq_unmask(&desc->irq_data);
>  		irq_state_clr_masked(desc);
> -		hard_cond_local_irq_restore(flags);
>  	}
> +	hard_cond_local_irq_restore(flags);
>  }
>  
>  /*
> @@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>  
>  #ifdef CONFIG_IPIPE
>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
> -	if (desc->irq_data.chip->irq_release)
> -		desc->irq_data.chip->irq_release(&desc->irq_data);
> +	cond_unmask_irq(desc);

I'm wondering now why we need this differently for the I-pipe case.

Let's revisit what happens with a fasteoi normally:

- it's masked only if it's a oneshot IRQ before calling the flow handler
- it's unmasked after the flow handling if the thread was not woken up

With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
conditions and spots to unmask are:
- from handle_fasteoi_irq if the thread wasn't woken up or we have
  non-threaded or non-oneshot handling
- otherwise on end_irq from the handler thread

Do you think this is correct? If so, I do not think it matches this
patch yet.

Jan

>  out_eoi:
>  #else  /* !CONFIG_IPIPE */
>  	if (desc->istate & IRQS_ONESHOT)
> @@ -682,12 +687,15 @@ void __ipipe_end_level_irq(unsigned irq, struct irq_desc *desc)
>  void __ipipe_ack_fasteoi_irq(unsigned irq, struct irq_desc *desc)
>  {
>  	desc->irq_data.chip->irq_hold(&desc->irq_data);
> +	irq_state_set_masked(desc);	
>  }
>  
>  void __ipipe_end_fasteoi_irq(unsigned irq, struct irq_desc *desc)
>  {
> -	if (desc->irq_data.chip->irq_release)
> +	if (desc->irq_data.chip->irq_release) {
> +		irq_state_clr_masked(desc);		
>  		desc->irq_data.chip->irq_release(&desc->irq_data);
> +	}
>  }
>  
>  void __ipipe_ack_edge_irq(unsigned irq, struct irq_desc *desc)
> 
> 
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 03/27/2013 02:30 PM, Jan Kiszka wrote:

> On 2013-03-27 13:55, Gilles Chanteperdrix wrote:
>> Ok, how about this one:
>>
>> iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 11e75d1..47d1d27 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc)
>>  void unmask_irq(struct irq_desc *desc)
>>  {
>>  	unsigned long flags;
>> -
>> +	
>> +	flags = hard_cond_local_irq_save();
>> +#ifdef CONFIG_IPIPE
>> +	if (desc->irq_data.chip->irq_release) {
>> +		desc->irq_data.chip->irq_release(&desc->irq_data);
>> +		irq_state_clr_masked(desc);
> 
> Are you sure that every call to unmask_irq implies (under I-pipe) that
> the IRQ was held before so that release_irq is appropriate here?


Yes, unmask_irq is always called after testing for irqd_irq_masked, so,
since ipipe_ack_fasteoi_irq is modified for setting the corresponding
bit, and ipipe_end_fasteoi_irq clearing it, if the bit is set, the irq
can be unmasked.

> Or did
> you rather want to patch cond_unmask_irq this way? But that has
> unfortunately also more users than our fast_eoi path.

No, the reason unmask_irq has to be changed that way is because
irq_finalize_oneshot, which does not know whether it is dealing with a
fasteoi, level, edge, irq, calls unmask_irq unconditionnaly, so, if we
want irq_release to be called from that point for fasteoi irqs, we have
to change unmask_irq.

Alternatively, we could call ->ipipe_end with a #ifdef CONFIG_IPIPE in
irq_finalize_oneshot..

> 
>> +	} else
>> +#endif /* CONFIG_IPIPE */
>>  	if (desc->irq_data.chip->irq_unmask) {
>> -		flags = hard_cond_local_irq_save();
>>  		desc->irq_data.chip->irq_unmask(&desc->irq_data);
>>  		irq_state_clr_masked(desc);
>> -		hard_cond_local_irq_restore(flags);
>>  	}
>> +	hard_cond_local_irq_restore(flags);
>>  }
>>  
>>  /*
>> @@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>  
>>  #ifdef CONFIG_IPIPE
>>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>> -	if (desc->irq_data.chip->irq_release)
>> -		desc->irq_data.chip->irq_release(&desc->irq_data);
>> +	cond_unmask_irq(desc);
> 
> I'm wondering now why we need this differently for the I-pipe case.


In the non I-pipe case, we have:
	if (desc->istate & IRQS_ONESHOT)
		cond_unmask_irq(desc);
Which is wrong with I-pipe because we want the irq unmasked even for
irqs which are not one-host.

Then:
	desc->irq_data.chip->irq_eoi(&desc->irq_data);
Which is wrong because the EOI was already sent as part of the irq_hold
callback.

> 
> Let's revisit what happens with a fasteoi normally:
> 
> - it's masked only if it's a oneshot IRQ before calling the flow handler
> - it's unmasked after the flow handling if the thread was not woken up
> 
> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
> conditions and spots to unmask are:
> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>   non-threaded or non-oneshot handling


That is what the unconditional call to cond_unmask_irq does.

> - otherwise on end_irq from the handler thread


That is what the call to unmask_irq at the end of irq_finalize_oneshot does.

> 
> Do you think this is correct? If so, I do not think it matches this

> patch yet.


Seems to me that it does (and indeed the patched kernel works for me, I
tested before sending the patch).

-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 03/27/2013 02:30 PM, Jan Kiszka wrote:

> I'm wondering now why we need this differently for the I-pipe case.
> 
> Let's revisit what happens with a fasteoi normally:
> 
> - it's masked only if it's a oneshot IRQ before calling the flow handler
> - it's unmasked after the flow handling if the thread was not woken up
> 
> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
> conditions and spots to unmask are:
> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>   non-threaded or non-oneshot handling
> - otherwise on end_irq from the handler thread
> 
> Do you think this is correct? If so, I do not think it matches this
> patch yet.


Hi,

here is a much simpler patch:
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 11e75d1..a1c9918 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc)
 static inline void preflow_handler(struct irq_desc *desc) { }
 #endif
 
+static void cond_release_fasteoi_irq(struct irq_desc *desc)
+{
+	if (desc->irq_data.chip->irq_release && 
+	    !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
+		desc->irq_data.chip->irq_release(&desc->irq_data);
+}
+
 /**
  *	handle_fasteoi_irq - irq handler for transparent controllers
  *	@irq:	the interrupt number
@@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 
 #ifdef CONFIG_IPIPE
 	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
-	if (desc->irq_data.chip->irq_release)
-		desc->irq_data.chip->irq_release(&desc->irq_data);
+	cond_release_fasteoi_irq(desc);
 out_eoi:
 #else  /* !CONFIG_IPIPE */
 	if (desc->istate & IRQS_ONESHOT)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index e49a288..e7ae8bd 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -715,9 +715,14 @@ again:
 
 	desc->threads_oneshot &= ~action->thread_mask;
 
+#ifndef CONFIG_IPIPE
 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
 	    irqd_irq_masked(&desc->irq_data))
 		unmask_irq(desc);
+#else /* CONFIG_IPIPE */
+	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data))
+		desc->ipipe_end(desc->irq_data.irq, desc);
+#endif /* CONFIG_IPIPE */
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);


-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Jan Kiszka]

On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
> 
>> I'm wondering now why we need this differently for the I-pipe case.
>>
>> Let's revisit what happens with a fasteoi normally:
>>
>> - it's masked only if it's a oneshot IRQ before calling the flow handler
>> - it's unmasked after the flow handling if the thread was not woken up
>>
>> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
>> conditions and spots to unmask are:
>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>   non-threaded or non-oneshot handling
>> - otherwise on end_irq from the handler thread
>>
>> Do you think this is correct? If so, I do not think it matches this
>> patch yet.
> 
> 
> Hi,
> 
> here is a much simpler patch:
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 11e75d1..a1c9918 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc)
>  static inline void preflow_handler(struct irq_desc *desc) { }
>  #endif
>  
> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
> +{
> +	if (desc->irq_data.chip->irq_release && 
> +	    !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
> +		desc->irq_data.chip->irq_release(&desc->irq_data);
> +}
> +
>  /**
>   *	handle_fasteoi_irq - irq handler for transparent controllers
>   *	@irq:	the interrupt number
> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>  
>  #ifdef CONFIG_IPIPE
>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */

Makes me wonder what this comment wants to tell us. That there is an
unhandled corner case or that this is intentionally ignored? I support
the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
you can clarify that line at this chance.

> -	if (desc->irq_data.chip->irq_release)
> -		desc->irq_data.chip->irq_release(&desc->irq_data);
> +	cond_release_fasteoi_irq(desc);
>  out_eoi:
>  #else  /* !CONFIG_IPIPE */
>  	if (desc->istate & IRQS_ONESHOT)
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index e49a288..e7ae8bd 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -715,9 +715,14 @@ again:
>  
>  	desc->threads_oneshot &= ~action->thread_mask;
>  
> +#ifndef CONFIG_IPIPE
>  	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>  	    irqd_irq_masked(&desc->irq_data))
>  		unmask_irq(desc);
> +#else /* CONFIG_IPIPE */
> +	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data))
> +		desc->ipipe_end(desc->irq_data.irq, desc);
> +#endif /* CONFIG_IPIPE */
>  
>  out_unlock:
>  	raw_spin_unlock_irq(&desc->lock);
> 
> 

Looks good. I should dig out our test case again and check it on x86. I
think it was PCI device assignment with KVM where the IRQ of the
assigned device is handled by an IRQ thread of KVM.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130406/ef69ff66/attachment.pgp>

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 04/06/2013 11:43 AM, Jan Kiszka wrote:

> On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
>> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
>>
>>> I'm wondering now why we need this differently for the I-pipe case.
>>>
>>> Let's revisit what happens with a fasteoi normally:
>>>
>>> - it's masked only if it's a oneshot IRQ before calling the flow handler
>>> - it's unmasked after the flow handling if the thread was not woken up
>>>
>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
>>> conditions and spots to unmask are:
>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>>   non-threaded or non-oneshot handling
>>> - otherwise on end_irq from the handler thread
>>>
>>> Do you think this is correct? If so, I do not think it matches this
>>> patch yet.
>>
>>
>> Hi,
>>
>> here is a much simpler patch:
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 11e75d1..a1c9918 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc)
>>  static inline void preflow_handler(struct irq_desc *desc) { }
>>  #endif
>>  
>> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
>> +{
>> +	if (desc->irq_data.chip->irq_release && 
>> +	    !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
>> +		desc->irq_data.chip->irq_release(&desc->irq_data);
>> +}
>> +
>>  /**
>>   *	handle_fasteoi_irq - irq handler for transparent controllers
>>   *	@irq:	the interrupt number
>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>  
>>  #ifdef CONFIG_IPIPE
>>  	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
> 
> Makes me wonder what this comment wants to tell us. That there is an
> unhandled corner case or that this is intentionally ignored? I support
> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
> you can clarify that line at this chance.


I have no idea where this comment come from. I do not think this flag
can be handled with the I-pipe kernel, as the EOI is sent before trying
to handle the IRQ. The only users are in arch/sparc and arch/powerpc,
maybe Philippe knows more?

-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Philippe Gerum]

On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote:
> On 04/06/2013 11:43 AM, Jan Kiszka wrote:
>
>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
>>>
>>>> I'm wondering now why we need this differently for the I-pipe case.
>>>>
>>>> Let's revisit what happens with a fasteoi normally:
>>>>
>>>> - it's masked only if it's a oneshot IRQ before calling the flow handler
>>>> - it's unmasked after the flow handling if the thread was not woken up
>>>>
>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The
>>>> conditions and spots to unmask are:
>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>>>    non-threaded or non-oneshot handling
>>>> - otherwise on end_irq from the handler thread
>>>>
>>>> Do you think this is correct? If so, I do not think it matches this
>>>> patch yet.
>>>
>>>
>>> Hi,
>>>
>>> here is a much simpler patch:
>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>> index 11e75d1..a1c9918 100644
>>> --- a/kernel/irq/chip.c
>>> +++ b/kernel/irq/chip.c
>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc)
>>>   static inline void preflow_handler(struct irq_desc *desc) { }
>>>   #endif
>>>
>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
>>> +{
>>> +	if (desc->irq_data.chip->irq_release &&
>>> +	    !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
>>> +		desc->irq_data.chip->irq_release(&desc->irq_data);
>>> +}
>>> +
>>>   /**
>>>    *	handle_fasteoi_irq - irq handler for transparent controllers
>>>    *	@irq:	the interrupt number
>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
>>>
>>>   #ifdef CONFIG_IPIPE
>>>   	/* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>
>> Makes me wonder what this comment wants to tell us. That there is an
>> unhandled corner case or that this is intentionally ignored? I support
>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
>> you can clarify that line at this chance.
>
>
> I have no idea where this comment come from. I do not think this flag
> can be handled with the I-pipe kernel, as the EOI is sent before trying
> to handle the IRQ. The only users are in arch/sparc and arch/powerpc,
> maybe Philippe knows more?
>

Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the 
regular flow handler, as a consequence of doing eoi+mask when holding 
the IRQ in the ipipe's flow handler.

-- 
Philippe.

Re: [Xenomai] one-shot fasteoi irqs

[Jan Kiszka]

On 2013-04-10 11:32, Philippe Gerum wrote:
> On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote:
>> On 04/06/2013 11:43 AM, Jan Kiszka wrote:
>>
>>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
>>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
>>>>
>>>>> I'm wondering now why we need this differently for the I-pipe case.
>>>>>
>>>>> Let's revisit what happens with a fasteoi normally:
>>>>>
>>>>> - it's masked only if it's a oneshot IRQ before calling the flow
>>>>> handler
>>>>> - it's unmasked after the flow handling if the thread was not woken up
>>>>>
>>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ
>>>>> masked. The
>>>>> conditions and spots to unmask are:
>>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>>>>    non-threaded or non-oneshot handling
>>>>> - otherwise on end_irq from the handler thread
>>>>>
>>>>> Do you think this is correct? If so, I do not think it matches this
>>>>> patch yet.
>>>>
>>>>
>>>> Hi,
>>>>
>>>> here is a much simpler patch:
>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>>> index 11e75d1..a1c9918 100644
>>>> --- a/kernel/irq/chip.c
>>>> +++ b/kernel/irq/chip.c
>>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct
>>>> irq_desc *desc)
>>>>   static inline void preflow_handler(struct irq_desc *desc) { }
>>>>   #endif
>>>>
>>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
>>>> +{
>>>> +    if (desc->irq_data.chip->irq_release &&
>>>> +        !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
>>>> +        desc->irq_data.chip->irq_release(&desc->irq_data);
>>>> +}
>>>> +
>>>>   /**
>>>>    *    handle_fasteoi_irq - irq handler for transparent controllers
>>>>    *    @irq:    the interrupt number
>>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct
>>>> irq_desc *desc)
>>>>
>>>>   #ifdef CONFIG_IPIPE
>>>>       /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>>
>>> Makes me wonder what this comment wants to tell us. That there is an
>>> unhandled corner case or that this is intentionally ignored? I support
>>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
>>> you can clarify that line at this chance.
>>
>>
>> I have no idea where this comment come from. I do not think this flag
>> can be handled with the I-pipe kernel, as the EOI is sent before trying
>> to handle the IRQ. The only users are in arch/sparc and arch/powerpc,
>> maybe Philippe knows more?
>>
> 
> Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the
> regular flow handler, as a consequence of doing eoi+mask when holding
> the IRQ in the ipipe's flow handler.

OK, then I'll write a clarification patch for that comment - to get rid
of that triple X...

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix]

On 04/10/2013 12:36 PM, Jan Kiszka wrote:

> On 2013-04-10 11:32, Philippe Gerum wrote:
>> On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote:
>>> On 04/06/2013 11:43 AM, Jan Kiszka wrote:
>>>
>>>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
>>>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
>>>>>
>>>>>> I'm wondering now why we need this differently for the I-pipe case.
>>>>>>
>>>>>> Let's revisit what happens with a fasteoi normally:
>>>>>>
>>>>>> - it's masked only if it's a oneshot IRQ before calling the flow
>>>>>> handler
>>>>>> - it's unmasked after the flow handling if the thread was not woken up
>>>>>>
>>>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ
>>>>>> masked. The
>>>>>> conditions and spots to unmask are:
>>>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>>>>>    non-threaded or non-oneshot handling
>>>>>> - otherwise on end_irq from the handler thread
>>>>>>
>>>>>> Do you think this is correct? If so, I do not think it matches this
>>>>>> patch yet.
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> here is a much simpler patch:
>>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>>>> index 11e75d1..a1c9918 100644
>>>>> --- a/kernel/irq/chip.c
>>>>> +++ b/kernel/irq/chip.c
>>>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct
>>>>> irq_desc *desc)
>>>>>   static inline void preflow_handler(struct irq_desc *desc) { }
>>>>>   #endif
>>>>>
>>>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
>>>>> +{
>>>>> +    if (desc->irq_data.chip->irq_release &&
>>>>> +        !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
>>>>> +        desc->irq_data.chip->irq_release(&desc->irq_data);
>>>>> +}
>>>>> +
>>>>>   /**
>>>>>    *    handle_fasteoi_irq - irq handler for transparent controllers
>>>>>    *    @irq:    the interrupt number
>>>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct
>>>>> irq_desc *desc)
>>>>>
>>>>>   #ifdef CONFIG_IPIPE
>>>>>       /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>>>
>>>> Makes me wonder what this comment wants to tell us. That there is an
>>>> unhandled corner case or that this is intentionally ignored? I support
>>>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
>>>> you can clarify that line at this chance.
>>>
>>>
>>> I have no idea where this comment come from. I do not think this flag
>>> can be handled with the I-pipe kernel, as the EOI is sent before trying
>>> to handle the IRQ. The only users are in arch/sparc and arch/powerpc,
>>> maybe Philippe knows more?
>>>
>>
>> Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the
>> regular flow handler, as a consequence of doing eoi+mask when holding
>> the IRQ in the ipipe's flow handler.
> 
> OK, then I'll write a clarification patch for that comment - to get rid
> of that triple X...


I understand the flag with a negative sense: do not eoi the interrupt if
the handler reports that it has not handled it. Obviously, if that is
what the flag means, we can not honour it when the I-pipe is on.

> 
> Jan
> 



-- 
                                                                Gilles.

Re: [Xenomai] one-shot fasteoi irqs

[Philippe Gerum]

On 04/10/2013 09:36 PM, Gilles Chanteperdrix wrote:
> On 04/10/2013 12:36 PM, Jan Kiszka wrote:
>
>> On 2013-04-10 11:32, Philippe Gerum wrote:
>>> On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote:
>>>> On 04/06/2013 11:43 AM, Jan Kiszka wrote:
>>>>
>>>>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote:
>>>>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote:
>>>>>>
>>>>>>> I'm wondering now why we need this differently for the I-pipe case.
>>>>>>>
>>>>>>> Let's revisit what happens with a fasteoi normally:
>>>>>>>
>>>>>>> - it's masked only if it's a oneshot IRQ before calling the flow
>>>>>>> handler
>>>>>>> - it's unmasked after the flow handling if the thread was not woken up
>>>>>>>
>>>>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ
>>>>>>> masked. The
>>>>>>> conditions and spots to unmask are:
>>>>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have
>>>>>>>     non-threaded or non-oneshot handling
>>>>>>> - otherwise on end_irq from the handler thread
>>>>>>>
>>>>>>> Do you think this is correct? If so, I do not think it matches this
>>>>>>> patch yet.
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> here is a much simpler patch:
>>>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>>>>> index 11e75d1..a1c9918 100644
>>>>>> --- a/kernel/irq/chip.c
>>>>>> +++ b/kernel/irq/chip.c
>>>>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct
>>>>>> irq_desc *desc)
>>>>>>    static inline void preflow_handler(struct irq_desc *desc) { }
>>>>>>    #endif
>>>>>>
>>>>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc)
>>>>>> +{
>>>>>> +    if (desc->irq_data.chip->irq_release &&
>>>>>> +        !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot)
>>>>>> +        desc->irq_data.chip->irq_release(&desc->irq_data);
>>>>>> +}
>>>>>> +
>>>>>>    /**
>>>>>>     *    handle_fasteoi_irq - irq handler for transparent controllers
>>>>>>     *    @irq:    the interrupt number
>>>>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct
>>>>>> irq_desc *desc)
>>>>>>
>>>>>>    #ifdef CONFIG_IPIPE
>>>>>>        /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */
>>>>>
>>>>> Makes me wonder what this comment wants to tell us. That there is an
>>>>> unhandled corner case or that this is intentionally ignored? I support
>>>>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe
>>>>> you can clarify that line at this chance.
>>>>
>>>>
>>>> I have no idea where this comment come from. I do not think this flag
>>>> can be handled with the I-pipe kernel, as the EOI is sent before trying
>>>> to handle the IRQ. The only users are in arch/sparc and arch/powerpc,
>>>> maybe Philippe knows more?
>>>>
>>>
>>> Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the
>>> regular flow handler, as a consequence of doing eoi+mask when holding
>>> the IRQ in the ipipe's flow handler.
>>
>> OK, then I'll write a clarification patch for that comment - to get rid
>> of that triple X...
>
>
> I understand the flag with a negative sense: do not eoi the interrupt if
> the handler reports that it has not handled it. Obviously, if that is
> what the flag means, we can not honour it when the I-pipe is on.
>

Which is the same as saying that we shall ignore EOI_IF_HANDLED because 
it does not apply when interrupts are pipelined, yes. Hence the comment.

-- 
Philippe.