Hi,
On Thursday 06 September 2012 16:36:54 Tomi Valkeinen wrote:
Hi,
On Sun, 2012-09-02 at 22:12 +0300, Dimitar Dimitrov wrote:
quoted
Very rare kernel crashes are reported on a custom OMAP4 board. Kernel
panics due to corrupted completion structure while executing
dispc_irq_wait_handler(). Excerpt from kernel log:
Internal error: Oops - undefined instruction: 0 [#1] PREEMPT SMP
Unable to handle kernel paging request at virtual address 00400130
...
PC is at 0xebf205bc
LR is at __wake_up_common+0x54/0x94
...
(__wake_up_common+0x0/0x94)
(complete+0x0/0x60)
(dispc_irq_wait_handler.36902+0x0/0x14)
(omap_dispc_irq_handler+0x0/0x354)
(handle_irq_event_percpu+0x0/0x188)
(handle_irq_event+0x0/0x64)
(handle_fasteoi_irq+0x0/0x10c)
(generic_handle_irq+0x0/0x48)
(asm_do_IRQ+0x0/0xc0)
DISPC IRQ executes callbacks with dispc.irq_lock released. Hence
unregister_isr() and DISPC IRQ might be running in parallel on different
CPUs. So there is a chance that a callback is executed even though it
has been unregistered. As omap_dispc_wait_for_irq_timeout() declares a
completion on stack, the dispc_irq_wait_handler() callback might try to
access a completion structure that is invalid. This leads to crashes and
hangs.
Solution is to divide unregister calls into two sets:
1. Non-strict unregistering of callbacks. Callbacks could safely be
executed after unregistering them. This is the case with unregister
calls from the IRQ handler itself.
2. Strict (synchronized) unregistering. Callbacks are not allowed
after unregistering. This is the case with completion waiting.
The above solution should satisfy one of the original intentions of the
driver: callbacks should be able to unregister themselves.
I think it'd be better to create a new function for the nosync version,
and keep the old name for the sync version. The reason for this is to
minimize the amount of changes, as I think this one needs to be applied
to stable kernel trees also.
My intention was to force all callers to pick sides. In case of rebase issues
we get link errors instead of rare and subtle run-time races. Still, if you
think we should leave the old name untouched then I'll change my
patch.
Also, I think we need similar one for dsi.c, as it has the same kind of
irq handling. But with a quick glance only sync version is needed there.
Thanks, I missed that. I'll try to fix and send again.
However, I'm not quite sure about this approach. The fix makes sense,
but it makes me think if the irq handling is designed the wrong way.
While debugging and fixing this, did you think some other irq handling
approach would be saner?
I tried but could not come up with better approach. The main difficulty is
that there are two contradicting requirements:
1. Some callbacks unregister other callbacks, including themselves, from
DISPC IRQ context.
2. Some functions expect that once a callback is unregistered it is never
executed.
Hence it is natural to split callers into two sets. I'm open for suggestions.
Tomi
Thanks,
Dimitar