Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking

[Brian Norris <briannorris@xxxxxxxxxxxx>]

Hi,

On Tue, Aug 23, 2022 at 10:50:16AM +0800, Jeffy Chen wrote:
> On 8/23 星期二 1:08, Doug Anderson wrote:
> > I'm happy to let others say for sure, but from my point of view I'm
> > not convinced. It feels like with your new code you could lose edges.

I won't claim to know for sure either, but I'm trying to follow along,
because we've dealt with various sorts of nasty logical errors in the
gpio and/or irqchip areas here :)

> > ...in other words an edge happened _after_ the IRQ handler ran but we
> > didn't call the IRQ handler again. I don't think this is right.

I believe Doug has identified one problem correctly -- that we can't
handle the both-edge toggling entirely after the ack+dispatch.

(out-of-order quote:)
> So if an edge come in between, that new IRQ status would be acked(cleared)
> in the following GPIO irq handler as well as the old one, without triggering
> another IRQ demux() to toggle the trigger mode.

I believe Jeffy has identified another -- that we can't handle the
both-edge toggling entirely before the ack+dispatch.

> Right, so guessing we could somehow move the IRQ ack into the toggling flow
> to make sure that it would not clear the new IRQ?

This sounds like we need the toggling to happen within the .irq_ack()
callback, in between the ACK and the chain dispatch.

I'm not sure I've fully walked through this yet, but would it suffice to
move the 'toggle_edge_mode' loop into irq_ack(), right after writing the
EOI register? That way we're in one of these cases within irq_ack():

(1) no further edges occurred after the ACK/EIO write: the toggle-loop
will toggle correctly, and then we dispatch the chained handler.

(2) one or more additional edges occurred after the ACK/EIO write
(possibly during the toggle-loop): the loop only exits when we're sure
things have quiesced, so future edges will toggle correctly (i.e.,
polarity is correct). We continue to dispatch the chained handler (which
"handles" all these edge(s)). We may still have leave an IRQ pending (if
many edges happen, racing with the toggle loop) and fire again shortly,
but that's OK.

Does this make sense to anyone else?

> And it looks like there are quite a few drivers having this kind of need,
> would it make sense to handle it in the framework?

I suppose it would be possible to implement the above as a general
irq_gc_ack*() helper, instead of open-coding it, although it might need
the irqchip framework to gain a better understanding of a chip's
polarity register.

Brian