Message-ID: <306af49fd69707feccb640d02dc4fbc5.sboyd@kernel.org>
Date: Tue, 09 Apr 2024 04:55:16 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, patches@...ts.linux.dev, linux-arm-msm@...r.kernel.org, Douglas Anderson <dianders@...omium.org>, Marek Szyprowski <m.szyprowski@...sung.com>, Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH v2 4/5] clk: Get runtime PM before walking tree during disable_unused

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! ;-)

Powered by blists - more mailing lists