Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 01/22/2014 09:52 PM, Jason Gunthorpe wrote:
>> Clearing BRIDGE_CAUSE will only clear all currently pending upstream
>> IRQs, of course. If WDT IRQ will be re-raised right after that in
>> BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no
>> clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.
>
> Which is why it makes no sense to clear it one time at kernel start.
>
> Either you only get new edge triggered interrupts after request_irq
> (sane behavior) or you might sometimes get an old pending edge
> triggered interrupt after request_irq (crazy behavior).
>
> Clearing BRIDGE_CAUSE at kernel start only shortens the racy window it
> doesn't eliminate it.

Actually, I missed that we mask all BRIDGE irqs in
orion_bridge_irq_init. If we now also clear already pending irqs, that
will not raise any old interrupts as long as watchdog clears all
reasons for upstream irqs before requesting a BRIDGE irq.

> In the more familiar level triggered world the driver would go to the
> device and ensure it wasn't asserting an IRQ level and then do the
> request_irq. This guarentees it won't get an interrupt callback.
>
> In a edge triggered world the driver should go to the device an ensure
> that it won't create a new IRQ, then do request_irq - confident that
> there will NEVER be a call to the IRQ handler, under any
> circumstances.
>
> So I think edge triggered interrupts need to ack any possible old edge
> trigger in the cause register before the first unmask - eg in the
> setup callback.
 >
>> So, you should also clear WDT's irq in the driver yourself to clear a
>> possible pending upstream BRIDGE_CAUSE.
>
> Which isn't possible - the BRIDGE_CAUSE is owned by the irq driver and
> it must be cleared there, and it must only be cleared after the wdt
> has been stopped so it doesn't set it again.

I should have been more precise here: I meant watchdog driver should
clear all sources of possible upstream interrupts in its _own_
registers.

> Notice that Ezequiel has added an IRQ handler that just calls panic,
> so a spurious interrupt call is VERY VERY BAD.

And I understand that he now clears watchdog's register before
requesting an irq. All that is missing is bridge_irq driver clearing
CAUSE register after masking all irqs, right?

I'll stich a patch for that hopefully tomorrow.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html