[PATCH 0/2] Fix GPR callback registration race during probe

[PATCH 0/2] Fix GPR callback registration race during probe

[Mohit Sharma]

GPR service drivers can issue synchronous commands directly from their
probe callbacks or indirectly while populating child devices. The GPR
bus currently publishes the service callback only after the client probe
returns. Consequently, a DSP response received during probe cannot be
dispatched and the command times out.

This was observed intermittently on Qualcomm Lemans and Monaco platforms
while q6apm child probing issued an APM shared-memory map command. The DSP
response reached the GPR receive path, but was discarded because the
service callback had not yet been installed.

The first patch prepares q6apm and q6prm for early callback publication by
ensuring callback-visible state is initialized before driver data is
published and safely ignoring packets received before that point. The
second patch publishes GPR callbacks before invoking client probes and
clears them again when probing fails. Existing APR callback ordering is
left unchanged.

Mohit Sharma (2):
  ASoC: qcom: q6apm: Guard callbacks until probe data is ready
  soc: qcom: apr: Publish GPR callback before client probe

 drivers/soc/qcom/apr.c          | 15 +++++++++++++--
 sound/soc/qcom/qdsp6/q6apm.c   |  6 ++++--
 sound/soc/qcom/qdsp6/q6prm.c   |  3 +++
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/2] ASoC: qcom: q6apm: Guard callbacks until probe data is ready

[Mohit Sharma]

The GPR bus needs to make response callbacks available before invoking a
client probe because clients can issue synchronous commands from probe.
This creates a short interval where a callback can run before the client
has published its private data.

Make q6apm publish its driver data only after all callback-visible state
has been initialized. Have the q6apm and q6prm callbacks ignore packets
received before driver data is available.

Fixes: ec1471a898cc ("soc: qcom: apr: Add GPR support")
Cc: stable@vger.kernel.org
Co-developed-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Signed-off-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Signed-off-by: Mohit Sharma <mohit.sharma@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6apm.c | 6 ++++--
 sound/soc/qcom/qdsp6/q6prm.c | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
index 3ec3c5a18c7a..dbebb643cd5e 100644
--- a/sound/soc/qcom/qdsp6/q6apm.c
+++ b/sound/soc/qcom/qdsp6/q6apm.c
@@ -872,8 +872,6 @@ static int apm_probe(gpr_device_t *gdev)
 	if (!apm)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, apm);
-
 	mutex_init(&apm->lock);
 	apm->dev = dev;
 	apm->gdev = gdev;
@@ -887,6 +885,7 @@ static int apm_probe(gpr_device_t *gdev)
 
 	idr_init(&apm->modules_idr);
 
+	dev_set_drvdata(dev, apm);
 	g_apm = apm;
 
 	q6apm_get_apm_state(apm);
@@ -930,6 +929,9 @@ static int apm_callback(const struct gpr_resp_pkt *data, void *priv, int op)
 	const struct gpr_hdr *hdr = &data->hdr;
 	int graph_id, is_pos_buf;
 
