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

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

[Tzung-Bi Shih]

`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).

Move clock prepare / unprepare operations to remoteproc prepare() /
unprepare() callbacks to break the lock dependency from `ec_dev->lock`
to `prepare_lock`.

This breaks the dependency chain in the lockdep report:
> WARNING: possible circular locking dependency detected
> ...
> the existing dependency chain (in reverse order) is:
> ...
> -> #0 (prepare_lock){+.+.}-{3:3}:
>        __lock_acquire
>        lock_acquire
>        __mutex_lock_common
>        mutex_lock_nested
>        clk_prepare
>        scp_ipi_send [mtk_scp_ipi]
>        scp_send_ipi [mtk_scp]
>        mtk_rpmsg_send [mtk_rpmsg]
>        rpmsg_send [rpmsg_core]
>        cros_ec_pkt_xfer_rpmsg [cros_ec_rpmsg]

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/remoteproc/mtk_scp.c     | 39 +++++++++++++++++++++++---------
 drivers/remoteproc/mtk_scp_ipi.c |  4 ++--
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index db8fd045468d..98d00bd5200c 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.351.gbe84eed79e-goog

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

[Mathieu Poirier]

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.

> 
> 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.  

Thanks,
Mathieu

> 
> This breaks the dependency chain in the lockdep report:
> > WARNING: possible circular locking dependency detected
> > ...
> > the existing dependency chain (in reverse order) is:
> > ...
> > -> #0 (prepare_lock){+.+.}-{3:3}:
> >        __lock_acquire
> >        lock_acquire
> >        __mutex_lock_common
> >        mutex_lock_nested
> >        clk_prepare
> >        scp_ipi_send [mtk_scp_ipi]
> >        scp_send_ipi [mtk_scp]
> >        mtk_rpmsg_send [mtk_rpmsg]
> >        rpmsg_send [rpmsg_core]
> >        cros_ec_pkt_xfer_rpmsg [cros_ec_rpmsg]
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/remoteproc/mtk_scp.c     | 39 +++++++++++++++++++++++---------
>  drivers/remoteproc/mtk_scp_ipi.c |  4 ++--
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index db8fd045468d..98d00bd5200c 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.351.gbe84eed79e-goog
> 

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

[Tzung-Bi Shih]

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

> > 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.

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

[Mathieu Poirier]

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?

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

[Tzung-Bi Shih]

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.

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

[Mathieu Poirier]

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?

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

[Bjorn Andersson]

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

Please suffix functions with (), instead of treating it just like any
other `symbol`, this makes it easier to see what is a function and what
is an object/variable.

> 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).

Can you please expand this explanation slightly. You're hinting that
there's an ABBA issue, but you're leaving it to the reader to know/guess
what that issue is.

I believe you have some concrete case where this is a probelm, please
include this, if nothing else as an example.

> 
> Move clock prepare / unprepare operations to remoteproc prepare() /
> unprepare() callbacks to break the lock dependency from `ec_dev->lock`
> to `prepare_lock`.

Please expand this with an argumentation that the prolonged "prepared
time" will not have any negative impact.

> 
> This breaks the dependency chain in the lockdep report:
> > WARNING: possible circular locking dependency detected
> > ...
> > the existing dependency chain (in reverse order) is:
> > ...
> > -> #0 (prepare_lock){+.+.}-{3:3}:
> >        __lock_acquire
> >        lock_acquire
> >        __mutex_lock_common
> >        mutex_lock_nested
> >        clk_prepare
> >        scp_ipi_send [mtk_scp_ipi]
> >        scp_send_ipi [mtk_scp]
> >        mtk_rpmsg_send [mtk_rpmsg]
> >        rpmsg_send [rpmsg_core]
> >        cros_ec_pkt_xfer_rpmsg [cros_ec_rpmsg]

If this is the AB case, can you please include a stack for the BA case
as well in the commit message?


With these things clarified, I expect that your patch is the correct way
to solve the problem.

Regards,
Bjorn

> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/remoteproc/mtk_scp.c     | 39 +++++++++++++++++++++++---------
>  drivers/remoteproc/mtk_scp_ipi.c |  4 ++--
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index db8fd045468d..98d00bd5200c 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.351.gbe84eed79e-goog
> 

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

[Tzung-Bi Shih]

On Sat, Jan 10, 2026 at 01:10:22PM -0600, Bjorn Andersson 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
> 
> Please suffix functions with (), instead of treating it just like any
> other `symbol`, this makes it easier to see what is a function and what
> is an object/variable.
> 
> > 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).
> 
> Can you please expand this explanation slightly. You're hinting that
> there's an ABBA issue, but you're leaving it to the reader to know/guess
> what that issue is.
> 
> I believe you have some concrete case where this is a probelm, please
> include this, if nothing else as an example.
> 
> > 
> > Move clock prepare / unprepare operations to remoteproc prepare() /
> > unprepare() callbacks to break the lock dependency from `ec_dev->lock`
> > to `prepare_lock`.
> 
> Please expand this with an argumentation that the prolonged "prepared
> time" will not have any negative impact.
> 
> > 
> > This breaks the dependency chain in the lockdep report:
> > > WARNING: possible circular locking dependency detected
> > > ...
> > > the existing dependency chain (in reverse order) is:
> > > ...
> > > -> #0 (prepare_lock){+.+.}-{3:3}:
> > >        __lock_acquire
> > >        lock_acquire
> > >        __mutex_lock_common
> > >        mutex_lock_nested
> > >        clk_prepare
> > >        scp_ipi_send [mtk_scp_ipi]
> > >        scp_send_ipi [mtk_scp]
> > >        mtk_rpmsg_send [mtk_rpmsg]
> > >        rpmsg_send [rpmsg_core]
> > >        cros_ec_pkt_xfer_rpmsg [cros_ec_rpmsg]
> 
> If this is the AB case, can you please include a stack for the BA case
> as well in the commit message?
> 
> 
> With these things clarified, I expect that your patch is the correct way
> to solve the problem.

An attempt: https://lore.kernel.org/r/20260112110755.2435899-1-tzungbi@kernel.org