| Message ID | 20240325184204.745706-6-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Fix a deadlock with clk_pm_runtime_get() | expand |
Hi, On Mon, Mar 25, 2024 at 11:42 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Similar to the previous commit, we should make sure that all devices are > runtime resumed before printing the clk_summary through debugfs. Failure > to do so would result in a deadlock if the thread is resuming a device > to print clk state and that device is also runtime resuming in another > thread, e.g the screen is turning on and the display driver is starting > up. We remove the calls to clk_pm_runtime_{get,put}() in this path > because they're superfluous now that we know the devices are runtime > resumed. This also squashes a bug where the return value of > clk_pm_runtime_get() wasn't checked, leading to an RPM count underflow > on error paths. Ah, interesting. Thinking about this, I guess it means that a single device that returns an error from its pm_runtime_get() will fully disable the entire system's unused clock disabling as well as the entire clk_summary. Crossing my fingers that doesn't show up in practice... > Fixes: 1bb294a7981c ("clk: Enable/Disable runtime PM for clk_summary") > Cc: Taniya Das <quic_tdas@quicinc.com> > Cc: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) Reviewed-by: Douglas Anderson <dianders@chromium.org>
Quoting Stephen Boyd (2024-03-25 11:41:59) > Similar to the previous commit, we should make sure that all devices are > runtime resumed before printing the clk_summary through debugfs. Failure > to do so would result in a deadlock if the thread is resuming a device > to print clk state and that device is also runtime resuming in another > thread, e.g the screen is turning on and the display driver is starting > up. We remove the calls to clk_pm_runtime_{get,put}() in this path > because they're superfluous now that we know the devices are runtime > resumed. This also squashes a bug where the return value of > clk_pm_runtime_get() wasn't checked, leading to an RPM count underflow > on error paths. > > Fixes: 1bb294a7981c ("clk: Enable/Disable runtime PM for clk_summary") > Cc: Taniya Das <quic_tdas@quicinc.com> > Cc: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- Applied to clk-fixes
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index c69de47afaba..3539675ea846 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3318,9 +3318,7 @@ static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c, { struct clk_core *child; - clk_pm_runtime_get(c); clk_summary_show_one(s, c, level); - clk_pm_runtime_put(c); hlist_for_each_entry(child, &c->children, child_node) clk_summary_show_subtree(s, child, level + 1); @@ -3330,11 +3328,15 @@ static int clk_summary_show(struct seq_file *s, void *data) { struct clk_core *c; struct hlist_head **lists = s->private; + int ret; seq_puts(s, " enable prepare protect duty hardware connection\n"); seq_puts(s, " clock count count count rate accuracy phase cycle enable consumer id\n"); seq_puts(s, "---------------------------------------------------------------------------------------------------------------------------------------------\n"); + ret = clk_pm_runtime_get_all(); + if (ret) + return ret; clk_prepare_lock(); @@ -3343,6 +3345,7 @@ static int clk_summary_show(struct seq_file *s, void *data) clk_summary_show_subtree(s, c, 0); clk_prepare_unlock(); + clk_pm_runtime_put_all(); return 0; } @@ -3390,8 +3393,14 @@ static int clk_dump_show(struct seq_file *s, void *data) struct clk_core *c; bool first_node = true; struct hlist_head **lists = s->private; + int ret; + + ret = clk_pm_runtime_get_all(); + if (ret) + return ret; seq_putc(s, '{'); + clk_prepare_lock(); for (; *lists; lists++) { @@ -3404,6 +3413,7 @@ static int clk_dump_show(struct seq_file *s, void *data) } clk_prepare_unlock(); + clk_pm_runtime_put_all(); seq_puts(s, "}\n"); return 0;
Similar to the previous commit, we should make sure that all devices are runtime resumed before printing the clk_summary through debugfs. Failure to do so would result in a deadlock if the thread is resuming a device to print clk state and that device is also runtime resuming in another thread, e.g the screen is turning on and the display driver is starting up. We remove the calls to clk_pm_runtime_{get,put}() in this path because they're superfluous now that we know the devices are runtime resumed. This also squashes a bug where the return value of clk_pm_runtime_get() wasn't checked, leading to an RPM count underflow on error paths. Fixes: 1bb294a7981c ("clk: Enable/Disable runtime PM for clk_summary") Cc: Taniya Das <quic_tdas@quicinc.com> Cc: Douglas Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)