Re: [PATCH v2 3/3] ASoC: qcom: qdsp6: generalize GPR service domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Global pointer `g_apm` is not cleared on driver removal, leading to a Use-After-Free.
- [High] Functions acquire a mutex and allocate memory with `GFP_KERNEL` while holding a spinlock with IRQs disabled.
--

commit bafa8aa7e49a38a568945d982b4109f80964d60c
Author: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>

ASoC: qcom: qdsp6: generalize GPR service domain

This commit generalizes the GPR destination domain in AudioReach to support 
targets where audio is served by the modem DSP instead of the ADSP. It 
updates the packet builders to dynamically stamp the correct domain from 
the device, rather than hardcoding it.

> diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> index f167b9dae3fa1..3ec3c5a18c7a2 100644
> --- a/sound/soc/qcom/qdsp6/q6apm.c
> +++ b/sound/soc/qcom/qdsp6/q6apm.c
> @@ -29,11 +29,13 @@ struct apm_graph_mgmt_cmd {
>  
>  static struct q6apm *g_apm;

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer
on driver removal?

The global pointer g_apm is assigned dynamically allocated memory during 
apm_probe(), which is bound to the device's lifecycle. However, apm_remove() 
fails to clear g_apm:

sound/soc/qcom/qdsp6/q6apm.c:apm_remove() {
	of_platform_depopulate(&gdev->dev);
	snd_soc_unregister_component(&gdev->dev);
}

The devres framework will free the apm structure automatically after removal. 
If another subsystem subsequently calls q6apm_is_adsp_ready(), it will 
dereference the freed pointer:

sound/soc/qcom/qdsp6/q6apm.c:q6apm_is_adsp_ready() {
	if (g_apm)
		return q6apm_get_apm_state(g_apm);
}

>  
> -int q6apm_send_cmd_sync(struct q6apm *apm, const struct gpr_pkt *pkt,
> +int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt,
>  			uint32_t rsp_opcode)
>  {

[ ... ]

> @@ -502,6 +504,8 @@ int q6apm_write_async(struct q6apm_graph *graph, uint32_t len, uint32_t msw_ts,
>  
>  	mutex_unlock(&graph->lock);
>  
> +	pkt->hdr.dest_domain = audioreach_gpr_dest_domain(graph->apm->gdev);
> +

[Severity: High]
This is a pre-existing issue, but does calling this function result in a 
sleep-in-atomic bug?

Earlier in q6apm_write_async(), it calls audioreach_alloc_pkt(), which 
allocates memory with GFP_KERNEL. It also acquires a sleepable mutex via 
mutex_lock(&graph->lock).

However, callers in sound/soc/qcom/qdsp6/q6apm-dai.c such as 
event_handler_compr() and q6apm_compr_copy() call this while holding an 
IRQ-disabled spinlock:

sound/soc/qcom/qdsp6/q6apm-dai.c:event_handler_compr() {
	...
	guard(spinlock_irqsave)(&prtd->lock);
	...
	q6apm_write_async(prtd->graph, bytes_to_write, 0, 0, wflags);
}

Could this lead to schedule-while-atomic panics or deadlocks during compressed 
audio playback?

>  	return gpr_send_port_pkt(graph->port, pkt);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-a2a-shikra-vmid-v5-v2-0-c3dc62354eee@oss.qualcomm.com?part=3