[PATCH 0/3] ASoC: qcom: qdsp6: Fix audio after an ADSP restart

[PATCH 0/3] ASoC: qcom: qdsp6: Fix audio after an ADSP restart

[Shawn Guo]

On Qualcomm Nord, audio does not come back after an ADSP restart
(echo stop/start > /sys/class/remoteproc/remoteproc0/state). There are
two separate problems:

 - The sound card takes 5 seconds to come back. The q6apm GPR service
   probes about 0.5 ms after remoteproc reports the DSP up, which is
   too early for SPF. SPF drops the command without replying, and
   apm_probe() waits out the 5 second command timeout before the card
   can bind. If the card binds earlier than that, its shared memory map
   times out and the card fails to come up.

 - Once the card is back, playback fails:

     pcm1681 0-004c: Failed to enable sck: -6

   The q6prm clock controller is a child of the q6prm GPR device, so it
   is unregistered when the DSP stops. Its consumers stay bound, and
   their clk handles are left pointing at clk_nodrv_ops. In addition,
   the DSP forgets every clock request and vote when it stops, so
   clocks that consumers held prepared across the restart are no
   longer running.

Patch 1 polls SPF with a short per-command timeout before q6apm
registers its component.

Patch 2 makes q6dsp-lpass-clocks track prepared clocks, and adds
q6dsp_clock_dsp_down()/q6dsp_clock_dsp_up() so the DSP service driver
can mark them lost and request them again. On its own, it does not
change behaviour.

Patch 3 makes q6prm keep the clock controller device across DSP
restarts, parented to the remoteproc platform device, and calls the
new helpers from its probe and remove functions. No DT or binding
changes are needed.

Shawn Guo (3):
  ASoC: qcom: q6apm: Wait for SPF to be ready before registering
  ASoC: qcom: q6dsp-lpass-clocks: Restore clocks after a DSP restart
  ASoC: qcom: q6prm: Keep the clock controller across DSP restarts

 sound/soc/qcom/Kconfig                    |   1 +
 sound/soc/qcom/qdsp6/q6apm.c              |  50 +++++-
 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c | 147 ++++++++++++++---
 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h |   2 +
 sound/soc/qcom/qdsp6/q6prm.c              | 190 ++++++++++++++++++++--
 5 files changed, 350 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH 1/3] ASoC: qcom: q6apm: Wait for SPF to be ready before registering

[Shawn Guo]

apm_probe() sends APM_CMD_GET_SPF_STATE once and ignores the result.
At cold boot this is harmless: module loading and PDR lookup delay the
probe by tens of milliseconds after the DSP is up, so SPF is listening
by the time the command is sent.

After an ADSP restart, everything is already loaded and PDR reports the
audio PD up at once. The GPR service is added about 0.5 ms after the
remoteproc reports the DSP up, and SPF drops a command sent that early
without replying:

  remoteproc remoteproc0: remote processor adsp is now up
  qcom,apr ...: Adding APR/GPR dev: gprsvc:service:2:1
  qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode

Once the q6apm component is registered, a sound card whose other
components are still bound can bind right away, inside apm_probe(), and
issue APM_CMD_SHARED_MEM_MAP_REGIONS before SPF can handle it. The map
times out and the card fails to come up:

  qcom-apm gprsvc:service:2:1: CMD timeout for [100100c] opcode

Poll GET_SPF_STATE before registering the component. Simply retrying
q6apm_get_apm_state() is not enough: it goes through
audioreach_send_cmd_sync(), whose 5 second timeout makes every dropped
command stall the probe, and the card with it, for 5 seconds. Instead,
send the command directly and wait only 100 ms for the reply, for up to
5 seconds in total, which is the worst case of the single command sent
before. A timed out try is retried at once, while a "not ready" reply
or a send failure backs off for 100 ms first. On Nord, SPF answers
within 100-200 ms of the restart. If it still does not answer, warn and
carry on as before rather than failing the probe.

Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6apm.c | 50 +++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
index 4cb597d737b1..e7ba6bc93142 100644
--- a/sound/soc/qcom/qdsp6/q6apm.c
+++ b/sound/soc/qcom/qdsp6/q6apm.c
@@ -26,6 +26,8 @@ struct apm_graph_mgmt_cmd {
 } __packed;
 
 #define APM_GRAPH_MGMT_PSIZE(p, n) ALIGN(struct_size(p, sub_graph_id_list, n), 8)
+#define Q6APM_SPF_POLL_MS	100
+#define Q6APM_SPF_READY_MS	5000
 
 static struct q6apm *g_apm;
 
@@ -154,6 +156,47 @@ static int q6apm_get_apm_state(struct q6apm *apm)
 	return apm->state;
 }
 
