Re: [PATCH v2 07/11] ASoC: qcom: Add QAIF regmap, DT parsing and platform init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Modifying a statically allocated, shared struct (`audio_qaif_regmap_config`) during probe causes race conditions and state corruption.
- [High] Clocks are manually deinitialized in remove() before devres unregisters the ASoC component, causing a devres ordering bug.
- [Medium] Unchecked underflow when passing `v->num_* - 1` to `GENMASK()`, leading to undefined behavior and hardware register corruption.
- [Medium] Bitwise ORing negative error codes returned by `regmap_write()` destroys original error values.
- [Medium] Unsigned variable `intf_idx` is checked for `< 0`, bypassing the intended error handler.
--

commit 70dc20df9813861f68ee157ba8ffd821195c8a29
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF regmap, DT parsing and platform init

This commit adds the core CPU and platform bring-up needed for the Qualcomm
Audio Interface driver. It sets up the MMIO regmap, parses child nodes from
the devicetree for PCM/TDM/MI2S configurations, and initializes variant
resources and the ASoC CPU component.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> index 93b75ea8e48f5..99c3465466b5a 100644
> --- a/sound/soc/qcom/qaif-cpu.c
> +++ b/sound/soc/qcom/qaif-cpu.c

[ ... ]

> +static int of_qaif_parse_aif_intf_cfg(struct device *dev,
> +				      struct qaif_drv_data *data)
> +{

[ ... ]

> +		if (v->get_dma_idx) {
> +			intf_idx = v->get_dma_idx(dai_id);
> +			if (intf_idx < 0) {

[Severity: Medium]
Can intf_idx ever be less than 0 here?

Since intf_idx is declared as a u32, this check will always evaluate to false.
If v->get_dma_idx() returns a negative error code, it will underflow into a
large positive number and bypass this intended error handling block.

> +				dev_err(dev, "Invalid DAI ID %d for interface '%s' (node %d)\n",
> +					dai_id, intf_np->name, num_interfaces);
> +				continue;
> +			}

[ ... ]

> +int asoc_qcom_qaif_cpu_platform_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	audio_qaif_regmap_config.max_register = resource_size(res);
> +	drvdata->audio_qaif_map =
> +		devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> +				      &audio_qaif_regmap_config);

[Severity: High]
Does modifying audio_qaif_regmap_config.max_register directly cause issues if
multiple QAIF devices are probed?

Since audio_qaif_regmap_config is a statically allocated global structure,
multiple instances of the driver might race to overwrite this field, leaving
all instances with the register bounds of the last probed device.

> +	if (IS_ERR(drvdata->audio_qaif_map))
> +		return PTR_ERR(drvdata->audio_qaif_map);

[ ... ]

> +void asoc_qcom_qaif_cpu_platform_remove(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (drvdata->variant->clk_deinit)
> +		drvdata->variant->clk_deinit(pdev);
> +}

[Severity: High]
Does manually deinitializing the clocks here create a devres ordering issue?

The ASoC component is registered using devm_snd_soc_register_component(),
which means it will be unregistered during the devres teardown phase after
this remove function returns.

If the hardware clocks are already disabled here, will any teardown operations
or DAPM sequences that interact with the hardware registers cause a
synchronous external abort?

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

[ ... ]

> +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 = 0;
> +	u32 mask;
> +
> +	mask = GENMASK(v->num_rddma - 1, 0);

[Severity: Medium]
Could v->num_rddma be zero for any hardware variants?

If a variant has 0 channels, v->num_rddma - 1 would underflow to U32_MAX,
causing GENMASK() to generate a fully populated mask. Writing this to the
hardware could map non-existent channels or corrupt reserved bits. There
doesn't appear to be a check for 0 here, unlike the v->num_intf check below.

> +	ret |= regmap_write(map, QAIF_EE_RDDMA_MAP_REG(v), mask);

[Severity: Medium]
Does bitwise ORing the return values from regmap_write() corrupt the error
codes?

Since regmap_write() returns negative errno values (like -EINVAL), bitwise
ORing two's complement negative numbers will mangle the return codes and
result in nonsensical error values being passed up the stack.

> +
> +	mask = GENMASK(v->num_wrdma - 1, 0);
> +	ret |= regmap_write(map, QAIF_EE_WRDMA_MAP_REG(v), mask);

[ ... ]

> +static int qaif_map_dma_path(struct qaif_drv_data *drvdata)
> +{
> +	struct regmap *map = drvdata->audio_qaif_map;
> +	const struct qaif_variant *v = drvdata->variant;
> +	int ret = 0;
> +	int qxm_sel = v->qxm_type;
> +
> +	if (qxm_sel != QXM0) {
> +		dev_err(regmap_get_device(map),
> +			"%s: only QXM0 is supported, qxm_type=%d\n",
> +			__func__, qxm_sel);
> +		return -EINVAL;
> +	}
> +
> +	ret |= regmap_write(map, QAIF_RDDMA_MAP_QXM, qxm_sel);
> +	ret |= regmap_write(map, QAIF_WRDMA_MAP_QXM, qxm_sel);

[Severity: Medium]
Similar to the issue above, does accumulating negative error codes using
bitwise OR here destroy the original error values returned by regmap_write()?

> +	ret |= regmap_write(map, QAIF_CODEC_RDDMA_MAP_QXM, qxm_sel);
> +	ret |= regmap_write(map, QAIF_CODEC_WRDMA_MAP_QXM, qxm_sel);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701105757.2779738-1-harendra.gautam@oss.qualcomm.com?part=7