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

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

On Wed, Jan 07, 2026 at 02:21:45AM +0000, Tzung-Bi Shih wrote:
> On Tue, Jan 06, 2026 at 10:10:27AM -0700, Mathieu Poirier wrote:
> > 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?
> 
> Correct, so that it doesn't need to prepare the clock (i.e., acquire the
> @prepare_lock) when @ec_dev->lock is held.

Is there anyone else that can review and test this patch?