On 2021-01-04 03:46, Samuel Holland wrote:

On 1/3/21 7:10 AM, Marc Zyngier wrote:

quoted

On Sun, 03 Jan 2021 12:08:43 +0000,
Samuel Holland [off-list ref] wrote:

quoted

On 1/3/21 5:27 AM, Marc Zyngier wrote:

[...]

quoted

quoted

For edge interrupts, don't you want to ack as early as possible,
before the handler clears the source of the interrupt? That way if a
second interrupt comes in while you're handling the first one, you
don't ack the second one without handling it?

It completely depends on what this block does. If, as I expect, it
latches the interrupt, then it needs clearing after the GIC has acked
the incoming interrupt.

Yes, there is an internal S/R latch.
 - For edge interrupts, the latch is set once for each pulse.
 - For level interrupts, it gets set continuously as long as the
   pin is high/low.
 - Writing a "1" to bit 0 of PENDING resets the latch.
 - The output of the latch goes to the GIC.

quoted

quoted

quoted

It also begs the question: why would you want to clear the signal to
the GIC on mask (or unmask)? The expectations are that a pending
interrupt is preserved across a mask/unmask sequence.

I hadn't thought about anything masking the IRQ outside of the
handler; but you're right, this breaks that case. I'm trying to work
within the constraints of stacking the GIC driver, which assumes
handle_fasteoi_irq, so it sounds like I should switch back to
handle_fasteoi_ack_irq and use .irq_ack. Or based on your previous
paragraph, maybe I'm missing some other consideration?

handle_fasteoi_ack_irq() sounds like a good match for edge
interrupts. Do you actually need to do anything for level signals? If
you do, piggybacking on .irq_eoi would do the trick.

For level interrupts, I have to reset the latch (see above) after the 
source of
the interrupt is cleared.

Right, so that is definitely to be done in .irq_eoi, at least in the
non-threaded case (as it doesn't involve masking/unmasking).

That was the bug with v2: I set IRQ_EOI_THREADED so .irq_eoi would run 
after the
thread. But with GICv2 EOImode==0, that blocked other interrupts from 
being
received during the IRQ thread. Which is why I moved it to .irq_unmask 
and
removed the flag: so .irq_eoi runs at the end of the hardirq 
(unblocking further
interrupts at the GIC), and .irq_unmask resets the latch at the end of
the thread.

With the flag removed, but still clearing the latch in .irq_eoi, every 
edge IRQ

edge? Didn't you mean level here? Edge interrupts really should clear
the latch in .irq_ack.

was followed by a second, spurious IRQ after the thread finished.

Does that make sense?

It does. It is a bit of a kludge, but hey, silly HW (if only this could 
be
turned into a bypass, it'd all be simpler).

To sum it up, this is what I'd expect to see:

For edge interrupts:
- clear latch in .irq_ack and .irq_set_irqchip_state(PENDING)
- interrupt flow set to fasteoi_ack

For level interrupts
- clear latch in .irq_eoi (non-threaded) and .irq_unmask (threaded)
- interrupt flow set to fasteoi (though leaving to the _ack version
   should not hurt).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel