Re: [PATCH 13/13] ASoC: qcom: Add Shikra QAIF support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Core clocks are disabled in `remove()` before devres unregisters the ALSA component and IRQ handler, creating a race condition for bus crashes.
- [Low] DAI mapping arrays are missing the `static` keyword, polluting the global namespace.
- [Low] Latent build breakage risk due to `module_platform_driver()` within a variant file linked into a generic module.
- [Low] Using `of_match_ptr()` with an unconditionally defined match table triggers compiler warnings when `CONFIG_OF` is disabled.

Pre-existing issues:
- [High] Unprotected increment and decrement of the global reference counter `qaif_init_ref_cnt`.
--

commit 977cd466322e3f1bab9395ba1c325fb256edb595
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add Shikra QAIF support

This commit adds the Shikra variant data for the Qualcomm Audio Interface
driver. It provides the register layout, DMA-to-DAI mappings, clocks, and
DAI descriptors required to enable playback or capture on Shikra platforms.
The new file is built as part of the snd-soc-qcom-qaif module.

[ ... ]

> diff --git a/sound/soc/qcom/qaif-shikra.c b/sound/soc/qcom/qaif-shikra.c
> new file mode 100644
> index 0000000000000..e835645030879
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-shikra.c
> @@ -0,0 +1,585 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * qaif-shikra.c -- ALSA SoC CPU-Platform DAI driver for QTi QAIF
> + */
> +
> +#include <linux/module.h>
> +#include <sound/pcm.h>
> +#include <sound/soc.h>
> +#include <linux/pm.h>
> +#include "qaif.h"
> +
> +struct qaif_dmaidx_dai_map shikra_aif_dma_dai_map[] = {
> +		{ QAIF_MI2S_TDM_AIF0 },
> +		{ QAIF_MI2S_TDM_AIF1 },
> +		{ QAIF_MI2S_TDM_AIF2 },
> +		{ QAIF_MI2S_TDM_AIF3 }
> +};
> +
> +struct qaif_dmaidx_dai_map shikra_cif_rx_dma_dai_map[] = {
> +		{ QAIF_CDC_DMA_RX0 },
> +		{ QAIF_CDC_DMA_RX1 },
> +		{ QAIF_CDC_DMA_RX2 },
> +		{ QAIF_CDC_DMA_RX3 }
> +};
> +
> +struct qaif_dmaidx_dai_map shikra_cif_tx_dma_dai_map[] = {
> +		{ QAIF_CDC_DMA_TX0 },
> +		{ QAIF_CDC_DMA_TX1 },
> +		{ QAIF_CDC_DMA_TX2 },
> +		{ QAIF_CDC_DMA_TX3 }
> +};
> +
> +struct qaif_dmaidx_dai_map shikra_cif_va_dma_dai_map[] = {
> +		{ QAIF_CDC_DMA_VA_TX0 },
> +		{ QAIF_CDC_DMA_VA_TX1 },
> +		{ QAIF_CDC_DMA_VA_TX2 },
> +		{ QAIF_CDC_DMA_VA_TX3 }
> +};

[Severity: Low]
Are these mapping arrays intended to be globally visible?

Since they appear to only be used locally in shikra_qaif_get_dma_idx(),
could they be marked static to prevent namespace pollution and avoid
sparse warnings?

[ ... ]

> +static int shikra_qaif_exit(struct platform_device *pdev)
> +{
> +	struct qaif_drv_data *drvdata = platform_get_drvdata(pdev);
> +
> +	if (!drvdata || !drvdata->clks)
> +		return -EINVAL;
> +
> +	clk_bulk_disable_unprepare(drvdata->num_clks, drvdata->clks);
> +	return 0;
> +}

[Severity: High]
Does disabling core clocks here create a race condition during teardown?

If shikra_qaif_exit() disables clocks directly from the remove() callback,
devm-managed resources like the QAIF IRQ handler and ALSA component will
still be active since devres cleanup happens after remove() completes.

If an IRQ fires or userspace accesses ALSA during this window, could
accessing the unclocked regmap trigger a synchronous external abort?

[ ... ]

> +static struct platform_driver shikra_qaif_cpu_platform_driver = {
> +	.driver = {
> +		.name = "shikra-qaif-cpu",
> +		.of_match_table = of_match_ptr(shikra_qaif_cpu_device_id),

[Severity: Low]
Could using of_match_ptr() here cause a compiler warning when CONFIG_OF is
disabled?

The shikra_qaif_cpu_device_id array is compiled unconditionally, so if
of_match_ptr() evaluates to NULL on non-OF builds, the array would be
left unreferenced.

> +		.pm = &shikra_qaif_pm_ops,
> +	},
> +	.probe = asoc_qcom_qaif_cpu_platform_probe,
> +	.remove = asoc_qcom_qaif_cpu_platform_remove,
> +	.shutdown = asoc_qcom_qaif_cpu_platform_shutdown,
> +};
> +module_platform_driver(shikra_qaif_cpu_platform_driver);

[Severity: Low]
Does using module_platform_driver() in a variant file cause issues for the
generic module?

Since snd-soc-qcom-qaif.ko links multiple object files, tying the module's
init and cleanup symbols directly to the Shikra variant might cause a multiple
definition build failure if another variant file is added later that also
calls module_platform_driver().

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the
linked qaif-platform.c driver. When qaif_platform_pcmops_open() increments
the counter:

qaif_platform_pcmops_open()
    drvdata->qaif_init_ref_cnt++;

Since different DAIs map to different ALSA PCM streams, their open and
close callbacks can execute concurrently. Does this non-atomic increment
allow two concurrent opens to both read the counter as 0, race on the
hardware initialization, and corrupt the refcount?

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