Re: [PATCH v8 13/25] clk: Set req_rate on reparenting

From: Maxime Ripard <maxime@cerno.tech>
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: linux-clk@vger.kernel.org,
	Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Tony Lindgren <tony@atomide.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Yassine Oudjana <y.oudjana@protonmail.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Naresh Kamboju <naresh.kamboju@linaro.org>
Subject: Re: [PATCH v8 13/25] clk: Set req_rate on reparenting
Date: Tue, 16 Aug 2022 13:14:01 +0200	[thread overview]
Message-ID: <20220816111401.tq5cvupjtshhdyew@houat> (raw)
In-Reply-To: <2024590.taCxCBeP46@steina-w>

[-- Attachment #1: Type: text/plain, Size: 6229 bytes --]

On Tue, Aug 16, 2022 at 11:57:26AM +0200, Alexander Stein wrote:
> Hello Maxime,
> 
> Am Dienstag, 16. August 2022, 11:24:16 CEST schrieb Maxime Ripard:
> > * PGP Signed by an unknown key
> > 
> > Hi Alexander,
> > 
> > On Tue, Aug 16, 2022 at 10:30:47AM +0200, Alexander Stein wrote:
> > > Hello Maxime,
> > > 
> > > Am Montag, 15. August 2022, 17:41:35 CEST schrieb Maxime Ripard:
> > > > If a non-rate clock started by default with a parent that never
> > > > registered, core->req_rate will be 0. The expectation is that whenever
> > > > the parent will be registered, req_rate will be updated with the new
> > > > value that has just been computed.
> > > > 
> > > > However, if that clock is a mux, clk_set_parent() can also make that
> > > > clock no longer orphan. In this case however, we never update req_rate.
> > > > Let's make sure it's the case for the newly unorphan clock and all its
> > > > children.
> > > 
> > > This works with my basic board DT, but adding an I2C attached audio codec
> > > 
> > > (sound/soc/codecs/tlv320aic32x4-clk.c) I get the following error:
> > > > BUG: sleeping function called from invalid context at
> > > > kernel/locking/mutex.c:283 in_atomic(): 1, irqs_disabled(): 128,
> > > > non_block:
> > > > 0, pid: 217, name: kworker/u8:6 preempt_count: 1, expected: 0
> > > > RCU nest depth: 0, expected: 0
> > > > CPU: 3 PID: 217 Comm: kworker/u8:6 Not tainted 6.0.0-rc1-next-20220816+
> > > > #521 ac6fe0b093ec56bf12af4f3eda948091742739aa Hardware name: TQ-Systems
> > > > i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT)
> > > > Workqueue: events_unbound deferred_probe_work_func
> > > > 
> > > > Call trace:
> > > >  dump_backtrace+0xd4/0x114
> > > >  show_stack+0x14/0x4c
> > > >  dump_stack_lvl+0x64/0x7c
> > > >  dump_stack+0x14/0x2c
> > > >  __might_resched+0x124/0x154
> > > >  __might_sleep+0x58/0xcc
> > > >  mutex_lock+0x20/0x70
> > > >  regmap_lock_mutex+0xc/0x1c
> > > >  regmap_read+0x38/0x70
> > > >  clk_aic32x4_div_recalc_rate+0x34/0x70 [snd_soc_tlv320aic32x4
> > > >  4f2256fee3bc49277632fba80c047a2b8a3ad122] clk_recalc+0x44/0xe0
> > > >  clk_core_update_orphan_child_rates+0x28/0x60
> > > >  clk_core_update_orphan_child_rates+0x48/0x60
> > > >  clk_core_update_orphan_child_rates+0x48/0x60
> > > >  clk_core_update_orphan_child_rates+0x48/0x60
> > > >  clk_core_update_orphan_child_rates+0x48/0x60
> > > >  clk_reparent+0xa4/0x14c
> > > >  __clk_set_parent_before+0x40/0xa0
> > > >  clk_core_set_parent_nolock+0x11c/0x27c
> > > >  clk_set_parent+0x3c/0x140
> > > >  __set_clk_parents+0x114/0x244
> > > >  of_clk_set_defaults+0x20/0x50
> > > >  platform_probe+0x38/0x100
> > > >  call_driver_probe+0x28/0x140
> > > >  really_probe+0xc0/0x334
> > > >  __driver_probe_device+0x84/0x144
> > > >  driver_probe_device+0x38/0x130
> > > >  __device_attach_driver+0xc8/0x17c
> > > >  bus_for_each_drv+0x74/0xc4
> > > >  __device_attach+0xa8/0x204
> > > >  device_initial_probe+0x10/0x1c
> > > >  bus_probe_device+0x90/0xa0
> > > >  deferred_probe_work_func+0x9c/0xf0
> > > >  process_one_work+0x1d0/0x330
> > > >  worker_thread+0x68/0x390
> > > >  kthread+0xf4/0xfc
> > > >  ret_from_fork+0x10/0x20
> > > 
> > > The audio codec driver provides clocks as well which can't be used in
> > > atomic contexts.
> > 
> > So, this is due to clk_reparent() being called with enable_lock taken in
> > __clk_set_parent_before(), and enable_lock is a spinlock.
> > 
> > The other call sites of clk_reparent() are __clk_set_parent(), that
> > takes that lock too, and clk_core_reparent() that doesn't.
> > 
> > __clk_set_parent() is used exclusively by clk_core_set_parent_nolock()
> > and has the assumption that only the prepare_lock (mutex) is taken.
> > 
> > clk_core_reparent() is used exclusively by clk_hw_reparent(), which is
> > then used by four drivers (clk-stm32mp1.c, stm32/clk-stm32-core.c,
> > clk/tegra/clk-tegra124-emc.c and tegra/clk-tegra210-emc.c)
> > 
> > All but tegra124 use clk_hw_reparent() in their set_parent
> > implementation. The set_parent hook is called in __clk_set_parent() and
> > clk_change_rate(), both times without the enable_lock taken.
> > 
> > tegra210 has it in its set_rate implementation, called only by
> > clk_change_rate(), without the enable_lock taken too.
> > 
> > So I think that if we move the call to
> > clk_core_update_orphan_child_rates() to the clk_reparent() call sites,
> > after the enable_lock has been released if it was taken, we should be
> > safe.
> > 
> > Could you test the following patch?
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 5bdfd645f1dc..453e2ff10961 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1941,7 +1941,6 @@ static void clk_reparent(struct clk_core *core, struct
> > clk_core *new_parent) }
> > 
> >  	core->parent = new_parent;
> > -	clk_core_update_orphan_child_rates(core);
> >  }
> > 
> >  static struct clk_core *__clk_set_parent_before(struct clk_core *core,
> > @@ -1987,6 +1986,8 @@ static struct clk_core *__clk_set_parent_before(struct
> > clk_core *core, clk_reparent(core, parent);
> >  	clk_enable_unlock(flags);
> > 
> > +	clk_core_update_orphan_child_rates(core);
> > +
> >  	return old_parent;
> >  }
> > 
> > @@ -2031,6 +2032,8 @@ static int __clk_set_parent(struct clk_core *core,
> > struct clk_core *parent, flags = clk_enable_lock();
> >  		clk_reparent(core, old_parent);
> >  		clk_enable_unlock(flags);
> > +
> > +		clk_core_update_orphan_child_rates(core);
> >  		__clk_set_parent_after(core, old_parent, parent);
> > 
> >  		return ret;
> > @@ -2654,6 +2657,7 @@ static void clk_core_reparent(struct clk_core *core,
> >  				  struct clk_core *new_parent)
> >  {
> >  	clk_reparent(core, new_parent);
> > +	clk_core_update_orphan_child_rates(core);
> >  	__clk_recalc_accuracies(core);
> >  	__clk_recalc_rates(core, POST_RATE_CHANGE);
> >  }
> 
> With this patch applied the BUG message is gone and the system boots without 
> any issue.

Thanks for testing :)

I've merged that patch into the original one, updated the commit log and
will submit a new version of this series.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]