Re: [PATCH v3 2/2] ASoC: codecs: add Qualcomm WSA885X codec driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The ASoC component is registered before the IRQ handler is requested, creating a race condition where unhandled interrupts can disable the IRQ line or cause use-after-free during teardown.
- [High] The SDCA_COMMIT register is not marked as volatile, causing volume and state changes to silently fail after the first commit.
- [High] Incorrect I2S TDM RX slot mask for STEREO mode deprives the right PA of audio data.
- [Medium] Supported fractional and high sample rates are missing from WSA885X_RATES.
--

--- Patch [2]: [PATCH v3 2/2] ASoC: codecs: add Qualcomm WSA885X codec driver ---
commit c6cb8f530e0636df275b374514b9c75c67764cac
Author: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>
Subject: ASoC: codecs: add Qualcomm WSA885X codec driver

This commit introduces an ASoC codec driver for the Qualcomm WSA885X stereo
smart speaker amplifier. It adds support for programming the register map, DAI
operations, and mixer controls. The driver handles power-state sequencing in
DAI callbacks and uses regmap for control path operations.

> diff --git a/sound/soc/codecs/wsa885x.c b/sound/soc/codecs/wsa885x.c
> new file mode 100644
> index 0000000000000..d2f4b980ac27d
> --- /dev/null
> +++ b/sound/soc/codecs/wsa885x.c
> @@ -195,5 +195,5 @@
>  
>  #define WSA885X_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 | \
>  			       SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_48000 | \
>  			       SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000)
>  

[Severity: Medium]
Are we missing supported fractional and high sample rates here?

If SNDRV_PCM_RATE_44100 and others are missing from WSA885X_RATES, the ALSA
core framework will reject any attempt to open a stream at 44.1kHz.

This would make the code blocks handling 44100Hz and other missing rates in
wsa885x_codec_hw_params() dead code and prevent playback of standard CD quality
audio.

[ ... ]

> @@ -881,13 +881,13 @@ static int wsa885x_codec_mute_stream(struct snd_soc_dai *dai, int mute, int stre
>  		ret = wsa885x_wait_for_pde_state(wsa885x, ps0);
>  		if (ret)
>  			goto exit_unlock;
>  
>  		if (wsa885x->rx_slot_mask == WSA885X_CHANNEL_STEREO) {
>  			regmap_write(wsa885x->regmap, WSA885X_DIG_CTRL1_I2S_TDM_CH_RX, 0x08);
>  			regmap_write(wsa885x->regmap, WSA885X_DIG_CTRL0_PA_FSM_CTL, 0x03);
>  		} else if (wsa885x->rx_slot_mask == WSA885X_CHANNEL_MONO_LEFT) {
>  			regmap_write(wsa885x->regmap, WSA885X_DIG_CTRL1_I2S_TDM_CH_RX, 0x08);
>  			regmap_write(wsa885x->regmap, WSA885X_DIG_CTRL0_PA_FSM_CTL, 0x01);

[Severity: High]
In stereo mode, should the TDM RX slot mask be 0x09 instead of 0x08?

Currently, the driver writes 0x08 to WSA885X_DIG_CTRL1_I2S_TDM_CH_RX for both
WSA885X_CHANNEL_STEREO and WSA885X_CHANNEL_MONO_LEFT. This leaves the right
amplifier channel without valid audio data, breaking stereo playback.

[ ... ]

> @@ -965,15 +965,15 @@ static bool wsa885x_volatile_register(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
>  	case WSA885X_ANA_TOP_PLL_STATUS_0:
>  	case WSA885X_ANA_TOP_PLL_STATUS_1:
>  	case WSA885X_SMP_AMP_CTRL_STEREO_PDE23_ACT_PS:
>  	case WSA885X_SMP_AMP_CTRL_STEREO_CS21_CLOCK_VALID:
>  	case WSA885X_INTR_STATUS0:
>  	case WSA885X_INTR_STATUS0 + 1:
>  	case WSA885X_INTR_STATUS0 + 2:
>  	case WSA885X_INTR_CLEAR0:
>  	case WSA885X_INTR_CLEAR0 + 1:
>  	case WSA885X_INTR_CLEAR0 + 2:
>  		return true;
>  	default:
>  		return false;
>  	}
>  }

[Severity: High]
Should WSA885X_DIG_CTRL0_SDCA_COMMIT be included in this volatile register
list?

When a user changes the volume or starts a stream, the driver performs:

    regmap_write(wsa885x->regmap, WSA885X_DIG_CTRL0_SDCA_COMMIT, 0x01);

Because this register is missing from wsa885x_volatile_register(), regmap
caches the value 0x01. Subsequent writes of 0x01 are dropped by the regmap
cache, causing volume and state changes to silently fail.

[ ... ]

> @@ -1486,9 +1486,9 @@ static int wsa885x_probe(struct i2c_client *client)
>  
>  	ret = devm_snd_soc_register_component(dev, component_driver,
>  					      wsa885x_dai,
>  					      ARRAY_SIZE(wsa885x_dai));
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Codec component registration failed\n");
>  
>  	ret = wsa885x_register_irq(wsa885x);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "wsa885x irq registration failed\n");

[Severity: High]
Could this ordering cause a race condition between unmasking hardware
interrupts and registering the IRQ handler?

The component registration triggers wsa885x_component_probe(), which calls
wsa885x_unmask_interrupts(). Because this happens before
wsa885x_register_irq() runs, any pending or new hardware interrupt will
assert before the handler is actually registered.

This can cause the kernel to permanently disable the IRQ line or cause a
use-after-free during teardown, since devres will free the IRQ handler before
the component is unregistered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709175915.3805851-1-prasad.kumpatla@oss.qualcomm.com?part=2