[RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[adharmap]

From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

When lazy disabling is implemented and an interrupt is disabled the
genirq code ends up marking it as IRQ_DISABLED in the descriptor.
The interrupt stays enabled in the controller.  If the interrupt
fires after disabling, the flow handlers namely handle_level_irq and
handle_edge_irq mask the interrupt in the controller.

This is not the case with handle_nested_irq. The interrupt stays enabled in
the controller and if it were a level interrupt it keeps firing only to be
ignored by handle_nested_irq.

Update handle_nested_irq to mask such an interrupt.

Change-Id: Id0fa3280c49a36aa8b8db1d5cc20472bf5e53c5f
Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
---
 This problem shows up on my hardware because the interrupt controller is
 over a slow bus and it doesnt deactivate the interrupt line to the processor
 until an ack or mask operation is carried out for each of its active
 interrupts.

 I could have updated the interrupt controller thread itself to check if the
 interrupt is marked IRQ_DISABLED but didn't seem right to take the
 desc->lock in there. Instead updating handle_nested_irq to handle this
 seemed like the right thing to do.

 kernel/irq/chip.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index baa5c4a..35ccc41 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -419,6 +419,7 @@ void handle_nested_irq(unsigned int irq)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	struct irqaction *action;
+	int mask_this_irq = 0;
 	irqreturn_t action_ret;
 
 	might_sleep();
@@ -428,8 +429,10 @@ void handle_nested_irq(unsigned int irq)
 	kstat_incr_irqs_this_cpu(irq, desc);
 
 	action = desc->action;
-	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
+	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+		mask_this_irq = 1;
 		goto out_unlock;
+	}
 
 	desc->status |= IRQ_INPROGRESS;
 	raw_spin_unlock_irq(&desc->lock);
@@ -443,6 +446,11 @@ void handle_nested_irq(unsigned int irq)
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);
+	if (unlikely(mask_this_irq)) {
+		chip_bus_lock(irq, desc);
+		desc->chip->mask(irq);
+		chip_bus_sync_unlock(irq, desc);
+	}
 }
 EXPORT_SYMBOL_GPL(handle_nested_irq);
 
-- 
1.7.1
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[Thomas Gleixner]

On Tue, 26 Oct 2010, adharmap@codeaurora.org wrote:

> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> 
> When lazy disabling is implemented and an interrupt is disabled the
> genirq code ends up marking it as IRQ_DISABLED in the descriptor.
> The interrupt stays enabled in the controller.  If the interrupt
> fires after disabling, the flow handlers namely handle_level_irq and
> handle_edge_irq mask the interrupt in the controller.
> 
> This is not the case with handle_nested_irq. The interrupt stays enabled in
> the controller and if it were a level interrupt it keeps firing only to be
> ignored by handle_nested_irq.
> 
> Update handle_nested_irq to mask such an interrupt.
> 
> Change-Id: Id0fa3280c49a36aa8b8db1d5cc20472bf5e53c5f
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> ---
>  This problem shows up on my hardware because the interrupt controller is
>  over a slow bus and it doesnt deactivate the interrupt line to the processor
>  until an ack or mask operation is carried out for each of its active
>  interrupts.
> 
>  I could have updated the interrupt controller thread itself to check if the
>  interrupt is marked IRQ_DISABLED but didn't seem right to take the
>  desc->lock in there. Instead updating handle_nested_irq to handle this
>  seemed like the right thing to do.
> 
>  kernel/irq/chip.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index baa5c4a..35ccc41 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -419,6 +419,7 @@ void handle_nested_irq(unsigned int irq)
>  {
>  	struct irq_desc *desc = irq_to_desc(irq);
>  	struct irqaction *action;
> +	int mask_this_irq = 0;
>  	irqreturn_t action_ret;
>  
>  	might_sleep();
> @@ -428,8 +429,10 @@ void handle_nested_irq(unsigned int irq)
>  	kstat_incr_irqs_this_cpu(irq, desc);
>  
>  	action = desc->action;
> -	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> +	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
> +		mask_this_irq = 1;
>  		goto out_unlock;
> +	}
>  
>  	desc->status |= IRQ_INPROGRESS;
>  	raw_spin_unlock_irq(&desc->lock);e
> @@ -443,6 +446,11 @@ void handle_nested_irq(unsigned int irq)
>  
>  out_unlock:
>  	raw_spin_unlock_irq(&desc->lock);
> +	if (unlikely(mask_this_irq)) {
> +		chip_bus_lock(irq, desc);
> +		desc->chip->mask(irq);

That does not work with current mainline due to the irq_chip changes,
can you please respin against linus latest?

Also there is no requirement for irq_chip instances to implement the
mask function, so this might crash the kernel for innocent users of
that infrastructure. mask_irq() is your friend.

Aside of that this wont work for edge triggered interrupts, as you'd
loose the edge, so this needs more thought and a thorough look at the
users of handle_nested_irq().

One possible solution would be to change handle_nested_irq() to return
int. So we could return 0 for the normal case and some sensible error
code when the irq is disabled, so the caller can deal with it. Either
that or we need a separate nested function for level irqs which then
can implement the mask logic.

Thanks,

	tglx

Re: [RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[Abhijeet Dharmapurikar]

Thomas Gleixner wrote:
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index baa5c4a..35ccc41 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -419,6 +419,7 @@ void handle_nested_irq(unsigned int irq)
>>  {
>>  	struct irq_desc *desc = irq_to_desc(irq);
>>  	struct irqaction *action;
>> +	int mask_this_irq = 0;
>>  	irqreturn_t action_ret;
>>  
>>  	might_sleep();
>> @@ -428,8 +429,10 @@ void handle_nested_irq(unsigned int irq)
>>  	kstat_incr_irqs_this_cpu(irq, desc);
>>  
>>  	action = desc->action;
>> -	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
>> +	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
>> +		mask_this_irq = 1;
>>  		goto out_unlock;
>> +	}
>>  
>>  	desc->status |= IRQ_INPROGRESS;
>>  	raw_spin_unlock_irq(&desc->lock);e
>> @@ -443,6 +446,11 @@ void handle_nested_irq(unsigned int irq)
>>  
>>  out_unlock:
>>  	raw_spin_unlock_irq(&desc->lock);
>> +	if (unlikely(mask_this_irq)) {
>> +		chip_bus_lock(irq, desc);
>> +		desc->chip->mask(irq);
> 
> That does not work with current mainline due to the irq_chip changes,
> can you please respin against linus latest?
Sure will address this in the next patch.
> 
> Also there is no requirement for irq_chip instances to implement the
> mask function, so this might crash the kernel for innocent users of
> that infrastructure. mask_irq() is your friend.
Thanks, this one too.
> 
> Aside of that this wont work for edge triggered interrupts, as you'd
> loose the edge, so this needs more thought and a thorough look at the
> users of handle_nested_irq().

I didn't understand this though. This patch will mask the interrupt in 
the controller even if it were edge. My interrupt controller latches
edges and wants a mask (or an ack) to be executed to deactivate the line 
summary line. Do you mean that I should mark the interrupt IRQ_PENDING 
if it were an edge before masking it? If not, can you please explain.

Thanks,
Abhijeet

Re: [RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[Thomas Gleixner]

On Thu, 28 Oct 2010, Abhijeet Dharmapurikar wrote:
> Thomas Gleixner wrote:
> > Aside of that this wont work for edge triggered interrupts, as you'd
> > loose the edge, so this needs more thought and a thorough look at the
> > users of handle_nested_irq().
> 
> I didn't understand this though. This patch will mask the interrupt in the
> controller even if it were edge. My interrupt controller latches
> edges and wants a mask (or an ack) to be executed to deactivate the line
> summary line. Do you mean that I should mark the interrupt IRQ_PENDING if it
> were an edge before masking it? If not, can you please explain.

See handle_edge_irq().

An edge is a one time event. Once you mask/ack it, it's gone. So now
when you unmask it won't reissue the interrupt on the hardware
level. Level interrupts do, as the mask does not affect that.

Thanks,

	tglx

Re: [RFC IRQ] genirq: fix handle_nested_irq for lazy disable

[Abhijeet Dharmapurikar]

Thomas Gleixner wrote:
> On Thu, 28 Oct 2010, Abhijeet Dharmapurikar wrote:
>> Thomas Gleixner wrote:
>>> Aside of that this wont work for edge triggered interrupts, as you'd
>>> loose the edge, so this needs more thought and a thorough look at the
>>> users of handle_nested_irq().
>> I didn't understand this though. This patch will mask the interrupt in the
>> controller even if it were edge. My interrupt controller latches
>> edges and wants a mask (or an ack) to be executed to deactivate the line
>> summary line. Do you mean that I should mark the interrupt IRQ_PENDING if it
>> were an edge before masking it? If not, can you please explain.
> 
> See handle_edge_irq().
> 
> An edge is a one time event. Once you mask/ack it, it's gone. So now
> when you unmask it won't reissue the interrupt on the hardware
> level. Level interrupts do, as the mask does not affect that.

handle_edge_irq() needs to handle nested invocations and so it checks if 
the irq is in progress and does the right masking/unmasking and calling 
the handler again.
But I think we will never be executing an interrupt within an interrupt 
in the threaded irq controller case. By that I mean the irq controller 
thread wont call handle_nested_irq on the same interrupt until the first 
execution finishes. We don't need to worry about nested calls to 
handle_nested_irq().
If this is right, simply marking the edge interrupt IRQF_PENDING before 
masking it will suffice IMO.
Please let me know if this is closer to what you were suggesting?

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index baa5c4a..2dd0228 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -419,6 +419,7 @@ void handle_nested_irq(unsigned int irq)
  {
         struct irq_desc *desc = irq_to_desc(irq);
         struct irqaction *action;
+       int mask_this_irq = 0;
         irqreturn_t action_ret;

         might_sleep();
@@ -428,8 +429,12 @@ void handle_nested_irq(unsigned int irq)
         kstat_incr_irqs_this_cpu(irq, desc);

         action = desc->action;
-       if (unlikely(!action || (desc->status & IRQ_DISABLED)))
+       if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+               mask_this_irq = 1;
+               if (!(desc->status & IRQ_LEVEL))
+                       desc->status |= IRQ_PENDING;
                 goto out_unlock;
+       }

         desc->status |= IRQ_INPROGRESS;
         raw_spin_unlock_irq(&desc->lock);
@@ -443,6 +448,11 @@ void handle_nested_irq(unsigned int irq)

  out_unlock:
         raw_spin_unlock_irq(&desc->lock);
+       if (unlikely(mask_this_irq)) {
+               chip_bus_lock(desc);
+               mask_irq(irq);
+               chip_bus_sync_unlock(desc);
+       }
  }
  EXPORT_SYMBOL_GPL(handle_nested_irq);

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.