[PATCH v2] remoteproc: mediatek: Break lock dependency to `prepare_lock`
Mathieu Poirier
mathieu.poirier at linaro.org
Mon Jan 26 07:47:24 PST 2026
On Mon, Jan 12, 2026 at 11:07:55AM +0000, Tzung-Bi Shih wrote:
> A potential circular locking dependency (ABBA deadlock) exists between
> `ec_dev->lock` and the clock framework's `prepare_lock`.
>
> The first order (A -> B) occurs when scp_ipi_send() is called while
> `ec_dev->lock` is held (e.g., within cros_ec_cmd_xfer()):
> 1. cros_ec_cmd_xfer() acquires `ec_dev->lock` and calls scp_ipi_send().
> 2. scp_ipi_send() calls clk_prepare_enable(), which acquires
> `prepare_lock`.
> See #0 in the following example calling trace.
> (Lock Order: `ec_dev->lock` -> `prepare_lock`)
>
> The reverse order (B -> A) is more complex and has been observed
> (learned) by lockdep. It involves the clock prepare operation
> triggering power domain changes, which then propagates through sysfs
> and power supply uevents, eventually calling back into the ChromeOS EC
> driver and attempting to acquire `ec_dev->lock`:
> 1. Something calls clk_prepare(), which acquires `prepare_lock`. It
> then triggers genpd operations like genpd_runtime_resume(), which
> takes `&genpd->mlock`.
> 2. Power domain changes can trigger regulator changes; regulator
> changes can then trigger device link changes; device link changes
> can then trigger sysfs changes. Eventually, power_supply_uevent()
> is called.
> 3. This leads to calls like cros_usbpd_charger_get_prop(), which calls
> cros_ec_cmd_xfer_status(), which then attempts to acquire
> `ec_dev->lock`.
> See #1 ~ #6 in the following example calling trace.
> (Lock Order: `prepare_lock` -> `&genpd->mlock` -> ... -> `&ec_dev->lock`)
>
> Move the clk_prepare()/clk_unprepare() operations for `scp->clk` to the
> remoteproc prepare()/unprepare() callbacks. This ensures `prepare_lock`
> is only acquired in prepare()/unprepare() callbacks. Since
> `ec_dev->lock` is not involved in the callbacks, the dependency loop is
> broken.
>
> This means the clock is always "prepared" when the SCP is running. The
> prolonged "prepared time" for the clock should be acceptable as SCP is
> designed to be a very power efficient processor. The power consumption
> impact can be negligible.
>
> A simplified calling trace reported by lockdep:
> > -> #6 (&ec_dev->lock)
> > 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
> > -> #5 (kn->active#2)
> > kernfs_drain
> > __kernfs_remove
> > kernfs_remove_by_name_ns
> > sysfs_remove_file_ns
> > device_del
> > __device_link_del
> > device_links_driver_bound
> > -> #4 (device_links_lock)
> > device_link_remove
> > _regulator_put
> > regulator_put
> > -> #3 (regulator_list_mutex)
> > regulator_lock_dependent
> > regulator_disable
> > scpsys_power_off
> > _genpd_power_off
> > genpd_power_off
> > -> #2 (&genpd->mlock/1)
> > genpd_add_subdomain
> > pm_genpd_add_subdomain
> > scpsys_add_subdomain
> > scpsys_probe
> > -> #1 (&genpd->mlock)
> > genpd_runtime_resume
> > __rpm_callback
> > rpm_callback
> > rpm_resume
> > __pm_runtime_resume
> > clk_core_prepare
> > clk_prepare
> > -> #0 (prepare_lock)
> > clk_prepare
> > scp_ipi_send
> > scp_send_ipi
> > mtk_rpmsg_send
> > rpmsg_send
> > cros_ec_pkt_xfer_rpmsg
>
> Signed-off-by: Tzung-Bi Shih <tzungbi at kernel.org>
> ---
> v2:
> - Re-organized commit message.
> - Rebased to next-20260109.
>
> v1: https://lore.kernel.org/r/20251229043146.4102967-1-tzungbi@kernel.org
>
> drivers/remoteproc/mtk_scp.c | 39 +++++++++++++++++++++++---------
> drivers/remoteproc/mtk_scp_ipi.c | 4 ++--
> 2 files changed, 30 insertions(+), 13 deletions(-)
>
Applied.
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 328541e62158..4651311aeb07 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -283,7 +283,7 @@ static irqreturn_t scp_irq_handler(int irq, void *priv)
> struct mtk_scp *scp = priv;
> int ret;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(scp->dev, "failed to enable clocks\n");
> return IRQ_NONE;
> @@ -291,7 +291,7 @@ static irqreturn_t scp_irq_handler(int irq, void *priv)
>
> scp->data->scp_irq_handler(scp);
>
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
>
> return IRQ_HANDLED;
> }
> @@ -665,7 +665,7 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw)
> struct device *dev = scp->dev;
> int ret;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(dev, "failed to enable clocks\n");
> return ret;
> @@ -680,7 +680,7 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw)
>
> ret = scp_elf_load_segments(rproc, fw);
> leave:
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
>
> return ret;
> }
> @@ -691,14 +691,14 @@ static int scp_parse_fw(struct rproc *rproc, const struct firmware *fw)
> struct device *dev = scp->dev;
> int ret;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(dev, "failed to enable clocks\n");
> return ret;
> }
>
> ret = scp_ipi_init(scp, fw);
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
> return ret;
> }
>
> @@ -709,7 +709,7 @@ static int scp_start(struct rproc *rproc)
> struct scp_run *run = &scp->run;
> int ret;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(dev, "failed to enable clocks\n");
> return ret;
> @@ -734,14 +734,14 @@ static int scp_start(struct rproc *rproc)
> goto stop;
> }
>
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
> dev_info(dev, "SCP is ready. FW version %s\n", run->fw_ver);
>
> return 0;
>
> stop:
> scp->data->scp_reset_assert(scp);
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
> return ret;
> }
>
> @@ -909,7 +909,7 @@ static int scp_stop(struct rproc *rproc)
> struct mtk_scp *scp = rproc->priv;
> int ret;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(scp->dev, "failed to enable clocks\n");
> return ret;
> @@ -917,12 +917,29 @@ static int scp_stop(struct rproc *rproc)
>
> scp->data->scp_reset_assert(scp);
> scp->data->scp_stop(scp);
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
>
> return 0;
> }
>
> +static int scp_prepare(struct rproc *rproc)
> +{
> + struct mtk_scp *scp = rproc->priv;
> +
> + return clk_prepare(scp->clk);
> +}
> +
> +static int scp_unprepare(struct rproc *rproc)
> +{
> + struct mtk_scp *scp = rproc->priv;
> +
> + clk_unprepare(scp->clk);
> + return 0;
> +}
> +
> static const struct rproc_ops scp_ops = {
> + .prepare = scp_prepare,
> + .unprepare = scp_unprepare,
> .start = scp_start,
> .stop = scp_stop,
> .load = scp_load,
> diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
> index c068227e251e..7a37e273b3af 100644
> --- a/drivers/remoteproc/mtk_scp_ipi.c
> +++ b/drivers/remoteproc/mtk_scp_ipi.c
> @@ -171,7 +171,7 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
> WARN_ON(len > scp_sizes->ipi_share_buffer_size) || WARN_ON(!buf))
> return -EINVAL;
>
> - ret = clk_prepare_enable(scp->clk);
> + ret = clk_enable(scp->clk);
> if (ret) {
> dev_err(scp->dev, "failed to enable clock\n");
> return ret;
> @@ -211,7 +211,7 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
>
> unlock_mutex:
> mutex_unlock(&scp->send_lock);
> - clk_disable_unprepare(scp->clk);
> + clk_disable(scp->clk);
>
> return ret;
> }
> --
> 2.52.0.457.g6b5491de43-goog
>
>
More information about the Linux-mediatek
mailing list