Re: [PATCH v3 3/6] clk: imx: imx8mp: Add audiomix block control

[Abel Vesa <abel.vesa@linaro.org>]

On 22-08-11 18:03:04, Abel Vesa wrote:
> On 22-08-11 16:30:20, Marek Vasut wrote:
> > On 8/11/22 16:20, Abel Vesa wrote:
> > > On 22-08-04 11:31:33, Marek Vasut wrote:
> > > > On 8/4/22 11:13, Peng Fan wrote:
> > > > > > Subject: Re: [PATCH v3 3/6] clk: imx: imx8mp: Add audiomix block control
> > > > > > 
> > > > > > On 6/28/22 09:44, Abel Vesa wrote:
> > > > > > > On 22-06-27 18:23:33, Marek Vasut wrote:
> > > > > > > > On 6/27/22 17:35, Abel Vesa wrote:
> > > > > > > > > On 22-06-25 03:32:32, Marek Vasut wrote:
> > > > > > > > > > Unlike the other block control IPs in i.MX8M, the audiomix is
> > > > > > > > > > mostly a series of clock gates and muxes. Model it as a large
> > > > > > > > > > static table of gates and muxes with one exception, which is the
> > > > > > > > > > PLL14xx . The PLL14xx SAI PLL has to be registered separately.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Again, there is a chance that the blk-ctrl driver might disable the
> > > > > > > > > PD from under this.
> > > > > > > > 
> > > > > > > > Can you elaborate a bit more on this ? How/why do you think so ?
> > > > > > > 
> > > > > > > At some point, the PDs from the Audiomix IP block will be added to the
> > > > > > > drivers/soc/imx/imx8mp-blk-ctrl.c. Then, you'll have 2 drivers with
> > > > > > > the same address range and the imx8mp-blk-ctrl also has runtime PM
> > > > > > enabled.
> > > > > > 
> > > > > > Why would the PDs be added into the block control driver?
> > > > > > 
> > > > > > The audiomix is purely a clock mux driver, not really a block control driver
> > > > > > providing PDs of its own.
> > > > > 
> > > > > I recalled that with with blk-ctrl working as clock provider, there is dead lock
> > > > > issue, if the blk-ctrl node has a power-domain entry. Not very sure.
> > > > 
> > > > How can I verify that ? Lockdep ?
> > > > 
> > > > I run this series for months and haven't seen a lock up or splat.
> > > 
> > > Audiomix (and every mix actually) has a PD. Once you add the PD, you'll
> > > end up with an ABBA deadlock between genpd lock and clock prepare lock.
> > 
> > Unlike the other mix drivers, this is a pure clock driver, not a power
> > domain driver. The PD is already available to this clock driver, see:
> > [PATCH v3 5/6] arm64: dts: imx8mp: Add SAI, SDMA, AudioMIX
> 
> When you will enable the runtime PM for this driver, the deadlock is
> going to happen and it will be in some scenario like:
>     clk_disable_unused_subtree
>       -> clk_prepare (takes prepare lock) (for a clock from your driver)
> 	-> runtime pm (takes genpd lock)
> 	  -> clk_prepare (tries to take prepare lock again) (for the clock of the PD)
> 

Actually, I'm wrong, that is not a deadlock, but this one is:

[   11.667711][  T108] -> #1 (&genpd->mlock){+.+.}-{3:3}:
[   11.675041][  T108]        __lock_acquire+0xae4/0xef8
[   11.680093][  T108]        lock_acquire+0xfc/0x2f8
[   11.684888][  T108]        __mutex_lock+0x90/0x870
[   11.689685][  T108]        mutex_lock_nested+0x44/0x50
[   11.694826][  T108]        genpd_lock_mtx+0x18/0x24
[   11.699706][  T108]        genpd_runtime_resume+0x90/0x214 (hold genpd->mlock)
[   11.705194][  T108]        __rpm_callback+0x80/0x2c0
[   11.710160][  T108]        rpm_resume+0x468/0x650
[   11.714866][  T108]        __pm_runtime_resume+0x60/0x88
[   11.720180][  T108]        clk_pm_runtime_get+0x28/0x9c
[   11.725410][  T108]        clk_disable_unused_subtree+0x8c/0x144
[   11.731420][  T108]        clk_disable_unused_subtree+0x124/0x144
[   11.737518][  T108]        clk_disable_unused+0xa4/0x11c (hold prepare_lock)
[   11.742833][  T108]        do_one_initcall+0x98/0x178
[   11.747888][  T108]        do_initcall_level+0x9c/0xb8
[   11.753028][  T108]        do_initcalls+0x54/0x94
[   11.757736][  T108]        do_basic_setup+0x24/0x30
[   11.762614][  T108]        kernel_init_freeable+0x70/0xa4
[   11.768014][  T108]        kernel_init+0x14/0x18c
[   11.772722][  T108]        ret_from_fork+0x10/0x18

[   11.777512][  T108] -> #0 (prepare_lock){+.+.}-{3:3}:
[   11.784749][  T108]        check_noncircular+0x134/0x13c
[   11.790064][  T108]        validate_chain+0x590/0x2a04
[   11.795204][  T108]        __lock_acquire+0xae4/0xef8
[   11.800258][  T108]        lock_acquire+0xfc/0x2f8
[   11.805050][  T108]        __mutex_lock+0x90/0x870
[   11.809841][  T108]        mutex_lock_nested+0x44/0x50
[   11.814983][  T108]        clk_unprepare+0x5c/0x100 ((hold prepare_lock))
[   11.819864][  T108]        imx8m_pd_power_off+0xac/0x110
[   11.825179][  T108]        genpd_power_off+0x1b4/0x2dc
[   11.830318][  T108]        genpd_power_off_work_fn+0x38/0x58 (hold genpd->mlock)
[   11.835981][  T108]        process_one_work+0x270/0x444
[   11.841208][  T108]        worker_thread+0x280/0x4e4
[   11.846176][  T108]        kthread+0x13c/0x14

All it needs is the runtime PM in your current driver.

Bottom line is, we cannot have clock providers that have PDs that have
in turn their own clocks.

So we need to drop the blk_ctrl as clock providers and go with Lucas's
approach.

> > 
> > Can you please elaborate on the deadlock problem ?
> > Because really, I just don't see it.
> > 
> > Were you able to reproduce the deadlock with this driver ?
> > 
> > > Have a read here:
> > > 
> > > https://lore.kernel.org/lkml/160453833813.3965362.13967343909525787375@swboyd.mtv.corp.google.com/T/#m0160265b0604ac8a524fedae7845e9f60bae67ef
> > 
> > Which part of the lengthy thread do you refer to ? I suspect the 'permalink'
> > might help pointing to specific email in the thread.
> > 
> > Note that the aforementioned thread discusses the other mix drivers which
> > are PDs, this driver is not, there is a difference.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel