Re: [PATCH RFC 17/23] ASoC: qcom: add SM8650 LPASS CPU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ASoC: qcom: wrong lpm_type for lpass-rxtx-lpaif in
  sm8650_lpaif_mi2s_desc[]
- [Medium] ASoC: qcom: missing set_bit() for RX channels in
  sm8650_lpass_alloc_dma_channel()
--

commit ee9ae911e7b58d190ad971326ccfe23116c1a06b
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: add SM8650 LPASS CPU driver

This commit adds the SM8650 LPASS CPU DAI driver variant supporting MI2S,
DisplayPort, and codec DMA audio interfaces when driven directly by the
application processor.

> diff --git a/sound/soc/qcom/lpass-sm8650.c b/sound/soc/qcom/lpass-sm8650.c
> new file mode 100644
> index 0000000000000..42050f56a4847
> --- /dev/null
> +++ b/sound/soc/qcom/lpass-sm8650.c
> @@ -0,0 +1,651 @@

[ ... ]

> +static int sm8650_lpass_alloc_dma_channel(struct lpass_data *drvdata,
> +					  int direction, unsigned int dai_id)
> +{

[ ... ]

> +	case LPASS_CDC_DMA_RX0 ... LPASS_CDC_DMA_RX9:
> +		chan = find_first_zero_bit(&drvdata->rxtx_dma_ch_bit_map,
> +					   v->rxtx_rdma_channels);
> +		if (chan >= v->rxtx_rdma_channels)
> +			return -EBUSY;
> +		break;
> +	case LPASS_CDC_DMA_TX0 ... LPASS_CDC_DMA_TX8:
> +		chan = find_next_zero_bit(&drvdata->rxtx_dma_ch_bit_map,
> +					  v->rxtx_wrdma_channel_start +
> +					  v->rxtx_wrdma_channels,
> +					  v->rxtx_wrdma_channel_start);
> +		if (chan >= v->rxtx_wrdma_channel_start + v->rxtx_wrdma_channels)
> +			return -EBUSY;
> +		set_bit(chan, &drvdata->rxtx_dma_ch_bit_map);
> +		break;
> +	case LPASS_CDC_DMA_VA_TX0 ... LPASS_CDC_DMA_VA_TX8:
> +		chan = find_next_zero_bit(&drvdata->va_dma_ch_bit_map,
> +					  v->va_wrdma_channel_start +
> +					  v->va_wrdma_channels,
> +					  v->va_wrdma_channel_start);
> +		if (chan >= v->va_wrdma_channel_start + v->va_wrdma_channels)
> +			return -EBUSY;
> +		set_bit(chan, &drvdata->va_dma_ch_bit_map);
> +		break;
> +	case LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9:
> +		chan = find_first_zero_bit(&drvdata->wsa_dma_ch_bit_map,
> +					   v->wsa_rdma_channels);
> +		if (chan >= v->wsa_rdma_channels)
> +			return -EBUSY;
> +		break;

[Severity: Medium]
Should the LPASS_CDC_DMA_RX0 ... LPASS_CDC_DMA_RX9 and
LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9 cases call set_bit() on
drvdata->rxtx_dma_ch_bit_map and drvdata->wsa_dma_ch_bit_map before
breaking?

Without set_bit(), channel 0 is never marked as allocated (even though
sm8650_lpass_free_dma_channel() clears the bit on close), so subsequent
allocations from those bitmaps will hand out channel 0 again and overwrite
drvdata->rxtx_substream[0] or drvdata->wsa_substream[0] in
lpass_platform_pcmops_open().

[ ... ]

> +/* MI2S mapping over LPAIF instances */
> +static const struct lpaif_mi2s_desc sm8650_lpaif_mi2s_desc[] = {
> +	{
> +		.reg_name = "lpass-lpaif",
> +		.irq_name = "lpass-irq-lpaif",
> +		.lpm_type = LPASS_LPM_NONE,
> +		.port_desc = (const struct mi2s_port_desc[]) {
> +			{ .intf = 0, .id = MI2S_PRIMARY, },
> +			{ .intf = 1, .id = MI2S_SECONDARY, },
> +		},
> +		.port_desc_count = 2,
> +	},
> +	{
> +		.reg_name = "lpass-rxtx-lpaif",
> +		.irq_name = "lpass-irq-rxtxif",
> +		.lpm_type = LPASS_VA_CDC_LPM,
> +		.port_desc = (const struct mi2s_port_desc[]) {
> +			{ .intf = 0, .id = MI2S_QUATERNARY, },
> +		},
> +		.port_desc_count = 1,
> +	},

[Severity: High]
Should the lpass-rxtx-lpaif descriptor use LPASS_RXTX_CDC_LPM instead of
LPASS_VA_CDC_LPM?

lpass-rxtx-lpaif is in the Audio CC / RXTX domain (clocked by
audio_cc_ext_if1_ibit), so setting LPASS_VA_CDC_LPM maps MI2S_QUATERNARY
DMA buffers from the 12 KiB VA LPM region (va_cdc_dma_lpm_buf) and enables
va_mem0 in lpass_cpu_daiops_startup() instead of using the 64 KiB RXTX LPM
region and audio_cc_codec_mem0..3.  If MI2S_QUATERNARY is enabled
alongside LPASS_CDC_DMA_VA_TX0, lpass_platform_allocate_lpm() also fails
probe with -EINVAL because VA Codec DMA already consumes all 12 KiB of VA
LPM, and if MI2S_QUATERNARY is enabled alongside MI2S_QUINARY without VA
Codec DMA, va_cdc_dma_lpm_remain underflows and assigns an offset past the
end of the 12 KiB VA LPM buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=17