+/*
+ * Right after the DSP boots, the SPF drops commands without replying. Poll
+ * GET_SPF_STATE with a short timeout per try instead of the 5 s one of
+ * q6apm_send_cmd_sync(), so a dropped command does not stall the probe.
+ */
+static bool q6apm_wait_spf_ready(struct q6apm *apm)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(Q6APM_SPF_READY_MS);
+	gpr_device_t *gdev = apm->gdev;
+
+	do {
+		struct gpr_pkt *pkt __free(kfree) =
+			audioreach_alloc_apm_cmd_pkt(0, APM_CMD_GET_SPF_STATE, 0);
+		long rc;
+
+		if (IS_ERR(pkt))
+			return false;
+
+		pkt->hdr.dest_domain = audioreach_gpr_dest_domain(gdev);
+
+		mutex_lock(&apm->lock);
+		apm->result.opcode = 0;
+		apm->state = 0;
+		rc = gpr_send_pkt(gdev, pkt);
+		if (rc >= 0)
+			rc = wait_event_timeout(apm->wait,
+					apm->result.opcode == APM_CMD_RSP_GET_SPF_STATE,
+					msecs_to_jiffies(Q6APM_SPF_POLL_MS));
+		mutex_unlock(&apm->lock);
+
+		if (apm->state)
+			return true;
+
+		/* A timeout has already waited, anything else backs off */
+		if (rc)
+			msleep(Q6APM_SPF_POLL_MS);
+	} while (time_before(jiffies, timeout));
+
+	return false;
+}
+
 bool q6apm_is_adsp_ready(void)
 {
 	if (g_apm)
@@ -906,7 +949,12 @@ static int apm_probe(gpr_device_t *gdev)
 	dev_set_drvdata(dev, apm);
 	g_apm = apm;
 
-	q6apm_get_apm_state(apm);
+	/*
+	 * Once the component is registered a card can bind and send commands
+	 * at once, so wait for the SPF first.
+	 */
+	if (!q6apm_wait_spf_ready(apm))
+		dev_warn(dev, "SPF not ready\n");
 
 	ret = snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
 	if (ret < 0) {
-- 
2.43.0

[PATCH 2/3] ASoC: qcom: q6dsp-lpass-clocks: Restore clocks after a DSP restart

[Shawn Guo]

The DSP forgets all clock requests and votes when it stops. A consumer
that still holds a clock prepared across a DSP restart then holds a
clock that the DSP no longer provides, and its later unprepare releases
a request the DSP never saw.

Track which clocks are prepared, and add q6dsp_clock_dsp_down() and
q6dsp_clock_dsp_up() for the DSP service driver to call when the DSP
goes away and comes back. dsp_down() marks the prepared clocks as lost.
dsp_up() requests them again at their current rate, or votes for them
again. Unpreparing a lost clock skips the release, since the DSP holds
nothing for it.

Fold the prepare/unprepare ops of rate and vote clocks into one pair,
so both kinds share the request/release helpers used by the restore
path. A mutex serializes prepare/unprepare against dsp_down/up.

This patch does not change behaviour on its own: no driver calls the
new functions yet.

Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c | 147 ++++++++++++++++++----
 sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h |   2 +
 2 files changed, 123 insertions(+), 26 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
index 94d96130245e..22d3f3c6d46f 100644
--- a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
+++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.c
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/slab.h>
 #include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
@@ -22,6 +23,10 @@ struct q6dsp_clk {
 	int attributes;
 	int rate;
 	uint32_t handle;
+	bool vote;
+	bool prepared;
+	/* Still prepared, but dropped by the DSP when it stopped */
+	bool lost;
 	struct clk_hw hw;
 };
 
@@ -29,17 +34,45 @@ struct q6dsp_clk {
 
 struct q6dsp_cc {
 	struct device *dev;
+	/* Serializes clock requests against DSP down/up */
+	struct mutex lock;
 	struct q6dsp_clk *clks[Q6DSP_MAX_CLK_ID];
 	const struct q6dsp_clk_desc *desc;
 };
 
+static int q6dsp_clk_request(struct q6dsp_cc *cc, struct q6dsp_clk *clk)
+{
+	if (clk->vote)
+		return cc->desc->lpass_vote_clk(clk->dev, clk->q6dsp_clk_id,
+						clk_hw_get_name(&clk->hw),
+						&clk->handle);
+
+	return cc->desc->lpass_set_clk(clk->dev, clk->q6dsp_clk_id, clk->attributes,
+				       Q6DSP_LPASS_CLK_ROOT_DEFAULT, clk->rate);
+}
+
+static void q6dsp_clk_release(struct q6dsp_cc *cc, struct q6dsp_clk *clk)
+{
+	if (clk->vote)
+		cc->desc->lpass_unvote_clk(clk->dev, clk->q6dsp_clk_id, clk->handle);
+	else
+		cc->desc->lpass_set_clk(clk->dev, clk->q6dsp_clk_id, clk->attributes,
+					Q6DSP_LPASS_CLK_ROOT_DEFAULT, 0);
+}
+
 static int clk_q6dsp_prepare(struct clk_hw *hw)
 {
 	struct q6dsp_clk *clk = to_q6dsp_clk(hw);
 	struct q6dsp_cc *cc = dev_get_drvdata(clk->dev);
+	int ret;
 
-	return cc->desc->lpass_set_clk(clk->dev, clk->q6dsp_clk_id, clk->attributes,
-				     Q6DSP_LPASS_CLK_ROOT_DEFAULT, clk->rate);
+	guard(mutex)(&cc->lock);
+
+	ret = q6dsp_clk_request(cc, clk);
+	if (!ret)
+		clk->prepared = true;
+
+	return ret;
 }
 
 static void clk_q6dsp_unprepare(struct clk_hw *hw)
@@ -47,8 +80,14 @@ static void clk_q6dsp_unprepare(struct clk_hw *hw)
 	struct q6dsp_clk *clk = to_q6dsp_clk(hw);
 	struct q6dsp_cc *cc = dev_get_drvdata(clk->dev);
 
-	cc->desc->lpass_set_clk(clk->dev, clk->q6dsp_clk_id, clk->attributes,
-			      Q6DSP_LPASS_CLK_ROOT_DEFAULT, 0);
+	guard(mutex)(&cc->lock);
+
+	/* A lost clock is not held by the DSP, so there is nothing to release */
+	if (!clk->lost)
+		q6dsp_clk_release(cc, clk);
+
+	clk->prepared = false;
+	clk->lost = false;
 }
 
 static int clk_q6dsp_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -83,26 +122,9 @@ static const struct clk_ops clk_q6dsp_ops = {
 	.recalc_rate	= clk_q6dsp_recalc_rate,
 };
 
-static int clk_vote_q6dsp_block(struct clk_hw *hw)
-{
-	struct q6dsp_clk *clk = to_q6dsp_clk(hw);
-	struct q6dsp_cc *cc = dev_get_drvdata(clk->dev);
-
-	return cc->desc->lpass_vote_clk(clk->dev, clk->q6dsp_clk_id,
-				  clk_hw_get_name(&clk->hw), &clk->handle);
-}
-
-static void clk_unvote_q6dsp_block(struct clk_hw *hw)
-{
-	struct q6dsp_clk *clk = to_q6dsp_clk(hw);
-	struct q6dsp_cc *cc = dev_get_drvdata(clk->dev);
-
-	cc->desc->lpass_unvote_clk(clk->dev, clk->q6dsp_clk_id, clk->handle);
-}
-
 static const struct clk_ops clk_vote_q6dsp_ops = {
-	.prepare	= clk_vote_q6dsp_block,
-	.unprepare	= clk_unvote_q6dsp_block,
+	.prepare	= clk_q6dsp_prepare,
+	.unprepare	= clk_q6dsp_unprepare,
 };
 
 
@@ -146,6 +168,10 @@ int q6dsp_clock_dev_probe(struct platform_device *pdev)
 	cc->dev = dev;
 	q6dsp_clks = desc->clks;
 
+	ret = devm_mutex_init(dev, &cc->lock);
+	if (ret)
+		return ret;
+
 	for (i = 0; i < desc->num_clks; i++) {
 		unsigned int id = q6dsp_clks[i].clk_id;
 		struct clk_init_data init = {
@@ -160,12 +186,13 @@ int q6dsp_clock_dev_probe(struct platform_device *pdev)
 		clk->dev = dev;
 		clk->q6dsp_clk_id = q6dsp_clks[i].q6dsp_clk_id;
 		clk->rate = q6dsp_clks[i].rate;
+		clk->vote = !clk->rate;
 		clk->hw.init = &init;
 
-		if (clk->rate)
-			init.ops = &clk_q6dsp_ops;
-		else
+		if (clk->vote)
 			init.ops = &clk_vote_q6dsp_ops;
+		else
+			init.ops = &clk_q6dsp_ops;
 
 		cc->clks[id] = clk;
 
@@ -183,3 +210,71 @@ int q6dsp_clock_dev_probe(struct platform_device *pdev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(q6dsp_clock_dev_probe);
+
+/**
+ * q6dsp_clock_dsp_down() - Mark the clocks dropped by a stopped DSP
+ * @dev: clock controller device
+ *
+ * The DSP forgets all clock requests when it stops. Remember the clocks that
+ * consumers still hold prepared, so q6dsp_clock_dsp_up() can request them
+ * again once the DSP is back.
+ */
+void q6dsp_clock_dsp_down(struct device *dev)
+{
+	struct q6dsp_cc *cc;
+	int i;
+
+	guard(device)(dev);
+
+	cc = dev_get_drvdata(dev);
+	if (!cc)
+		return;
+
+	guard(mutex)(&cc->lock);
+
+	for (i = 0; i < Q6DSP_MAX_CLK_ID; i++) {
+		struct q6dsp_clk *clk = cc->clks[i];
+
+		if (clk && clk->prepared)
+			clk->lost = true;
+	}
+}
+EXPORT_SYMBOL_GPL(q6dsp_clock_dsp_down);
+
+/**
+ * q6dsp_clock_dsp_up() - Restore the clocks dropped by a stopped DSP
+ * @dev: clock controller device
+ *
+ * Request again, at their current rate, the clocks that were still prepared
+ * when the DSP stopped.
+ */
+void q6dsp_clock_dsp_up(struct device *dev)
+{
+	struct q6dsp_cc *cc;
+	int i, ret;
+
+	guard(device)(dev);
+
+	cc = dev_get_drvdata(dev);
+	if (!cc)
+		return;
+
+	guard(mutex)(&cc->lock);
+
+	for (i = 0; i < Q6DSP_MAX_CLK_ID; i++) {
+		struct q6dsp_clk *clk = cc->clks[i];
+
+		if (!clk || !clk->lost)
+			continue;
+
+		ret = q6dsp_clk_request(cc, clk);
+		if (ret) {
+			dev_err(dev, "Failed to restore %s: %d\n",
+				clk_hw_get_name(&clk->hw), ret);
+			continue;
+		}
+
+		clk->lost = false;
+	}
+}
+EXPORT_SYMBOL_GPL(q6dsp_clock_dsp_up);
diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h
index 3770d81f2bd6..e4655a0b77c6 100644
--- a/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h
+++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-clocks.h
@@ -26,5 +26,7 @@ struct q6dsp_clk_desc {
 };
 
 int q6dsp_clock_dev_probe(struct platform_device *pdev);
+void q6dsp_clock_dsp_down(struct device *dev);
+void q6dsp_clock_dsp_up(struct device *dev);
 
 #endif  /* __Q6DSP_AUDIO_CLOCKS_H__ */
-- 
2.43.0

[PATCH 3/3] ASoC: qcom: q6prm: Keep the clock controller across DSP restarts

[Shawn Guo]

The q6prm LPASS clock controller is populated as a child of the q6prm
GPR device. That device is unregistered whenever the DSP stops, which
unregisters the clock provider while consumers still hold their clocks.
fw_devlink links the consumers to the remoteproc device rather than to
the clock controller, so they stay bound, and their clk handles fall
back to clk_nodrv_ops. After the DSP comes back, every operation on
those handles fails with -ENXIO. On Nord RRD, playback after an ADSP
restart fails with:

  pcm1681 0-004c: Failed to enable sck: -6

Create the clock controller device once and keep it until the module is
unloaded. Its parent is the closest platform device ancestor of the GPR
device, which is the remoteproc platform device. That device persists
across DSP restarts, while the rpmsg and remoteproc devices between it
and the GPR device do not. The device tree is unchanged: the controller
keeps its clock-controller node under the q6prm node, so no binding
change is needed.

Each probe of the q6prm GPR device attaches its instance to the existing
controller, found by device tree node, and restores the clocks that
consumers held across the restart. Remove detaches the instance first,
so new clock requests fail with -EBUSY while the DSP is down, then marks
the prepared clocks as lost.

Since q6prm now destroys these devices itself, replace
module_gpr_driver() with explicit init and exit functions. q6prm also
calls into snd-q6dsp-common now, so select SND_SOC_QDSP6_COMMON.

Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
---
 sound/soc/qcom/Kconfig       |   1 +
 sound/soc/qcom/qdsp6/q6prm.c | 190 ++++++++++++++++++++++++++++++++---
 2 files changed, 178 insertions(+), 13 deletions(-)

diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index f6d810931a9a..74f0190b05f1 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -127,6 +127,7 @@ config SND_SOC_QDSP6_PRM_LPASS_CLOCKS
 
 config SND_SOC_QDSP6_PRM
 	tristate
+	select SND_SOC_QDSP6_COMMON
 	select SND_SOC_QDSP6_PRM_LPASS_CLOCKS
 
 config SND_SOC_QCOM_OFFLOAD_UTILS
diff --git a/sound/soc/qcom/qdsp6/q6prm.c b/sound/soc/qcom/qdsp6/q6prm.c
index 73b5a96e8311..861d48bdb5ea 100644
--- a/sound/soc/qcom/qdsp6/q6prm.c
+++ b/sound/soc/qcom/qdsp6/q6prm.c
@@ -8,22 +8,56 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/jiffies.h>
 #include <linux/soc/qcom/apr.h>
 #include <dt-bindings/soc/qcom,gpr.h>
 #include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
 #include "q6apm.h"
+#include "q6dsp-lpass-clocks.h"
 #include "q6prm.h"
 #include "audioreach.h"
 
+struct q6prm_clkctl;
+
 struct q6prm {
 	struct device *dev;
 	gpr_device_t *gdev;
 	wait_queue_head_t wait;
 	struct gpr_ibasic_rsp_result_t result;
 	struct mutex lock;
+	struct q6prm_clkctl *clkctl;
 };
 
+/*
+ * The q6prm GPR device is removed whenever the DSP stops, but clock consumers
+ * hold their clk handles across a DSP restart. The clock controller device is
+ * therefore created only once, under the remoteproc device, and kept until the
+ * module is unloaded. Its clock ops reach the q6prm instance of the running
+ * DSP through @prm, which is NULL while the DSP is down.
+ */
+struct q6prm_clkctl {
+	struct list_head node;
+	struct device_node *np;
+	struct platform_device *pdev;
+	struct q6prm *prm;
+};
+
+static DEFINE_MUTEX(q6prm_clkctl_lock);
+static LIST_HEAD(q6prm_clkctl_list);
+
+/* Called with q6prm_clkctl_lock held */
+static struct q6prm *q6prm_clkctl_get_prm(struct device *dev)
+{
+	struct q6prm_clkctl *ctl;
+
+	list_for_each_entry(ctl, &q6prm_clkctl_list, node)
+		if (ctl->np == dev->of_node)
+			return ctl->prm;
+
+	return NULL;
+}
+
 #define PRM_CMD_REQUEST_HW_RSC		0x0100100F
 #define PRM_CMD_RSP_REQUEST_HW_RSC	0x02001002
 #define PRM_CMD_RELEASE_HW_RSC		0x01001010
@@ -63,9 +97,8 @@ static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt, uint32_t
 					NULL, &prm->wait, pkt, rsp_opcode);
 }
 
-static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id, bool enable)
+static int q6prm_set_hw_core_req(struct q6prm *prm, uint32_t hw_block_id, bool enable)
 {
-	struct q6prm *prm = dev_get_drvdata(dev->parent);
 	struct apm_module_param_data *param_data;
 	struct prm_cmd_request_hw_core *req;
 	gpr_device_t *gdev = prm->gdev;
@@ -105,21 +138,35 @@ static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id, bool
 int q6prm_vote_lpass_core_hw(struct device *dev, uint32_t hw_block_id,
 			     const char *client_name, uint32_t *client_handle)
 {
-	return q6prm_set_hw_core_req(dev, hw_block_id, true);
+	struct q6prm *prm;
 
+	guard(mutex)(&q6prm_clkctl_lock);
+
+	prm = q6prm_clkctl_get_prm(dev);
+	if (!prm)
+		return -EBUSY;
+
+	return q6prm_set_hw_core_req(prm, hw_block_id, true);
 }
 EXPORT_SYMBOL_GPL(q6prm_vote_lpass_core_hw);
 
 int q6prm_unvote_lpass_core_hw(struct device *dev, uint32_t hw_block_id, uint32_t client_handle)
 {
-	return q6prm_set_hw_core_req(dev, hw_block_id, false);
+	struct q6prm *prm;
+
+	guard(mutex)(&q6prm_clkctl_lock);
+
+	prm = q6prm_clkctl_get_prm(dev);
+	if (!prm)
+		return -EBUSY;
+
+	return q6prm_set_hw_core_req(prm, hw_block_id, false);
 }
 EXPORT_SYMBOL_GPL(q6prm_unvote_lpass_core_hw);
 
-static int q6prm_request_lpass_clock(struct device *dev, int clk_id, int clk_attr, int clk_root,
+static int q6prm_request_lpass_clock(struct q6prm *prm, int clk_id, int clk_attr, int clk_root,
 				     unsigned int freq)
 {
-	struct q6prm *prm = dev_get_drvdata(dev->parent);
 	struct apm_module_param_data *param_data;
 	struct prm_cmd_request_rsc *req;
 	gpr_device_t *gdev = prm->gdev;
@@ -148,10 +195,9 @@ static int q6prm_request_lpass_clock(struct device *dev, int clk_id, int clk_att
 	return q6prm_send_cmd_sync(prm, pkt, PRM_CMD_RSP_REQUEST_HW_RSC);
 }
 
-static int q6prm_release_lpass_clock(struct device *dev, int clk_id, int clk_attr, int clk_root,
+static int q6prm_release_lpass_clock(struct q6prm *prm, int clk_id, int clk_attr, int clk_root,
 			  unsigned int freq)
 {
-	struct q6prm *prm = dev_get_drvdata(dev->parent);
 	struct apm_module_param_data *param_data;
 	struct prm_cmd_release_rsc *rel;
 	gpr_device_t *gdev = prm->gdev;
@@ -180,10 +226,18 @@ static int q6prm_release_lpass_clock(struct device *dev, int clk_id, int clk_att
 int q6prm_set_lpass_clock(struct device *dev, int clk_id, int clk_attr, int clk_root,
 			  unsigned int freq)
 {
-	if (freq)
-		return q6prm_request_lpass_clock(dev, clk_id, clk_attr, clk_root, freq);
+	struct q6prm *prm;
 
-	return q6prm_release_lpass_clock(dev, clk_id, clk_attr, clk_root, freq);
+	guard(mutex)(&q6prm_clkctl_lock);
+
+	prm = q6prm_clkctl_get_prm(dev);
+	if (!prm)
+		return -EBUSY;
+
+	if (freq)
+		return q6prm_request_lpass_clock(prm, clk_id, clk_attr, clk_root, freq);
+
+	return q6prm_release_lpass_clock(prm, clk_id, clk_attr, clk_root, freq);
 }
 EXPORT_SYMBOL_GPL(q6prm_set_lpass_clock);
 
@@ -212,10 +266,76 @@ static int prm_callback(const struct gpr_resp_pkt *data, void *priv, int op)
 	return 0;
 }
 
+/*
+ * The GPR device sits on a chain of rpmsg and remoteproc devices that are all
+ * torn down when the DSP stops. Its closest platform device ancestor is the
+ * remoteproc platform device, which stays across DSP restarts.
+ */
+static struct device *q6prm_clkctl_parent(struct device *dev)
+{
+	for (dev = dev->parent; dev; dev = dev->parent)
+		if (dev_is_platform(dev))
+			return dev;
+
+	return NULL;
+}
+
+static int q6prm_clkctl_attach(struct q6prm *prm)
+{
+	struct device *dev = prm->dev;
+	struct q6prm_clkctl *ctl = NULL, *iter;
+	struct device_node *np;
+
+	np = of_get_child_by_name(dev->of_node, "clock-controller");
+	if (!np)
+		return 0;
+
+	scoped_guard(mutex, &q6prm_clkctl_lock) {
+		list_for_each_entry(iter, &q6prm_clkctl_list, node) {
+			if (iter->np == np) {
+				ctl = iter;
+				break;
+			}
+		}
+
+		if (ctl) {
+			of_node_put(np);
+		} else {
+			ctl = kzalloc_obj(*ctl);
+			if (!ctl) {
+				of_node_put(np);
+				return -ENOMEM;
+			}
+
+			ctl->np = np;
+			list_add(&ctl->node, &q6prm_clkctl_list);
+		}
+
+		ctl->prm = prm;
+	}
+
+	/* Created on first probe, once its clock ops can already reach the DSP */
+	if (!ctl->pdev) {
+		ctl->pdev = of_platform_device_create(ctl->np, NULL,
+						      q6prm_clkctl_parent(dev));
+		if (!ctl->pdev) {
+			scoped_guard(mutex, &q6prm_clkctl_lock)
+				ctl->prm = NULL;
+			return dev_err_probe(dev, -ENODEV,
+					     "Failed to create clock controller\n");
+		}
+	}
+
+	prm->clkctl = ctl;
+
+	return 0;
+}
+
 static int prm_probe(gpr_device_t *gdev)
 {
 	struct device *dev = &gdev->dev;
 	struct q6prm *cc;
+	int ret;
 
 	cc = devm_kzalloc_obj(dev, *cc);
 	if (!cc)
@@ -230,7 +350,29 @@ static int prm_probe(gpr_device_t *gdev)
 	if (!q6apm_is_adsp_ready())
 		return -EPROBE_DEFER;
 
-	return devm_of_platform_populate(dev);
+	ret = q6prm_clkctl_attach(cc);
+	if (ret)
+		return ret;
+
+	if (cc->clkctl)
+		q6dsp_clock_dsp_up(&cc->clkctl->pdev->dev);
+
+	return 0;
+}
+
+static void prm_remove(gpr_device_t *gdev)
+{
+	struct q6prm *prm = dev_get_drvdata(&gdev->dev);
+	struct q6prm_clkctl *ctl = prm->clkctl;
+
+	if (!ctl)
+		return;
+
+	/* Fail new clock requests first, then mark what the DSP dropped */
+	scoped_guard(mutex, &q6prm_clkctl_lock)
+		ctl->prm = NULL;
+
+	q6dsp_clock_dsp_down(&ctl->pdev->dev);
 }
 
 #ifdef CONFIG_OF
@@ -243,6 +385,7 @@ MODULE_DEVICE_TABLE(of, prm_device_id);
 
 static gpr_driver_t prm_driver = {
 	.probe = prm_probe,
+	.remove = prm_remove,
 	.gpr_callback = prm_callback,
 	.driver = {
 		.name = "qcom-prm",
@@ -250,6 +393,27 @@ static gpr_driver_t prm_driver = {
 	},
 };
 
-module_gpr_driver(prm_driver);
+static int __init q6prm_init(void)
+{
+	return apr_driver_register(&prm_driver);
+}
+module_init(q6prm_init);
+
+static void __exit q6prm_exit(void)
+{
+	struct q6prm_clkctl *ctl, *tmp;
+
+	apr_driver_unregister(&prm_driver);
+
+	list_for_each_entry_safe(ctl, tmp, &q6prm_clkctl_list, node) {
+		if (ctl->pdev)
+			of_platform_device_destroy(&ctl->pdev->dev, NULL);
+		of_node_put(ctl->np);
+		list_del(&ctl->node);
+		kfree(ctl);
+	}
+}
+module_exit(q6prm_exit);
+
 MODULE_DESCRIPTION("Q6 Proxy Resource Manager");
 MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 3/3] ASoC: qcom: q6prm: Keep the clock controller across DSP restarts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] ASoC: qcom: q6prm: clock restore in prm_probe() times out
  before callback setup
--

commit 02c1625331bd9fcb6147637d9b08efb05abcc888
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

ASoC: qcom: q6prm: Keep the clock controller across DSP restarts

This commit keeps the q6prm LPASS clock controller platform device alive
across DSP restarts and restores prepared clocks when the q6prm GPR device
probes again.

> diff --git a/sound/soc/qcom/qdsp6/q6prm.c b/sound/soc/qcom/qdsp6/q6prm.c
> index 73b5a96e83119..861d48bdb5eaf 100644
> --- a/sound/soc/qcom/qdsp6/q6prm.c
> +++ b/sound/soc/qcom/qdsp6/q6prm.c
[ ... ]
> @@ -230,7 +350,29 @@ static int prm_probe(gpr_device_t *gdev)
>  	if (!q6apm_is_adsp_ready())
>  		return -EPROBE_DEFER;
>  
> -	return devm_of_platform_populate(dev);
> +	ret = q6prm_clkctl_attach(cc);
> +	if (ret)
> +		return ret;
> +
> +	if (cc->clkctl)
> +		q6dsp_clock_dsp_up(&cc->clkctl->pdev->dev);
> +
> +	return 0;
> +}

[Severity: High]
Will q6dsp_clock_dsp_up() always time out when restoring lost clocks inside
prm_probe()?

apr_device_probe() only installs adrv->gpr_callback into adev->svc.callback
after adrv->probe() returns 0:

drivers/soc/qcom/apr.c:apr_device_probe() {
    ...
	ret = adrv->probe(adev);
	if (!ret)
		adev->svc.callback = adrv->gpr_callback;
    ...
}

When q6dsp_clock_dsp_up() sends PRM_CMD_REQUEST_HW_RSC via
q6prm_send_cmd_sync() -> audioreach_send_cmd_sync() and waits on prm->wait,
gdev->svc.callback is still NULL, so gpr_do_rx_callback() drops the DSP
response without calling prm_callback(). Every lost clock waits 5 seconds
and fails with -ETIMEDOUT, leaving clk->lost set. The same happens if a
consumer requests a clock after q6prm_clkctl_attach() sets ctl->prm and
calls of_platform_device_create() before prm_probe() returns.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115032027.414953-1-shengchao.guo@oss.qualcomm.com?part=3

Re: [PATCH 1/3] ASoC: qcom: q6apm: Wait for SPF to be ready before registering

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: q6apm: unsynchronized state polling in
  q6apm_wait_spf_ready()
- [Low] ASoC: qcom: q6apm: missing Fixes trailer for apm_probe()
  readiness fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] ASoC: qdsp6: use-after-free in apm_probe() [1]
--

commit 57c0d12f224a6a066f01b2ff52ff14b9359892f8
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

> ASoC: qcom: q6apm: Wait for SPF to be ready before registering
>
> apm_probe() sends APM_CMD_GET_SPF_STATE once and ignores the result.
> At cold boot this is harmless: module loading and PDR lookup delay the
> probe by tens of milliseconds after the DSP is up, so SPF is listening
> by the time the command is sent.
>
> After an ADSP restart, everything is already loaded and PDR reports the
> audio PD up at once. The GPR service is added about 0.5 ms after the
> remoteproc reports the DSP up, and SPF drops a command sent that early
> without replying:
>
>   remoteproc remoteproc0: remote processor adsp is now up
>   qcom,apr ...: Adding APR/GPR dev: gprsvc:service:2:1
>   qcom-apm gprsvc:service:2:1: CMD timeout for [1001021] opcode
>
> Once the q6apm component is registered, a sound card whose other
> components are still bound can bind right away, inside apm_probe(), and
> issue APM_CMD_SHARED_MEM_MAP_REGIONS before SPF can handle it. The map
> times out and the card fails to come up:
>
>   qcom-apm gprsvc:service:2:1: CMD timeout for [100100c] opcode
>
> Poll GET_SPF_STATE before registering the component. Simply retrying
> q6apm_get_apm_state() is not enough: it goes through
> audioreach_send_cmd_sync(), whose 5 second timeout makes every dropped
> command stall the probe, and the card with it, for 5 seconds. Instead,
> send the command directly and wait only 100 ms for the reply, for up to
> 5 seconds in total, which is the worst case of the single command sent
> before. A timed out try is retried at once, while a "not ready" reply
> or a send failure backs off for 100 ms first. On Nord, SPF answers
> within 100-200 ms of the restart. If it still does not answer, warn and
> carry on as before rather than failing the probe.
>
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>

[Severity: Low]
Should this bug fix include a Fixes: tag for the commit that added the
unchecked q6apm_get_apm_state() call in apm_probe()?

  Fixes: 5477518b8a0e ("ASoC: qdsp6: audioreach: add q6apm support")

> diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> index 4cb597d737b16..e7ba6bc931421 100644
> --- a/sound/soc/qcom/qdsp6/q6apm.c
> +++ b/sound/soc/qcom/qdsp6/q6apm.c

[ ... ]

> @@ -154,6 +156,47 @@ static int q6apm_get_apm_state(struct q6apm *apm)
>  	return apm->state;
>  }
>  
> +/*
> + * Right after the DSP boots, the SPF drops commands without replying. Poll
> + * GET_SPF_STATE with a short timeout per try instead of the 5 s one of
> + * q6apm_send_cmd_sync(), so a dropped command does not stall the probe.
> + */
> +static bool q6apm_wait_spf_ready(struct q6apm *apm)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(Q6APM_SPF_READY_MS);
> +	gpr_device_t *gdev = apm->gdev;
> +
> +	do {
> +		struct gpr_pkt *pkt __free(kfree) =
> +			audioreach_alloc_apm_cmd_pkt(0, APM_CMD_GET_SPF_STATE, 0);
> +		long rc;
> +
> +		if (IS_ERR(pkt))
> +			return false;
> +
> +		pkt->hdr.dest_domain = audioreach_gpr_dest_domain(gdev);
> +
> +		mutex_lock(&apm->lock);
> +		apm->result.opcode = 0;
> +		apm->state = 0;
> +		rc = gpr_send_pkt(gdev, pkt);
> +		if (rc >= 0)
> +			rc = wait_event_timeout(apm->wait,
> +					apm->result.opcode == APM_CMD_RSP_GET_SPF_STATE,
> +					msecs_to_jiffies(Q6APM_SPF_POLL_MS));
> +		mutex_unlock(&apm->lock);
> +
> +		if (apm->state)
> +			return true;

[Severity: Medium]
Can q6apm_wait_spf_ready() race with apm_callback() on apm->result.opcode
and apm->state?

sound/soc/qcom/qdsp6/q6apm.c:apm_callback() {
    ...
	case APM_CMD_RSP_GET_SPF_STATE:
		apm->result.opcode = hdr->opcode;
		apm->result.status = 0;
		/* First word of result it state */
		apm->state = result->opcode;
		wake_up(&apm->wait);
		break;
    ...
}

CPU0                              CPU1 (apr_rxwq)
q6apm_wait_spf_ready()
  wait_event_timeout() -> 0
  /* next iteration */
  mutex_lock(&apm->lock)
  apm->result.opcode = 0
                                  apm_callback()
                                    apm->result.opcode =
                                      APM_CMD_RSP_GET_SPF_STATE
                                    apm->state = 1
  apm->state = 0  <-- clobbers state
  gpr_send_pkt()
  wait_event_timeout()
    /* opcode already set */
  if (apm->state) /* 0 */

When a poll attempt times out just as a delayed reply arrives, the next
iteration can overwrite apm->state = 1 with 0 while leaving
apm->result.opcode set, or return true on the stale reply while the newly
sent APM_CMD_GET_SPF_STATE is still in flight to overwrite apm->result
during a later command.

Also, apm_callback() writes apm->result.opcode before apm->state without
a lock or barrier, so wait_event_timeout() can observe the opcode update
before apm->state is visible. Because apm_probe() sets g_apm = apm before
calling q6apm_wait_spf_ready(), resetting apm->state = 0 and reading it
outside apm->lock also races with concurrent q6apm_is_adsp_ready() calls.

[ ... ]

> @@ -906,7 +949,12 @@ static int apm_probe(gpr_device_t *gdev)
>  	dev_set_drvdata(dev, apm);
>  	g_apm = apm;
>  
> -	q6apm_get_apm_state(apm);
> +	/*
> +	 * Once the component is registered a card can bind and send commands
> +	 * at once, so wait for the SPF first.
> +	 */
> +	if (!q6apm_wait_spf_ready(apm))
> +		dev_warn(dev, "SPF not ready\n");
>  
>  	ret = snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
>  	if (ret < 0) {

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failure in
snd_soc_register_component() or of_platform_populate() leave g_apm
pointing to freed memory [1]?

apm is allocated with devm_kzalloc_obj(), and g_apm = apm is set before
snd_soc_register_component() and of_platform_populate(). If either call
fails, devres frees apm while g_apm remains non-NULL, causing a
use-after-free when a later caller such as q6apm_is_adsp_ready()
dereferences g_apm.

[1] https://sashiko.dev/#/bug/linux-8f66e185-2dfb-43c1-bfaa-a2be8b96868d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115032027.414953-1-shengchao.guo@oss.qualcomm.com?part=1