+	if (!apm)
+		return 0;
+
 	result = data->payload;
 
 	switch (hdr->opcode) {
diff --git a/sound/soc/qcom/qdsp6/q6prm.c b/sound/soc/qcom/qdsp6/q6prm.c
index 2042ff1e8ff1..5c927ebd81f4 100644
--- a/sound/soc/qcom/qdsp6/q6prm.c
+++ b/sound/soc/qcom/qdsp6/q6prm.c
@@ -194,6 +194,9 @@ static int prm_callback(const struct gpr_resp_pkt *data, void *priv, int op)
 	const struct gpr_ibasic_rsp_result_t *result;
 	const struct gpr_hdr *hdr = &data->hdr;
 
+	if (!prm)
+		return 0;
+
 	switch (hdr->opcode) {
 	case PRM_CMD_RSP_REQUEST_HW_RSC:
 	case PRM_CMD_RSP_RELEASE_HW_RSC:
-- 
2.34.1

[PATCH 2/2] soc: qcom: apr: Publish GPR callback before client probe

[Mohit Sharma]

A GPR service driver can issue synchronous commands from its probe path,
or populate child devices which issue commands before the parent probe
returns. The APR/GPR bus currently installs the GPR service callback only
after the service driver's probe has completed.

If a response arrives while the client probe is still running, GPR finds
the destination service but cannot dispatch the response because the
service callback is NULL. The client waiting for the response eventually
times out.

This was observed intermittently on Qualcomm Lemans and Monaco platforms
when q6apm child probing sent an APM shared-memory map command before the
parent probe returned. The failure has the following signature:

  qcom-apm ... CMD timeout for [100100c] opcode
  q6apm-dai ... Audio Start: Buffer Allocation failed rc = -110
  snd-sc8280xp ... ASoC: can't create pcm MultiMedia1 Playback :-110

Install the GPR callback before invoking the client probe so responses to
probe-time commands can be dispatched. Clear the callback again if probe
fails, and preserve the existing callback ordering for APR clients.

Fixes: ec1471a898cc ("soc: qcom: apr: Add GPR support")
Cc: stable@vger.kernel.org
Co-developed-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Signed-off-by: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Signed-off-by: Mohit Sharma <mohit.sharma@oss.qualcomm.com>
---
 drivers/soc/qcom/apr.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
index ea7f83916d8d..829e5c8fdf98 100644
--- a/drivers/soc/qcom/apr.c
+++ b/drivers/soc/qcom/apr.c
@@ -365,13 +365,24 @@ static int apr_device_probe(struct device *dev)
 {
 	struct apr_device *adev = to_apr_device(dev);
 	struct apr_driver *adrv = to_apr_driver(dev->driver);
+	struct packet_router *apr = dev_get_drvdata(dev->parent);
 	int ret;
 
+	if (apr->type == PR_TYPE_GPR)
+		adev->svc.callback = adrv->gpr_callback;
+
 	ret = adrv->probe(adev);
-	if (!ret)
+	if (ret) {
+		if (apr->type == PR_TYPE_GPR)
+			adev->svc.callback = NULL;
+
+		return ret;
+	}
+
+	if (apr->type != PR_TYPE_GPR)
 		adev->svc.callback = adrv->gpr_callback;
 
-	return ret;
+	return 0;
 }
 
 static void apr_device_remove(struct device *dev)
-- 
2.34.1

Re: [PATCH 2/2] soc: qcom: apr: Publish GPR callback before client probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soc: qcom: apr: race with gpr_do_rx_callback() on apr_device_probe() failure
--

commit b8085aea7f4dfac0f85898613864891fa165e4b0
Author: Mohit Sharma <mohit.sharma@oss.qualcomm.com>

soc: qcom: apr: Publish GPR callback before client probe

This commit installs the service callback in apr_device_probe() before calling
the client driver's probe function when the packet router type is GPR, so that
responses to commands sent during probe can be dispatched. If the client probe
fails, it clears the callback before returning the error.

> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> index ea7f83916d8d8..829e5c8fdf984 100644
> --- a/drivers/soc/qcom/apr.c
> +++ b/drivers/soc/qcom/apr.c
> @@ -365,13 +365,24 @@ static int apr_device_probe(struct device *dev)
>  {
>  	struct apr_device *adev = to_apr_device(dev);
>  	struct apr_driver *adrv = to_apr_driver(dev->driver);
> +	struct packet_router *apr = dev_get_drvdata(dev->parent);
>  	int ret;
>  
> +	if (apr->type == PR_TYPE_GPR)
> +		adev->svc.callback = adrv->gpr_callback;
> +
>  	ret = adrv->probe(adev);
> -	if (!ret)
> +	if (ret) {
> +		if (apr->type == PR_TYPE_GPR)
> +			adev->svc.callback = NULL;
> +
> +		return ret;
> +	}

[Severity: High]
Can clearing adev->svc.callback on probe failure without synchronizing with
apr->rxwq race with an in-flight gpr_do_rx_callback() execution and cause a
use-after-free on devm-allocated driver data?

In drivers/soc/qcom/apr.c, apr_add_device() inserts &adev->svc into
apr->svcs_idr before calling device_register(&adev->dev). Once
apr_device_probe() sets adev->svc.callback = adrv->gpr_callback, incoming
packets for that port on apr->rxwq can invoke svc->callback via
gpr_do_rx_callback():

drivers/soc/qcom/apr.c:gpr_do_rx_callback() {
	spin_lock_irqsave(&gpr->svcs_lock, flags);
	svc = idr_find(&gpr->svcs_idr, hdr->dest_port);
	spin_unlock_irqrestore(&gpr->svcs_lock, flags);

	if (!svc) {
		dev_err(gpr->dev, "GPR: Port(%x) is not registered\n",
			hdr->dest_port);
		return -EINVAL;
	}

	if (svc->callback)
		svc->callback(&resp, svc->priv, 0);
}

During adrv->probe(adev), client drivers such as apm_probe() in
sound/soc/qcom/qdsp6/q6apm.c and prm_probe() in sound/soc/qcom/qdsp6/q6prm.c
allocate their private state with devm_kzalloc(), publish it via
dev_set_drvdata(), and can then fail or defer probe after sending commands or
while unsolicited packets arrive:

sound/soc/qcom/qdsp6/q6apm.c:apm_probe() {
	...
	dev_set_drvdata(dev, apm);
	g_apm = apm;

	q6apm_get_apm_state(apm);

	ret = snd_soc_register_component(dev, &q6apm_audio_component, NULL, 0);
	if (ret < 0) {
		dev_err(dev, "failed to register q6apm: %d\n", ret);
		return ret;
	}

	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
	if (ret)
		snd_soc_unregister_component(dev);

	return ret;
}

sound/soc/qcom/qdsp6/q6prm.c:prm_probe() {
	...
	dev_set_drvdata(dev, cc);

	if (!q6apm_is_adsp_ready())
		return -EPROBE_DEFER;

	return devm_of_platform_populate(dev);
}

When adrv->probe(adev) returns an error, apr_device_probe() clears
adev->svc.callback = NULL with a plain store and returns immediately to
really_probe() -> device_unbind_cleanup(), which frees devres allocations
before clearing drvdata:

drivers/base/dd.c:device_unbind_cleanup() {
	devres_release_all(dev);
	arch_teardown_dma_ops(dev);
	kfree(dev->dma_range_map);
	dev->dma_range_map = NULL;
	device_set_driver(dev, NULL);
	dev_set_drvdata(dev, NULL);
}

If gpr_do_rx_callback() on apr->rxwq has already passed the svc->callback
check, could apm_callback() or prm_callback() concurrently dereference and
write to the freed struct q6apm or struct q6prm obtained from
dev_get_drvdata(&gdev->dev) (or hit a NULL function pointer dereference if
svc->callback is reloaded after the unlocked check in gpr_do_rx_callback())?

Should adev->svc.callback be updated with READ_ONCE()/WRITE_ONCE() (or under
lock) and apr->rxwq be flushed after clearing adev->svc.callback on the probe
error path before returning?

> +
> +	if (apr->type != PR_TYPE_GPR)
>  		adev->svc.callback = adrv->gpr_callback;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static void apr_device_remove(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022144130.2443667-2-mohit.sharma@oss.qualcomm.com?part=2