Re: [PATCH 5/13] ASoC: qcom: Add QAIF shared data structures and variant interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Client status register bitmasks omit critical summary IRQ bits, which can lead to an unhandled interrupt storm.
- [High] The `qaif_drv_data` structure lacks a lock to serialize concurrent process accesses to shared mutable state, leading to data races and channel double-allocation.
- [Medium] The commit message explicitly claims to define 'QAIF-private MI2S port IDs used for the senary and septenary interfaces', but these definitions are missing from the patch.
- [Low] Generic identifiers `enum dma_type` and `DMA_TYPE_MAX` pollute the global subsystem namespace.
--

commit 54235d0e0d62830d345de2e4cee5111d43b55417
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF shared data structures and variant interface

This commit introduces the qaif.h header, defining common data structures,
macros, and enums used by QAIF CPU DAI and PCM platform drivers. It includes
definitions for DMA control, IRQ mapping helpers, and the SOC variant
descriptor.

> Also define the QAIF-private MI2S port IDs used for the senary and
> septenary interfaces.

[Severity: Medium]
Are these MI2S port IDs missing from the patch? The commit message explicitly
claims to define QAIF-private MI2S port IDs for the senary and septenary
interfaces, but these definitions do not appear to be present in the provided
code.

> diff --git a/sound/soc/qcom/qaif.h b/sound/soc/qcom/qaif.h
> new file mode 100644
> index 0000000000000..d0f3b3026f5d9
> --- /dev/null
> +++ b/sound/soc/qcom/qaif.h

[ ... ]

> +enum qaif_dma_type {
> +	QAIF_AIF_DMA = 0,
> +	QAIF_CIF_DMA = 1,
> +	DMA_TYPE_MAX
> +};

[Severity: Low]
Does DMA_TYPE_MAX overlap confusingly with enum dma_type? It is defined inside
enum qaif_dma_type instead of enum dma_type.

[ ... ]

> +struct qaif_drv_data {
> +
> +	/* MI2S bit clock (derived from system clock by a divider) */
> +	struct clk *mi2s_bit_clk[LPASS_MAX_MI2S_PORTS];
> +
> +	/* SOC specific clock list */
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +
> +	struct clk *aud_dma_clk;
> +	struct clk *aud_dma_mem_clk;
> +
> +	/* Qualcomm audio interface (QAIF) registers */
> +	void __iomem *audio_qaif;
> +
> +	/* regmap backed by the Qualcomm audio interface (QAIF) registers */
> +	struct regmap *audio_qaif_map;
> +
> +	/* interrupts from the Qualcomm audio interface (QAIF) */
> +	int audio_qaif_irq;
> +
> +	/* QAIF init config refcount*/
> +	unsigned int qaif_init_ref_cnt;

[Severity: High]
Does this structure lack a lock to serialize concurrent access? 

Multiple PCMs mapped to different DAIs can be opened simultaneously by
userspace. Since ALSA only serializes operations per PCM via the PCM open
mutex, this allows concurrent execution of operations like PCM open for
different PCMs.

Could this lead to data races and channel double-allocation if multiple streams
concurrently read and increment qaif_init_ref_cnt or modify the channel
tracking bitmaps below?

> +
> +	/* SOC specific variations in the QAIF IP integration */
> +	const struct qaif_variant *variant;
> +
> +	/* Runtime-allocated regmap field handles (populated at probe) */
> +	struct qaif_dmactl *aif_rd_dmactl;
> +	struct qaif_dmactl *aif_wr_dmactl;
> +	struct qaif_dmactl *cif_rd_dmactl;
> +	struct qaif_dmactl *cif_wr_dmactl;
> +	struct qaif_aud_intfctl *aif_intfctl;
> +	struct qaif_cdc_intfctl *cif_rddma_intfctl;
> +	struct qaif_cdc_intfctl *cif_wrdma_intfctl;
> +	struct qaif_aif_config aif_intf_cfg[QAIF_MAX_AIF_CFG_CNT];
> +
> +	/* bit map to keep track of dma idx allocations */
> +	unsigned long aif_dma_idx_bit_map;
> +	unsigned long cif_dma_idx_bit_map;

[ ... ]

> +enum qaif_summary_irq_bitmask {
> +	QAIF_SUMMARY_BITMASK_AIF_PERIOD_RDDMA		= BIT(0),
> +	QAIF_SUMMARY_BITMASK_AIF_UNDERFLOW_RDDMA	= BIT(1),
> +	QAIF_SUMMARY_BITMASK_AIF_ERR_RSP_RDDMA		= BIT(2),
> +	QAIF_SUMMARY_BITMASK_AIF_PERIOD_WRDMA		= BIT(3),
> +	QAIF_SUMMARY_BITMASK_AIF_OVERFLOW_WRDMA		= BIT(4),
> +	QAIF_SUMMARY_BITMASK_AIF_ERR_RSP_WRDMA		= BIT(5),
> +
> +	QAIF_SUMMARY_BITMASK_AUD_OVERFLOW			= BIT(6),
> +	QAIF_SUMMARY_BITMASK_AUD_UNDERFLOW			= BIT(7),
> +
> +	QAIF_SUMMARY_BITMASK_RATE_DET				= BIT(8),
> +	QAIF_SUMMARY_BITMASK_VFR					= BIT(9),
> +	QAIF_SUMMARY_BITMASK_GRP					= BIT(10),
> +	QAIF_SUMMARY_BITMASK_RDDMA_OVERLAP			= BIT(11),
> +	QAIF_SUMMARY_BITMASK_WRDMA_OVERLAP			= BIT(12),
> +	QAIF_SUMMARY_BITMASK_INTF_OVERLAP			= BIT(13),
> +	QAIF_SUMMARY_BITMASK_GRP_OVERLAP			= BIT(14),
> +
> +	QAIF_SUMMARY_BITMASK_CIF_OVERLAP_RDDMA		= BIT(15),
> +	QAIF_SUMMARY_BITMASK_CIF_OVERLAP_WRDMA		= BIT(17),
> +	QAIF_SUMMARY_BITMASK_CIF_PERIOD_RDDMA		= BIT(18),
> +	QAIF_SUMMARY_BITMASK_CIF_UNDERFLOW_RDDMA	= BIT(19),
> +	QAIF_SUMMARY_BITMASK_CIF_ERR_RSP_RDDMA		= BIT(20),
> +	QAIF_SUMMARY_BITMASK_CIF_PERIOD_WRDMA		= BIT(24),
> +	QAIF_SUMMARY_BITMASK_CIF_OVERFLOW_WRDMA		= BIT(25),
> +	QAIF_SUMMARY_BITMASK_CIF_ERR_RSP_WRDMA		= BIT(26)
> +
> +};
> +
> +enum qaif_client_status_register_bitmask_info {
> +	QAIF_BITMASK_GROUP_INF			= 0x400,
> +	QAIF_BITMASK_AIF_RDDMA_WRDMA	= 0x3F,
> +	QAIF_BITMASK_CIF_RDDMA_WRDMA	= 0x71c0000,
> +	QAIF_BITMASK_DP_RDDMA			= 0xe00000,
> +	QAIF_BITMASK_AUD_INF			= 0xC0,
> +};

[Severity: High]
Are critical summary IRQ bits missing from these routing masks? 

The client status register bitmasks appear to omit several bits defined in
enum qaif_summary_irq_bitmask, such as the overlap bits (e.g., bit 15
QAIF_SUMMARY_BITMASK_CIF_OVERLAP_RDDMA), RATE_DET (bit 8), and VFR (bit 9).

If the hardware asserts an omitted bit, could the top-level IRQ handler return
IRQ_NONE and fail to clear it? This might lead to an unhandled interrupt storm
where the kernel IRQ subsystem eventually disables the IRQ line, breaking all
QAIF audio streams.

> +
> +struct qaif_irq_map {
> +	int client_id;
> +	u32 mask;
> +	irqreturn_t (*client_irq_handler)(struct qaif_drv_data *drvdata, u32 irq_status);
> +};
> +
> +enum dma_type {
> +	DMA_TYPE_RDDMA,
> +	DMA_TYPE_WRDMA
> +};

[Severity: Low]
Could this generic enum name cause namespace collisions? Defining enum
dma_type with values like DMA_TYPE_RDDMA in a shared SOC header lacks a
subsystem-specific prefix like qaif_. This risks colliding with other headers
in the global namespace.

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