[1/3] genirq: Add support for priority-drop/deactivate interrupt controllers

Moderately recent ARM interrupt controllers can use a "split mode" EOI,
where instead of just using a single write to notify the controller of
the end of interrupt, uses the following:
- priority-drop: the interrupt is still active, but other interrupts can
  now be taken
- deactivate: the interrupt is not active anymore, and can be taken again.

This makes it very useful for threaded interrupts, as it avoids the usual
mask/unmask dance (and has the potential of being more efficient on ARM,
as it is using the CPU interface instead of the global distributor).

To implement this, a new optional irqchip method is added (irq_priority_drop).
The usual irq_eoi is expected to implement the deactivate method.

Non threaded interrupts are using these two callbacks back to back, but threaded
ones only perform the irq_priority_drop call in the interrupt context, leaving
the irq_eoi call to the thread context (which are expected to use the
IRQCHIP_EOI_THREADED flag).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irq.h |  1 +
 kernel/irq/chip.c   | 53 +++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 38 insertions(+), 16 deletions(-)

On Sat, 25 Oct 2014, Marc Zyngier wrote:

@@ -330,6 +330,7 @@ struct irq_chip {
        void            (*irq_mask)(struct irq_data *data);
        void            (*irq_mask_ack)(struct irq_data *data);
        void            (*irq_unmask)(struct irq_data *data);
+       void            (*irq_priority_drop)(struct irq_data *data);

Lacks the docbook comment.

> +static void mask_threaded_irq(struct irq_desc *desc)

There is only one caller for this, i.e handle_fasteoi_irq, right? So
this should go to the other eoi handler specific helpers and have eoi
in its name.

> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	/* If we can do priority drop, then masking comes for free */
> +	if (chip->irq_priority_drop)
> +		irq_state_set_masked(desc);
> +	else
> +		mask_irq(desc);
> +}

