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

[Marc Zyngier]

On 29/10/14 10:26, Thomas Gleixner wrote:
> On Wed, 29 Oct 2014, Marc Zyngier wrote:
>> On 28/10/14 20:14, Thomas Gleixner wrote:
>>> 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.
> 
> Right. So the untested patch below should do the trick and prevent
> irq_enable() to invoke irq_unmask() if the interrupt is not flagged
> masked. And it only can be flagged masked if it was masked in the
> handler. The startup callback will make sure that irq_enable() is not
> invoked at startup time.
> 
> Thanks,
> 
> 	tglx
> 
> ------------------
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index e5202f00cabc..9c0f73e1994a 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -182,7 +182,7 @@ int irq_startup(struct irq_desc *desc, bool resend)
>  		ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
>  		irq_state_clr_masked(desc);
>  	} else {
> -		irq_enable(desc);
> +		irq_enable(desc, false);
>  	}
>  	if (resend)
>  		check_irq_resend(desc, desc->irq_data.irq);
> @@ -202,13 +202,14 @@ void irq_shutdown(struct irq_desc *desc)
>  	irq_state_set_masked(desc);
>  }
>  
> -void irq_enable(struct irq_desc *desc)
> +void irq_enable(struct irq_desc *desc, bool ifmasked)
>  {
>  	irq_state_clr_disabled(desc);
> -	if (desc->irq_data.chip->irq_enable)
> +	if (desc->irq_data.chip->irq_enable) {
>  		desc->irq_data.chip->irq_enable(&desc->irq_data);
> -	else
> +	} else if (!ifmasked || irqd_irq_masked(&desc->irq_data)) {
>  		desc->irq_data.chip->irq_unmask(&desc->irq_data);
> +	}
>  	irq_state_clr_masked(desc);
>  }
>  
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 4332d766619d..6eff2678cf6d 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -68,7 +68,7 @@ extern void __enable_irq(struct irq_desc *desc, unsigned int irq);
>  
>  extern int irq_startup(struct irq_desc *desc, bool resend);
>  extern void irq_shutdown(struct irq_desc *desc);
> -extern void irq_enable(struct irq_desc *desc);
> +extern void irq_enable(struct irq_desc *desc, bool ifmasked);
>  extern void irq_disable(struct irq_desc *desc);
>  extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
>  extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 0a9104b4608b..d8c474608c2d 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -448,7 +448,7 @@ void __enable_irq(struct irq_desc *desc, unsigned int irq)
>  			goto err_out;
>  		/* Prevent probing on this irq: */
>  		irq_settings_set_noprobe(desc);
> -		irq_enable(desc);
> +		irq_enable(desc, true);
>  		check_irq_resend(desc, irq);
>  		/* fall-through */
>  	}
> 

So I actually implemented this, and did hit another snag: per cpu interrupts.
They don't use the startup/shutdown methods, and reproducing the above logic
on a per-cpu basis is not very pretty.

In order to make some progress, I went on a slightly different path, which
is to use enable/disable instead of startup/shutdown. As far as I can see,
the only thing we loose by doing so is the lazy disable, but the code
becomes very straightforward (see below). I gave it a go on an ARMv7 box,
and it even survived.

Thoughts?

	M.