Re: [PATCH] remoteproc: mediatek: Break lock dependency to `prepare_lock`

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, Jan 06, 2026 at 03:13:22AM +0000, Tzung-Bi Shih wrote:
> On Mon, Jan 05, 2026 at 03:16:33PM -0700, Mathieu Poirier wrote:
> > On Mon, Dec 29, 2025 at 04:31:46AM +0000, Tzung-Bi Shih wrote:
> > > `scp_ipi_send` acquires `prepare_lock` via `clk_prepare_enable` while
> > > the caller often holds `ec_dev->lock` (e.g., `cros_ec_cmd_xfer`).  The
> > > reverse dependency exists where `clk_prepare` can trigger operations
> > > that eventually take `ec_dev->lock` (e.g., via sysfs/regulator/genpd).
> > 
> > What operation would that be?  Please be specific so that I can trace the code.
> 
> The chain is discovered by lockdep: &ec_dev->lock -> prepare_lock ->
> &genpd->mlock -> ... -> kn->active#2 -> &ec_dev->lock.
> 
> -> #6 (&ec_dev->lock){+.+.}-{3:3}:
>        __mutex_lock_common
>        mutex_lock_nested
>        cros_ec_cmd_xfer
>        cros_ec_cmd_xfer_status
>        cros_usbpd_charger_get_port_status
>        cros_usbpd_charger_get_prop
>        power_supply_get_property
>        power_supply_show_property
>        power_supply_uevent
>        dev_uevent
>        uevent_show
>        dev_attr_show
>        sysfs_kf_seq_show
>        kernfs_seq_show
>        seq_read_iter
>        kernfs_fop_read_iter
>        vfs_read
> -> #5 (kn->active#2){++++}-{0:0}:
>        kernfs_drain
>        __kernfs_remove
>        kernfs_remove_by_name_ns
>        sysfs_remove_file_ns
>        device_del
>        __device_link_del
>        device_links_driver_bound
>        driver_bound
> -> #4 (device_links_lock){+.+.}-{3:3}:
>        __mutex_lock_common
>        mutex_lock_nested
>        device_link_remove
>        _regulator_put
>        regulator_put
>        devm_regulator_release
> ...
> -> #1 (&genpd->mlock){+.+.}-{3:3}:
>        __mutex_lock_common
>        mutex_lock_nested
>        genpd_lock_mtx
>        genpd_runtime_resume
>        __rpm_callback
>        rpm_callback
>        rpm_resume
>        __pm_runtime_resume
>        clk_core_prepare
>        clk_prepare
> -> #0 (prepare_lock){+.+.}-{3:3}:
>        __lock_acquire
>        lock_acquire
>        __mutex_lock_common
>        mutex_lock_nested
>        clk_prepare
>        scp_ipi_send
>        scp_send_ipi
>        mtk_rpmsg_send
>        rpmsg_send
>        cros_ec_pkt_xfer_rpmsg
>

From what I understand, cros_ec_cmd_xfer() gets called and takes @ec_dev->lock.
From there scp_ipi_send() and clk_prepare_enable() are eventually called.  The
latter takes @prepare_lock and proceeds to enable the mechanic that will get the
clock prepared.  The process to enable the clock mechanic, which may happen on
a different CPU, involves calling cros_ec_cmd_xfer() and lockdep complains
because @ec_dev->lock is already held. 
 
> > > Move clock prepare / unprepare operations to remoteproc prepare() /
> > > unprepare() callbacks to break the lock dependency from `ec_dev->lock`
> > > to `prepare_lock`.
> > 
> > With the information presented to me, I don't see how doing that changes
> > anything.  @prepare_lock is simply held for a longer period of time.  
> 
> In prepare() callback, the clock becomes prepared and prepare_lock won't be
> held after that.

If my theory (above) is correct, you are proposing to avoid the condition by
preparing the clock ahead of time before any IPI can take place.  Is this
correct?