>  void unmask_irq(struct irq_desc *desc)
>  {
> -	if (desc->irq_data.chip->irq_unmask) {
> -		desc->irq_data.chip->irq_unmask(&desc->irq_data);
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	if (chip->irq_unmask && !chip->irq_priority_drop)
> +		chip->irq_unmask(&desc->irq_data);

I have a hard time to understand that logic. Assume the interrupt
being masked at the hardware level after boot. Now at request_irq()
time what is going to unmask that very interrupt? Ditto for masking
after disable_irq(). Probably not what you really want.

> +static void eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
> +{
> +	if (chip->irq_priority_drop)
> +		chip->irq_priority_drop(&desc->irq_data);
> +	if (chip->irq_eoi)
> +		chip->irq_eoi(&desc->irq_data);
> +}

So if you are using that priority drop stuff, you need both calls even
for the non threaded case?

>  static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>  {
>  	if (!(desc->istate & IRQS_ONESHOT)) {
> -		chip->irq_eoi(&desc->irq_data);
> +		eoi_irq(desc, chip);
>  		return;
>  	}
> +
> +	if (chip->irq_priority_drop)
> +		chip->irq_priority_drop(&desc->irq_data);
> +
>  	/*
>  	 * We need to unmask in the following cases:
>  	 * - Oneshot irq which did not wake the thread (caused by a
> @@ -485,7 +507,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>  	if (!irqd_irq_disabled(&desc->irq_data) &&
>  	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
>  		chip->irq_eoi(&desc->irq_data);
> -		unmask_irq(desc);
> +		if (!chip->irq_priority_drop)
> +			unmask_irq(desc);

This is really completely obfuscated: Brain starts melting and
spiraling towards some unidentified universe.

Seriously, I don't think it's a good idea to bandaid this
functionality into the existing handle_fasteoi_irq() mechanism. It's
complex enough already.

So what you really want is a separate handler for this. But aside of
adding the drop prio callback you probably want to handle the other
existing callbacks completely differently than for the regular mode of
that irq controller.

Can you please explain detailed how this "priority drop" mode
works? 

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Sat, 25 Oct 2014, Thomas Gleixner wrote:
> Can you please explain detailed how this "priority drop" mode
> works? 

And I mean how it works from all aspects, not only from handling the
interrupt itself.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Thomas,

Thanks for looking into this.

On 25/10/14 21:27, Thomas Gleixner wrote:
> On Sat, 25 Oct 2014, Marc Zyngier wrote:
> 
> @@ -330,6 +330,7 @@ struct irq_chip {
>         void            (*irq_mask)(struct irq_data *data);
>         void            (*irq_mask_ack)(struct irq_data *data);
>         void            (*irq_unmask)(struct irq_data *data);
> +       void            (*irq_priority_drop)(struct irq_data *data);
> 
> Lacks the docbook comment.

Yup, will add.

>> +static void mask_threaded_irq(struct irq_desc *desc)
> 
> There is only one caller for this, i.e handle_fasteoi_irq, right? So
> this should go to the other eoi handler specific helpers and have eoi
> in its name.

I was seeing it as the pendent of unmask_threaded_irq(). But reading
below, you seem to have a very different approach

>> +{
>> +	struct irq_chip *chip = desc->irq_data.chip;
>> +
>> +	/* If we can do priority drop, then masking comes for free */
>> +	if (chip->irq_priority_drop)
>> +		irq_state_set_masked(desc);
>> +	else
>> +		mask_irq(desc);
>> +}
> 
>>  void unmask_irq(struct irq_desc *desc)
>>  {
>> -	if (desc->irq_data.chip->irq_unmask) {
>> -		desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> +	struct irq_chip *chip = desc->irq_data.chip;
>> +
>> +	if (chip->irq_unmask && !chip->irq_priority_drop)
>> +		chip->irq_unmask(&desc->irq_data);
> 
> I have a hard time to understand that logic. Assume the interrupt
> being masked at the hardware level after boot. Now at request_irq()
> time what is going to unmask that very interrupt? Ditto for masking
> after disable_irq(). Probably not what you really want.

Peering at the code (and assuming I'm finally awake), request_irq() uses
irq_startup() -> irq_enable() -> chip->irq_unmask().

But you're perfectly right, it breaks an independent use of
unmask_irq(), which is pretty bad.

>> +static void eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>> +{
>> +	if (chip->irq_priority_drop)
>> +		chip->irq_priority_drop(&desc->irq_data);
>> +	if (chip->irq_eoi)
>> +		chip->irq_eoi(&desc->irq_data);
>> +}
> 
> So if you are using that priority drop stuff, you need both calls even
> for the non threaded case?

Yes. This is a global property (all interrupt lines for this irqchip are
affected), so even the non-threaded case has to issue both calls.

>>  static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>>  {
>>  	if (!(desc->istate & IRQS_ONESHOT)) {
>> -		chip->irq_eoi(&desc->irq_data);
>> +		eoi_irq(desc, chip);
>>  		return;
>>  	}
>> +
>> +	if (chip->irq_priority_drop)
>> +		chip->irq_priority_drop(&desc->irq_data);
>> +
>>  	/*
>>  	 * We need to unmask in the following cases:
>>  	 * - Oneshot irq which did not wake the thread (caused by a
>> @@ -485,7 +507,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>>  	if (!irqd_irq_disabled(&desc->irq_data) &&
>>  	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
>>  		chip->irq_eoi(&desc->irq_data);
>> -		unmask_irq(desc);
>> +		if (!chip->irq_priority_drop)
>> +			unmask_irq(desc);
> 
> This is really completely obfuscated: Brain starts melting and
> spiraling towards some unidentified universe.

Ah! I'm glad I'm not the only one with that feeling ;-).

> Seriously, I don't think it's a good idea to bandaid this
> functionality into the existing handle_fasteoi_irq() mechanism. It's
> complex enough already.

That was the other option. I may have to duplicate (or tweak)
handle_percpu_devid_irq as well though.

> So what you really want is a separate handler for this. But aside of
> adding the drop prio callback you probably want to handle the other
> existing callbacks completely differently than for the regular mode of
> that irq controller.
> 
> Can you please explain detailed how this "priority drop" mode
> works? 

The basics of this mode are pretty simple:
- Interrupt signalled, CPU enter the GIC code
- Read the IAR register, interrupt becomes active:
  -> no other interrupt can be taken
- Run whatever interrupt handler
- Write to the EOI register:
  -> interrupt is still active, and cannot be taken again, but other
interrupts can now be taken
- Write to the DIR register:
  -> interrupt is now inactive, and can be taken again.

A few interesting things here:
- EOI (which causes priority drop) acts as a mask
- DIR (which causes deactivate) acts as unmask+EOI

To me, it looks like DIR operation is exactly what we need when running
a threaded interrupt with IRQCHIP_EOI_THREADED, saving the whole
mask/unmask that is rather slow on ARM.

With that in mind, I end up mapping mask to priority_drop_irq (write to
EOI), and unmask to eoi_irq (write to DIR). Which is admittedly an
interesting brainfuck when trying to wire it into the existing framework.

So yeah, having a different handler will make it much simpler. My main
concern is how to plug this "elegantly" into the epilogue for a threaded
interrupt (irq_finalize_oneshot).

Thanks,

	M.

On Mon, 27 Oct 2014, Marc Zyngier wrote:
> On 25/10/14 21:27, Thomas Gleixner wrote:
> > On Sat, 25 Oct 2014, Marc Zyngier wrote:
> >> +{
> >> +	struct irq_chip *chip = desc->irq_data.chip;
> >> +
> >> +	/* If we can do priority drop, then masking comes for free */
> >> +	if (chip->irq_priority_drop)
> >> +		irq_state_set_masked(desc);
> >> +	else
> >> +		mask_irq(desc);
> >> +}
> > 
> >>  void unmask_irq(struct irq_desc *desc)
> >>  {
> >> -	if (desc->irq_data.chip->irq_unmask) {
> >> -		desc->irq_data.chip->irq_unmask(&desc->irq_data);
> >> +	struct irq_chip *chip = desc->irq_data.chip;
> >> +
> >> +	if (chip->irq_unmask && !chip->irq_priority_drop)
> >> +		chip->irq_unmask(&desc->irq_data);
> > 
> > I have a hard time to understand that logic. Assume the interrupt
> > being masked at the hardware level after boot. Now at request_irq()
> > time what is going to unmask that very interrupt? Ditto for masking
> > after disable_irq(). Probably not what you really want.
> 
> Peering at the code (and assuming I'm finally awake), request_irq() uses
> irq_startup() -> irq_enable() -> chip->irq_unmask().

Right. That's the default implementation.

> But you're perfectly right, it breaks an independent use of
> unmask_irq(), which is pretty bad.

Indeed.
 
> >> +static void eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
> >> +{
> >> +	if (chip->irq_priority_drop)
> >> +		chip->irq_priority_drop(&desc->irq_data);
> >> +	if (chip->irq_eoi)
> >> +		chip->irq_eoi(&desc->irq_data);
> >> +}
> > 
> > So if you are using that priority drop stuff, you need both calls even
> > for the non threaded case?
> 
> Yes. This is a global property (all interrupt lines for this irqchip are
> affected), so even the non-threaded case has to issue both calls.

Ok.
 
> > Can you please explain detailed how this "priority drop" mode
> > works? 
> 
> The basics of this mode are pretty simple:
> - Interrupt signalled, CPU enter the GIC code
> - Read the IAR register, interrupt becomes active:
>   -> no other interrupt can be taken
> - Run whatever interrupt handler
> - Write to the EOI register:
>   -> interrupt is still active, and cannot be taken again, but other
> interrupts can now be taken
> - Write to the DIR register:
>   -> interrupt is now inactive, and can be taken again.
> 
> A few interesting things here:
> - EOI (which causes priority drop) acts as a mask
> - DIR (which causes deactivate) acts as unmask+EOI

Let me make a few assumptions and correct me if I'm wrong as usual.

1) The startup/shutdown procedure for such an interrupt is the
   expensive mask/unmask which you want to avoid for the actual
   handling case

2) In case of an actual interrupt the flow (ignoring locking) is:

   handle_xxx_irq()

	mask_irq();	/* chip->irq_mask() maps to EOI */

	if (!action || irq_disabled())
	   return;

	handle_actions();

	if (irq_threads_active() || irq_disabled())
	   return;

	unmask_irq();	/* chip->irq_unmask() maps to DIR */

  So that is handle_level_irq() with the chip callbacks being:

   irq_startup  = gic_unmask
   irq_shutdown = gic_mask
   irq_unmask	= gic_dir
   irq_mask	= gic_eoi

3) In the threaded case as seen above finalize_oneshot() will call
   chip->unmask_irq() which maps to the DIR write and gets things
   going again.

4) In the lazy irq disable case if the interrupt fires mask_irq()
   [EOI] is good enough to silence it.

   Though in the enable_irq() case you cannot rely on the automatic
   resend of the interrupt when you unmask [DIR]. So we need to make
   sure that even in the level case (dunno whether that's supported in
   that mode) we end up calling the irq_retrigger() callback. But
   that's rather simple to achieve with a new chip flag.

You might have to look at the suspend/resume implications, but if I
did not miss anything crucial that should be fine as well.

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 28/10/14 15:32, Thomas Gleixner wrote:
> On Mon, 27 Oct 2014, Marc Zyngier wrote:
>> On 25/10/14 21:27, Thomas Gleixner wrote:
>>> On Sat, 25 Oct 2014, Marc Zyngier wrote:
>>>> +{
>>>> +	struct irq_chip *chip = desc->irq_data.chip;
>>>> +
>>>> +	/* If we can do priority drop, then masking comes for free */
>>>> +	if (chip->irq_priority_drop)
>>>> +		irq_state_set_masked(desc);
>>>> +	else
>>>> +		mask_irq(desc);
>>>> +}
>>>
>>>>  void unmask_irq(struct irq_desc *desc)
>>>>  {
>>>> -	if (desc->irq_data.chip->irq_unmask) {
>>>> -		desc->irq_data.chip->irq_unmask(&desc->irq_data);
>>>> +	struct irq_chip *chip = desc->irq_data.chip;
>>>> +
>>>> +	if (chip->irq_unmask && !chip->irq_priority_drop)
>>>> +		chip->irq_unmask(&desc->irq_data);
>>>
>>> I have a hard time to understand that logic. Assume the interrupt
>>> being masked at the hardware level after boot. Now at request_irq()
>>> time what is going to unmask that very interrupt? Ditto for masking
>>> after disable_irq(). Probably not what you really want.
>>
>> Peering at the code (and assuming I'm finally awake), request_irq() uses
>> irq_startup() -> irq_enable() -> chip->irq_unmask().
> 
> Right. That's the default implementation.
> 
>> But you're perfectly right, it breaks an independent use of
>> unmask_irq(), which is pretty bad.
> 
> Indeed.
>  
>>>> +static void eoi_irq(struct irq_desc *desc, struct irq_chip *chip)
>>>> +{
>>>> +	if (chip->irq_priority_drop)
>>>> +		chip->irq_priority_drop(&desc->irq_data);
>>>> +	if (chip->irq_eoi)
>>>> +		chip->irq_eoi(&desc->irq_data);
>>>> +}
>>>
>>> So if you are using that priority drop stuff, you need both calls even
>>> for the non threaded case?
>>
>> Yes. This is a global property (all interrupt lines for this irqchip are
>> affected), so even the non-threaded case has to issue both calls.
> 
> Ok.
>  
>>> Can you please explain detailed how this "priority drop" mode
>>> works? 
>>
>> The basics of this mode are pretty simple:
>> - Interrupt signalled, CPU enter the GIC code
>> - Read the IAR register, interrupt becomes active:
>>   -> no other interrupt can be taken
>> - Run whatever interrupt handler
>> - Write to the EOI register:
>>   -> interrupt is still active, and cannot be taken again, but other
>> interrupts can now be taken
>> - Write to the DIR register:
>>   -> interrupt is now inactive, and can be taken again.
>>
>> A few interesting things here:
>> - EOI (which causes priority drop) acts as a mask
>> - DIR (which causes deactivate) acts as unmask+EOI
> 
> Let me make a few assumptions and correct me if I'm wrong as usual.
> 
> 1) The startup/shutdown procedure for such an interrupt is the
>    expensive mask/unmask which you want to avoid for the actual
>    handling case

Indeed.

> 2) In case of an actual interrupt the flow (ignoring locking) is:
> 
>    handle_xxx_irq()
> 
> 	mask_irq();	/* chip->irq_mask() maps to EOI */
> 
> 	if (!action || irq_disabled())
> 	   return;
> 
> 	handle_actions();
> 
> 	if (irq_threads_active() || irq_disabled())
> 	   return;
> 
> 	unmask_irq();	/* chip->irq_unmask() maps to DIR */
> 
>   So that is handle_level_irq() with the chip callbacks being:
> 
>    irq_startup  = gic_unmask
>    irq_shutdown = gic_mask
>    irq_unmask	= gic_dir
>    irq_mask	= gic_eoi

So while this works really well for the interrupt handling part, it will
break [un]mask_irq(). This is because you can only write to EOI for an
interrupt that you have ACKed just before (anything else and the GIC
state machine goes crazy). Basically, any use for EOI/DIR outside of the
interrupt context itself (hardirq or thread) is really dangerous.

If we had a flag like IRQCHIP_UNMASK_IS_STARTUP, we could distinguish
this particular case, but that's borderline ugly.

> 3) In the threaded case as seen above finalize_oneshot() will call
>    chip->unmask_irq() which maps to the DIR write and gets things
>    going again.

Yup.

> 4) In the lazy irq disable case if the interrupt fires mask_irq()
>    [EOI] is good enough to silence it.
> 
>    Though in the enable_irq() case you cannot rely on the automatic
>    resend of the interrupt when you unmask [DIR]. So we need to make
>    sure that even in the level case (dunno whether that's supported in
>    that mode) we end up calling the irq_retrigger() callback. But
>    that's rather simple to achieve with a new chip flag.

I think this one breaks for the same reason as above. And an interrupt
masked with EOI cannot easily be restarted without clearing the ACTIVE
bit (and everything becomes even more of a complete madness).

I need to think about it again.

Thanks,

	M.

On Tue, 28 Oct 2014, Marc Zyngier wrote:
> On 28/10/14 15:32, Thomas Gleixner wrote:
> > Let me make a few assumptions and correct me if I'm wrong as usual.
> > 
> > 1) The startup/shutdown procedure for such an interrupt is the
> >    expensive mask/unmask which you want to avoid for the actual
> >    handling case
> 
> Indeed.
> 
> > 2) In case of an actual interrupt the flow (ignoring locking) is:
> > 
> >    handle_xxx_irq()
> > 
> > 	mask_irq();	/* chip->irq_mask() maps to EOI */
> > 
> > 	if (!action || irq_disabled())
> > 	   return;
> > 
> > 	handle_actions();
> > 
> > 	if (irq_threads_active() || irq_disabled())
> > 	   return;
> > 
> > 	unmask_irq();	/* chip->irq_unmask() maps to DIR */
> > 
> >   So that is handle_level_irq() with the chip callbacks being:
> > 
> >    irq_startup  = gic_unmask
> >    irq_shutdown = gic_mask
> >    irq_unmask	= gic_dir
> >    irq_mask	= gic_eoi
> 
> So while this works really well for the interrupt handling part, it will
> break [un]mask_irq(). This is because you can only write to EOI for an
> interrupt that you have ACKed just before (anything else and the GIC
> state machine goes crazy). Basically, any use for EOI/DIR outside of the
> interrupt context itself (hardirq or thread) is really dangerous.

I really doubt that the DIR invocation is dangerous outside of
interrupt context. Otherwise your threaded scheme would not work at
all as the DIR invocation happens in thread context.

The nice thing about the lazy irq disable code is that the irq_mask(),
i.e. EOI, invocation actually happens always in hard interrupt
context. We should never invoke irq_mask() from any other context if
you supply a startup/shutdown function.

> If we had a flag like IRQCHIP_UNMASK_IS_STARTUP, we could distinguish
> this particular case, but that's borderline ugly.

Indeed. But I don't think it is required. See also below.

> > 4) In the lazy irq disable case if the interrupt fires mask_irq()
> >    [EOI] is good enough to silence it.
> > 
> >    Though in the enable_irq() case you cannot rely on the automatic
> >    resend of the interrupt when you unmask [DIR]. So we need to make
> >    sure that even in the level case (dunno whether that's supported in
> >    that mode) we end up calling the irq_retrigger() callback. But
> >    that's rather simple to achieve with a new chip flag.
> 
> I think this one breaks for the same reason as above. And an interrupt
> masked with EOI cannot easily be restarted without clearing the ACTIVE
> bit (and everything becomes even more of a complete madness).

