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