Re: [PATCH 10/13] ASoC: qcom: Add QAIF regmap, DT parsing and platform init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing lock protection around `qaif_init_ref_cnt` leads to un-serialized hardware initialization data race.
- [High] Using `GENMASK` with unchecked channel counts (e.g., `v->num_rddma - 1`) causes integer underflow and undefined behavior if the count is zero.
- [High] Component teardown order is inverted, shutting down hardware before devm-managed ALSA components are unregistered.
- [High] Modifying the global static `regmap_config` structure during probe causes race conditions and an out-of-bounds register access bug.
- [High] The error code from enabling the AIF CTL register is overwritten without being returned, allowing a stream to silently fail to start.
- [High] A missing `qcom,qaif-aif-bits-per-lane` property defaults to 0, causing underflows during regmap writes and setting the DAI bitclock to 0Hz.
- [Medium] Variable `intf_idx` is declared as an unsigned 32-bit integer, making the error check `if (intf_idx < 0)` tautologically false.
- [Medium] Accumulating negative error codes using bitwise OR (`ret |= regmap_write(...)`) corrupts the return value.
--

commit 84915833d4dcf01d21e4e1a894c18953827f441d
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

    ASoC: qcom: Add QAIF regmap, DT parsing and platform init
    
    Add the core CPU and platform bring-up needed for the Qualcomm Audio
    Interface driver.
    
    The existing QAIF DAI code needs device-level infrastructure before it can
    configure interfaces or run PCM streams. Add the MMIO regmap setup,
    including readable, writable and volatile register tables, so the driver
    can safely access QAIF control, DMA, interrupt and SHRAM registers.

> diff --git a/sound/soc/qcom/qaif-cpu.c b/sound/soc/qcom/qaif-cpu.c
> index c9c8d78c5f5f8..e1030f5b17524 100644
> --- a/sound/soc/qcom/qaif-cpu.c
> +++ b/sound/soc/qcom/qaif-cpu.c
> @@ -783,3 +783,804 @@ const struct snd_soc_dai_ops asoc_qcom_qaif_aif_cpu_dai_ops = {
>  	.trigger	= qaif_aif_cpu_daiops_trigger,
>  };

[Severity: High]
Does qaif_aif_cpu_daiops_trigger() overwrite the error code from enabling the
AIF CTL register?

In the SNDRV_PCM_TRIGGER_START path of qaif_aif_cpu_daiops_trigger(), the
driver calls regmap_fields_write() to turn on the interface. If this write
fails, it logs an error but proceeds to call clk_enable(), overwriting the
ret variable.

If the clock enables successfully, the trigger function returns 0 (success)
to the ALSA core. The framework and application will falsely assume the
stream is running, but the hardware interface remains disabled, resulting
in silent audio playback/capture failures.

[ ... ]
> +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]
Since intf_idx is declared as a u32, will this check for negative error codes
ever evaluate to true?

If v->get_dma_idx() returns a negative error code like -EINVAL, it wraps to a
large positive integer. The check intf_idx < 0 will always evaluate to false,
bypassing this error path. The error is only caught by the subsequent bounds
check, which will print a misleading error message instead of failing properly.

[ ... ]
> +		/* Parse frame configuration */
> +		cfg->full_cycle_en = of_property_read_bool(intf_np, "qcom,qaif-aif-full-cycle-en");
> +
> +		ret = of_property_read_u32(intf_np, "qcom,qaif-aif-bits-per-lane", &cfg->bits_per_lane);
> +		if (ret) {
> +			dev_warn(dev, "Missing bits-per-lane for interface %d\n", num_interfaces);
> +			cfg->bits_per_lane = 0;

[Severity: High]
Does assigning 0 as a default here cause an integer underflow later?

When this default value is used in qaif_aif_cpu_daiops_hw_params(), it causes
an integer underflow: (slot_width * 0) - 1 evaluates to 0xFFFFFFFF. This
illegal value is then written directly to the bits_per_lane hardware
register field. It also causes clk_set_rate() to request exactly 0Hz
for the IBIT clock.

[ ... ]
> +int asoc_qcom_qaif_cpu_platform_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	audio_qaif_regmap_config.max_register = resource_size(res);

[Severity: High]
Does modifying this global static struct cause a data race if multiple
QAIF instances are probed concurrently?

Also, resource_size(res) defines the maximum inclusive valid register
address. Setting max_register exactly to the resource size permits regmap
accesses one byte beyond the valid mapped region.

> +
> +	drvdata->audio_qaif_map = devm_regmap_init_mmio(dev, drvdata->audio_qaif,
> +				&audio_qaif_regmap_config);
[ ... ]
> +void asoc_qcom_qaif_cpu_platform_remove(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (drvdata->variant->exit)
> +		drvdata->variant->exit(pdev);

[Severity: High]
Does shutting down the hardware here invert the component teardown order?

The driver registers sound components and IRQs using devm-managed functions,
meaning they are unregistered after the remove() callback returns. Manually
calling the exit() variant here disables the hardware clocks while the ALSA
component is still fully registered and accessible to userspace.

If an active stream or concurrent operation attempts unclocked regmap
accesses during this window, it could cause a synchronous external abort
or kernel panic.

> +}
[ ... ]

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> index 0000000000000..b298ee9d865ee
> --- /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: High]
If a hardware variant has 0 channels for a specific DMA type, does this cause
an integer underflow?

Calling GENMASK(0xFFFFFFFF, 0) invokes an out-of-bounds right shift in the
kernel's __GENMASK macro, which is undefined behavior. This can generate
invalid bitmasks and potentially grant access to nonexistent channels or
corrupt registers.

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

[Severity: Medium]
Is it safe to accumulate negative error codes using bitwise OR?

regmap_write() returns a negative error code (e.g., -EINVAL) on failure.
If multiple writes fail, their bitwise combination can result in an
arbitrary and misleading error code, masking the true cause of the failure
and confusing upper layers.

[ ... ]
> +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]
Could bitwise ORing the return values here corrupt standard error values?

If multiple regmap_write() calls return negative standard error codes,
combining them via bitwise OR will result in arbitrary and misleading error
values.

[ ... ]
> +static int qaif_init(struct snd_soc_component *component)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	if (drvdata->qaif_init_ref_cnt) {
> +		dev_dbg(component->dev, "%s: QAIF init is done already: ref cnt: %d\n",
> +				__func__, drvdata->qaif_init_ref_cnt);
> +		return 0;
> +	}

[Severity: High]
Is it possible for concurrent PCM opens to race on this lockless check?

qaif_init() checks qaif_init_ref_cnt without holding any locks. Its caller,
qaif_platform_pcmops_open(), calls qaif_init() and then increments the
counter. Since ALSA's pcm->open_mutex is per-PCM device, different DAIs
can be opened concurrently by userspace, causing a data race.

This could result in simultaneous writes to shared hardware registers and
corrupted hardware state.

[ ... ]

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