So we already established that irq_mask()/EOI will only be called from
the actual interrupt context and irq_unmask()/DIR must be safe to be
called from any context in order to make the EOI/DIR based threaded
optimization work.

So the only interesting code path is enable_irq() which invokes
irq_enable() and then the resend/retrigger machinery. 

irq_enable() calls chip->irq_unmask(), i.e. DIR. So that clears the
ACTIVE bit and then the IRQ either gets resent by hardware (in case of
level as the device interrupt is still active) or retriggered by the
irq_retrigger() callback.

I might be wrong as usual, but if there is any restriction on DIR
versus the invocation context, your whole optimization scheme is hosed
anyway.

Thanks

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 28/10/14 20:14, Thomas Gleixner wrote:
> On Tue, 28 Oct 2014, Marc Zyngier wrote:
>> On 28/10/14 15:32, Thomas Gleixner wrote:
>>> Let me make a few assumptions and correct me if I'm wrong as usual.
>>>
>>> 1) The startup/shutdown procedure for such an interrupt is the
>>>    expensive mask/unmask which you want to avoid for the actual
>>>    handling case
>>
>> Indeed.
>>
>>> 2) In case of an actual interrupt the flow (ignoring locking) is:
>>>
>>>    handle_xxx_irq()
>>>
>>> 	mask_irq();	/* chip->irq_mask() maps to EOI */
>>>
>>> 	if (!action || irq_disabled())
>>> 	   return;
>>>
>>> 	handle_actions();
>>>
>>> 	if (irq_threads_active() || irq_disabled())
>>> 	   return;
>>>
>>> 	unmask_irq();	/* chip->irq_unmask() maps to DIR */
>>>
>>>   So that is handle_level_irq() with the chip callbacks being:
>>>
>>>    irq_startup  = gic_unmask
>>>    irq_shutdown = gic_mask
>>>    irq_unmask	= gic_dir
>>>    irq_mask	= gic_eoi
>>
>> So while this works really well for the interrupt handling part, it will
>> break [un]mask_irq(). This is because you can only write to EOI for an
>> interrupt that you have ACKed just before (anything else and the GIC
>> state machine goes crazy). Basically, any use for EOI/DIR outside of the
>> interrupt context itself (hardirq or thread) is really dangerous.
> 
> I really doubt that the DIR invocation is dangerous outside of
> interrupt context. Otherwise your threaded scheme would not work at
> all as the DIR invocation happens in thread context.

