On Tue, Sep 9, 2014 at 3:07 AM, Tomi Valkeinen tomi.valkeinen@ti.com wrote:
lockdep won't complain though since you essentially open-code a workqueue_flush, and lockdep also doesn't complain about all possible deadlocks (due to some design issues with lockdep).
What do you mean "open-code a workqueue_flush"?. I use flush_workqueue there. We have two things to wait for: work on the workqueue and work which is triggered by the vsync irq. So we loop and test for both of those, until there's no more work.
Oops, missed that. Ordering looks wrong though since if the irq can latch the workqueue you need to wait for irqs to happen first before flushing. And obviously queue the work before signalling the completion of the interrupt. But since this seems to lack locking anyway and is only used for unload it doesn't really matter.
from a quick look at omap_crtc_flush(), you probably also race w/ apply worker list manipulation (like moving from queued_applies to pending_applies)
Yeah, well, the workqueue can create work for the irq also. I don't know if it does, currently, but I think it's safer to presume that both workqueue and the irq can create work to the other.
But that's why I have a loop there. So we flush, then check if there is work for the irq. If yes, sleep a bit and go back to start. So if the irq work created new work for the wq, we flush that. And if that work created new work for the irq, we check that again. Etc.
I think adding an internal per-crtc apply_lock would solve a lot of problems. I was originally shying away from recommending that, mostly because I didn't want to think about it and I though there was an easier solution.
But with an apply_lock we could at least add some locking to _flush() and make it not racy as well..
Although, I wonder if some waitqueue scheme would be a bit more sane.. ie. end of apply_worker, if there is nothing more queued up, signal the event that _flush() is waiting on.
BR, -R