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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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
Alexander