Re: [Xenomai] one-shot fasteoi irqs

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

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.