Skip to content

Commit 22cfa9e

Browse files
Tzung-Bi ShihSasha Levin
authored andcommitted
remoteproc: mediatek: Break lock dependency to prepare_lock
[ Upstream commit d935187cfb27fc4168f78f3959aef4eafaae76bb ] 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 microsoft#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 > -> microsoft#4 (device_links_lock) > device_link_remove > _regulator_put > regulator_put > -> microsoft#3 (regulator_list_mutex) > regulator_lock_dependent > regulator_disable > scpsys_power_off > _genpd_power_off > genpd_power_off > -> microsoft#2 (&genpd->mlock/1) > genpd_add_subdomain > pm_genpd_add_subdomain > scpsys_add_subdomain > scpsys_probe > -> microsoft#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@kernel.org> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> Tested-by: Chen-Yu Tsai <wenst@chromium.org> Link: https://lore.kernel.org/r/20260112110755.2435899-1-tzungbi@kernel.org Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Signed-off-by: Sasha Levin <sashal@kernel.org>
1 parent 34035dd commit 22cfa9e

2 files changed

Lines changed: 30 additions & 13 deletions

File tree

drivers/remoteproc/mtk_scp.c

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -282,15 +282,15 @@ static irqreturn_t scp_irq_handler(int irq, void *priv)
282282
struct mtk_scp *scp = priv;
283283
int ret;
284284

285-
ret = clk_prepare_enable(scp->clk);
285+
ret = clk_enable(scp->clk);
286286
if (ret) {
287287
dev_err(scp->dev, "failed to enable clocks\n");
288288
return IRQ_NONE;
289289
}
290290

291291
scp->data->scp_irq_handler(scp);
292292

293-
clk_disable_unprepare(scp->clk);
293+
clk_disable(scp->clk);
294294

295295
return IRQ_HANDLED;
296296
}
@@ -664,7 +664,7 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw)
664664
struct device *dev = scp->dev;
665665
int ret;
666666

667-
ret = clk_prepare_enable(scp->clk);
667+
ret = clk_enable(scp->clk);
668668
if (ret) {
669669
dev_err(dev, "failed to enable clocks\n");
670670
return ret;
@@ -679,7 +679,7 @@ static int scp_load(struct rproc *rproc, const struct firmware *fw)
679679

680680
ret = scp_elf_load_segments(rproc, fw);
681681
leave:
682-
clk_disable_unprepare(scp->clk);
682+
clk_disable(scp->clk);
683683

684684
return ret;
685685
}
@@ -690,14 +690,14 @@ static int scp_parse_fw(struct rproc *rproc, const struct firmware *fw)
690690
struct device *dev = scp->dev;
691691
int ret;
692692

693-
ret = clk_prepare_enable(scp->clk);
693+
ret = clk_enable(scp->clk);
694694
if (ret) {
695695
dev_err(dev, "failed to enable clocks\n");
696696
return ret;
697697
}
698698

699699
ret = scp_ipi_init(scp, fw);
700-
clk_disable_unprepare(scp->clk);
700+
clk_disable(scp->clk);
701701
return ret;
702702
}
703703

@@ -708,7 +708,7 @@ static int scp_start(struct rproc *rproc)
708708
struct scp_run *run = &scp->run;
709709
int ret;
710710

711-
ret = clk_prepare_enable(scp->clk);
711+
ret = clk_enable(scp->clk);
712712
if (ret) {
713713
dev_err(dev, "failed to enable clocks\n");
714714
return ret;
@@ -733,14 +733,14 @@ static int scp_start(struct rproc *rproc)
733733
goto stop;
734734
}
735735

736-
clk_disable_unprepare(scp->clk);
736+
clk_disable(scp->clk);
737737
dev_info(dev, "SCP is ready. FW version %s\n", run->fw_ver);
738738

739739
return 0;
740740

741741
stop:
742742
scp->data->scp_reset_assert(scp);
743-
clk_disable_unprepare(scp->clk);
743+
clk_disable(scp->clk);
744744
return ret;
745745
}
746746

@@ -908,20 +908,37 @@ static int scp_stop(struct rproc *rproc)
908908
struct mtk_scp *scp = rproc->priv;
909909
int ret;
910910

911-
ret = clk_prepare_enable(scp->clk);
911+
ret = clk_enable(scp->clk);
912912
if (ret) {
913913
dev_err(scp->dev, "failed to enable clocks\n");
914914
return ret;
915915
}
916916

917917
scp->data->scp_reset_assert(scp);
918918
scp->data->scp_stop(scp);
919-
clk_disable_unprepare(scp->clk);
919+
clk_disable(scp->clk);
920920

921921
return 0;
922922
}
923923

924+
static int scp_prepare(struct rproc *rproc)
925+
{
926+
struct mtk_scp *scp = rproc->priv;
927+
928+
return clk_prepare(scp->clk);
929+
}
930+
931+
static int scp_unprepare(struct rproc *rproc)
932+
{
933+
struct mtk_scp *scp = rproc->priv;
934+
935+
clk_unprepare(scp->clk);
936+
return 0;
937+
}
938+
924939
static const struct rproc_ops scp_ops = {
940+
.prepare = scp_prepare,
941+
.unprepare = scp_unprepare,
925942
.start = scp_start,
926943
.stop = scp_stop,
927944
.load = scp_load,

drivers/remoteproc/mtk_scp_ipi.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
171171
WARN_ON(len > scp_sizes->ipi_share_buffer_size) || WARN_ON(!buf))
172172
return -EINVAL;
173173

174-
ret = clk_prepare_enable(scp->clk);
174+
ret = clk_enable(scp->clk);
175175
if (ret) {
176176
dev_err(scp->dev, "failed to enable clock\n");
177177
return ret;
@@ -211,7 +211,7 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
211211

212212
unlock_mutex:
213213
mutex_unlock(&scp->send_lock);
214-
clk_disable_unprepare(scp->clk);
214+
clk_disable(scp->clk);
215215

216216
return ret;
217217
}

0 commit comments

Comments
 (0)