There is a small restriction in the use of DIR (quoting the spec):

"If the interrupt identified in the GICC_DIR is not active, and is not a
spurious interrupt, the effect of the register write is UNPREDICTABLE.
This means any GICC_DIR write must identify an interrupt for which there
has been a valid GICC_EOIR or GICC_AEOIR write."

I think that affect the irq_enable you describe below.

> The nice thing about the lazy irq disable code is that the irq_mask(),
> i.e. EOI, invocation actually happens always in hard interrupt
> context. We should never invoke irq_mask() from any other context if
> you supply a startup/shutdown function.
> 
>> If we had a flag like IRQCHIP_UNMASK_IS_STARTUP, we could distinguish
>> this particular case, but that's borderline ugly.
> 
> Indeed. But I don't think it is required. See also below.
> 
>>> 4) In the lazy irq disable case if the interrupt fires mask_irq()
>>>    [EOI] is good enough to silence it.
>>>
>>>    Though in the enable_irq() case you cannot rely on the automatic
>>>    resend of the interrupt when you unmask [DIR]. So we need to make
>>>    sure that even in the level case (dunno whether that's supported in
>>>    that mode) we end up calling the irq_retrigger() callback. But
>>>    that's rather simple to achieve with a new chip flag.
>>
>> I think this one breaks for the same reason as above. And an interrupt
>> masked with EOI cannot easily be restarted without clearing the ACTIVE
>> bit (and everything becomes even more of a complete madness).
> 
> So we already established that irq_mask()/EOI will only be called from
> the actual interrupt context and irq_unmask()/DIR must be safe to be
> called from any context in order to make the EOI/DIR based threaded
> optimization work.
> 
> So the only interesting code path is enable_irq() which invokes
> irq_enable() and then the resend/retrigger machinery. 
> 
> irq_enable() calls chip->irq_unmask(), i.e. DIR. So that clears the
> ACTIVE bit and then the IRQ either gets resent by hardware (in case of
> level as the device interrupt is still active) or retriggered by the
> irq_retrigger() callback.

The problem I see here is for an interrupt that has been flagged as
disabled with irq_disabled(), but that hasn't fired. We'd end up doing a
DIR on something that hasn't had an EOI first. I think that's the only
wrinkle in this scheme.

I'll implement something today, that will help me thinking.

Thanks,

	M.