Re: [PATCH v9 07/10] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: qcom: qaif: Undefined behavior in GENMASK due to unchecked zero channel counts
- [High] ASoC: qcom: qaif: HW register state loss on suspend/resume
- [Medium] ASoC: qcom: qaif: Typo in DMA configuration writes DMACTL constant to DMACFG register
--

commit 19722e0c0803b48feb5e66f22dfc7dca7c2d611e
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF PCM operations

This commit adds the PCM platform callbacks needed to use QAIF DAIs for
playback and capture. It handles pre-allocating the DMA buffer, reserving
DMA stream indices, one-time hardware initialization, and programming DMA
clocks and registers.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-platform.c

[ ... ]

> +static const struct snd_pcm_hardware qaif_platform_hardware = {
> +	.info			=	SNDRV_PCM_INFO_MMAP |
> +					SNDRV_PCM_INFO_MMAP_VALID |
> +					SNDRV_PCM_INFO_INTERLEAVED |
> +					SNDRV_PCM_INFO_PAUSE |
> +					SNDRV_PCM_INFO_RESUME,

[Severity: High]
When the system suspends and the LPASS power domain is turned off, will the
QAIF hardware registers (SHRAM mappings, EE resources, AIF/CIF interface
configs, and DMA base/lengths) lose their state?

If the hardware loses power context, advertising SNDRV_PCM_INFO_RESUME
instructs ALSA not to call the prepare function on resume, leaving DMA base
and period lengths unprogrammed.

Additionally, because the qaif-cpu regmap uses REGCACHE_NONE, it will not
automatically restore state during pm_runtime_force_resume. 

[ ... ]

> +static int qaif_map_ee_resource(struct qaif_drv_data *drvdata)
> +{
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct regmap *map = drvdata->audio_qaif_map;
> +	int ret;
> +
> +	ret = regmap_write(map, QAIF_EE_RDDMA_MAP_REG(v), GENMASK(v->num_rddma - 1, 0));

[Severity: High]
Is it possible for v->num_rddma to be 0 if a SoC variant lacks a specific DMA
path?

If v->num_rddma is 0, v->num_rddma - 1 underflows to -1. Passing a negative
upper bound to GENMASK results in an out-of-bounds shift operation, which is
undefined behavior and can generate a corrupted bitmask that will corrupt the
hardware's EE map registers.

Could we add boundary checks for the DMA channel counts similar to the check
done for v->num_intf?

> +	if (ret)
> +		return ret;
> +	ret = regmap_write(map, QAIF_EE_WRDMA_MAP_REG(v), GENMASK(v->num_wrdma - 1, 0));
> +	if (ret)
> +		return ret;
> +	if (v->num_intf > 0) {
> +		ret = regmap_write(map, QAIF_EE_INTF_MAP_REG(v), GENMASK(v->num_intf - 1, 0));
> +		if (ret)
> +			return ret;
> +	}
> +	ret = regmap_write(map, QAIF_EE_CODEC_RDDMA_MAP_REG(v),
> +			   GENMASK(v->num_codec_rddma - 1, 0));
> +	if (ret)
> +		return ret;
> +	return regmap_write(map, QAIF_EE_CODEC_WRDMA_MAP_REG(v),
> +			    GENMASK(v->num_codec_wrdma - 1, 0));

[Severity: High]
Does this code have the same GENMASK underflow risk if v->num_codec_wrdma
is 0?

[ ... ]

> +static int qaif_init(struct snd_soc_component *component)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	if (drvdata->qaif_hw_configured)
> +		return 0;

[Severity: High]
If the system resumes and attempts to open a new stream, will this prevent
the SHRAM and EE resource maps from being reprogrammed after they were wiped
during suspend?

[ ... ]

> +static int qaif_platform_pcmops_hw_params(struct snd_soc_component *component,
> +					  struct snd_pcm_substream *substream,
> +					  struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	ret = regmap_update_bits(drvdata->audio_qaif_map,
> +				 qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACFG_SHRAM_WM_MASK,
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACTL_WM_5);

[Severity: Medium]
Is QAIF_DMACTL_WM_5 the correct constant to use here?

The code is updating the DMA Configuration (DMACFG) register, but
QAIF_DMACTL_WM_5 appears to be a constant intended for the DMA Control
(DMACTL) register namespace. Should this use a QAIF_DMACFG_ prefixed constant
instead to avoid misconfiguring the hardware watermark?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-qaif-shikra-audio-v9-0-f3022e28f2fd@oss.qualcomm.com?part=7