Quoting Ulf Hansson (2024-04-09 03:32:04) > > Apologies for not being able to review this, it got lost in my email > filters. Looks like you manage to solve the locking order for the clk > disable unused thing - great! > > However I think the main problem we are seeing with these kind of > locking issues is that we are holding a global lock while calling into > pm_runtime_get|put*(). Similar problems have also been reported in the > past. It's been on my todo list for quite some time to have a closer > look, but I haven't reached it yet. > > Without going into too much detail, let me just ask a related > question. Would it not be possible to call pm_runtime_get/put() within > the clock framework, without *always* keeping the clock prepare lock > acquired? I assume a clock can't be unregistered, as long as there is > reference taken for it, right? Wouldn't that be a sufficient guarantee > that it's okay to runtime_resume|suspend its corresponding device? The problem is that the clk tree is being walked with the prepare_lock held and during that walk pm_runtime_get() is called on different devices. We hold the prepare_lock while walking the tree because we don't want the tree topology to change while walking. The prepare_lock is also a topology lock. If we could walk the tree, figure out what all clks will change, drop the prepare_lock, pm_runtime_get() all of those devices, regrab the prepare_lock, check the topology to make sure topology hasn't changed, and then make all the clk_ops calls, it would work because we have extracted the runtime PM calls out of the prepare_lock. Dropping and regrabbing the lock is probably a bad idea though, because we may never make progress because we're preempted by another task that changes the topology. I was also wondering if we get stuck again if the clk driver implementing the clk_ops is on some bus like i2c or spi, that runtime PM resumes the bus controller for register writes from the clk_ops, and that resume function calls clk operations, and that happens in another thread. Maybe that isn't a problem though because the runtime resume of the device will also runtime resume the parent which is spi or i2c? Either way, it really seems like we need a per-clk prepare_lock. That would let us know for sure the topology isn't changing while we walk the tree and figure out what we're going to do. Anything that calls into the clk framework again hopefully gets a different prepare lock for a different clk. BTW, I don't think lockdep is aware that runtime PM is waiting like this for a parallel resume in another thread. Probably we need to add annotations so that any locks held while waiting for the resume or suspend to finish are chained to a pseudo read lock and the actual resume/suspend operation is a pseudo write lock. > > Or maybe I should just send a patch. :-) > Sure! ;-)
