remoteproc: mediatek: Break lock dependency to `prepare_lock`

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]

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:

Quoted 7 lines

WARNING: possible circular locking dependency detected
...
the existing dependency chain (in reverse order) is:
...
-> #0 (prepare_lock){+.+.}-{3:3}:
__lock_acquire
lock_acquire

Quoted 8 lines

__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 <[email protected]>

---
 drivers/remoteproc/mtk_scp.c     | 39 +++++++++++++++++++++++---------
 drivers/remoteproc/mtk_scp_ipi.c |  4 ++--
 2 files changed, 30 insertions(+), 13 deletions(-)

drivers/remoteproc/mtk_scp.c +28 -11

index db8fd045468d..98d00bd5200c 100644
@@ -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,

drivers/remoteproc/mtk_scp_ipi.c +2 -2

index c068227e251e..7a37e273b3af 100644
@@ -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;
 }

Signature · 1 line

2.52.0.351.gbe84eed79e-goog

Quoted 4 lines · 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).

What operation would that be? Please be specific so that I can trace the code.

Quoted 3 lines

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.

Quoted 7 lines · Mathieu Poirier

Quoted 4 lines · 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).

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 61 lines · Tzung-Bi Shih

Quoted 7 lines · Mathieu Poirier

Quoted 4 lines · 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).

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

Quoted 9 lines

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

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 89 lines · Mathieu Poirier

Quoted 61 lines · Tzung-Bi Shih

Quoted 7 lines · Mathieu Poirier

Quoted 4 lines · 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).

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

Quoted 9 lines

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

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

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:

Quoted 15 lines

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 <[email protected]>

---
 drivers/remoteproc/mtk_scp.c     | 39 +++++++++++++++++++++++---------
 drivers/remoteproc/mtk_scp_ipi.c |  4 ++--
 2 files changed, 30 insertions(+), 13 deletions(-)

drivers/remoteproc/mtk_scp.c +28 -11

index db8fd045468d..98d00bd5200c 100644
@@ -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,

drivers/remoteproc/mtk_scp_ipi.c +2 -2

index c068227e251e..7a37e273b3af 100644
@@ -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;
 }

Signature · 1 line

2.52.0.351.gbe84eed79e-goog

Quoted 1 line · Tzung-Bi Shih

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 17 lines

This breaks the dependency chain in the lockdep report:

Quoted 15 lines

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.