Re: [PATCH v2 2/4] ASoC: cdns: Add Cadence I2S-SC controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] sound/soc/cdns: resource leak in probe error paths
- [High] sound/soc/cdns: missing remove callback causes clock leak and unbalanced IRQ
- [High] sound/soc/cdns: race condition in ISR dropping interrupts via regmap_update_bits
- [High] sound/soc/cdns: inverted ALSA DAI master/slave format mapping
--

commit 56f3604b0b0ce570297b51aeb442be0b9378dd2b
Author: Joakim Zhang <joakim.zhang@cixtech.com>

ASoC: cdns: Add Cadence I2S-SC controller driver

This commit introduces the driver for the Cadence I2S-SC controller found in
the CIX SKY1 audio subsystem. It supports full-duplex stereo playback and
capture in standard I2S, left/right-justified, and DSP modes, as well as TDM
operation.

> diff --git a/sound/soc/cdns/cdns-i2s-sc.c b/sound/soc/cdns/cdns-i2s-sc.c
> new file mode 100644
> index 0000000000000..99ba7e34a06f1
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-sc.c

[ ... ]

> @@ -412,18 +412,18 @@ static irqreturn_t cdns_i2s_sc_isr(int irq, void *devid)
>  	if (regmap_read(i2s_sc_priv->regmap, I2S_STAT, &stat))
>  		return IRQ_NONE;
>  
>  	stat &= I2S_STAT_TDATA_UNDERR | I2S_STAT_RDATA_OVERR;
>  	if (!stat)
>  		return IRQ_NONE;
>  
>  	/* Clear the status */
>  	regmap_update_bits(i2s_sc_priv->regmap, I2S_STAT, stat, 0);

[Severity: High]
Does this create a race condition that can drop concurrent interrupts?

In cdns_i2s_sc_isr(), regmap_update_bits() performs a read-modify-write on a
volatile interrupt status register. If a new interrupt arrives between the
read and the write, writing back the modified value will incorrectly clear
the newly arrived interrupt or fail to clear the handled ones, depending on
whether the register is W0C or W1C.

Should this use regmap_write() directly to avoid losing hardware interrupts?

[ ... ]

> @@ -495,19 +495,19 @@ static int cdns_i2s_sc_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
>  		return -EINVAL;
>  	}
>  
>  	/* DAI hardware clock masters */
>  	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
>  	case SND_SOC_DAIFMT_CBC_CFC:
>  		ctrl &= ~I2S_CTRL_MS_CFG;
>  		i2s_sc_priv->is_master_mode = false;
>  		break;
>  	case SND_SOC_DAIFMT_CBP_CFP:
>  		ctrl |= I2S_CTRL_MS_CFG;
>  		i2s_sc_priv->is_master_mode = true;
>  		break;

[Severity: High]
Does this code invert the ALSA DAI master/slave format mapping?

When the machine driver requests SND_SOC_DAIFMT_CBC_CFC (Codec Clock
Consumer, meaning CPU is Master), the driver clears I2S_CTRL_MS_CFG and sets
is_master_mode = false, configuring the CPU DAI as a slave.

Conversely, when requested to be a slave (SND_SOC_DAIFMT_CBP_CFP), it
configures the CPU DAI as a master. Does this prevent correct audio
playback and capture?

[ ... ]

> @@ -950,20 +950,20 @@ static int cdns_i2s_sc_probe(struct platform_device *pdev)
>  	ret = cdns_i2s_sc_clks_enable(i2s_sc_priv);
>  	if (ret) {
>  		dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
>  		pm_runtime_put_noidle(&pdev->dev);
>  		return ret;
>  	}
>  
>  	cdns_i2s_sc_rst(i2s_sc_priv);
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;

[Severity: High]
Does this error path leak manually enabled clocks and the PM runtime usage
counter?

If platform_get_irq() fails, the function returns directly without dropping
the PM runtime reference (pm_runtime_put_noidle()) or disabling the clocks
(cdns_i2s_sc_clks_disable()). This prevents the device from entering low
power states and prevents clock gating if this error path is taken.

>  	i2s_sc_priv->irq = irq;
>  
>  	ret = devm_request_irq(&pdev->dev, irq, cdns_i2s_sc_isr, 0,
>  			       dev_name(&pdev->dev), i2s_sc_priv);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
>  		pm_runtime_put_noidle(&pdev->dev);
>  		return ret;
>  	}

[Severity: High]
Does this error path fail to disable the clocks?

If devm_request_irq() fails, it correctly drops the PM runtime reference by
calling pm_runtime_put_noidle(), but it fails to call
cdns_i2s_sc_clks_disable(). This leaves the clocks permanently enabled on
this error path.

[ ... ]

> @@ -1146,13 +1146,13 @@ static const struct of_device_id cdns_i2s_sc_of_match[] = {
>  MODULE_DEVICE_TABLE(of, cdns_i2s_sc_of_match);
>  
>  static struct platform_driver cdns_i2s_sc_driver = {
>  	.probe = cdns_i2s_sc_probe,
>  	.driver = {
>  		.name = DRV_NAME,
>  		.pm = pm_ptr(&cdns_i2s_sc_pm_ops),
>  		.of_match_table = cdns_i2s_sc_of_match,
>  	},
>  };

[Severity: High]
Is this missing a .remove callback to symmetrically tear down manually
enabled clocks and IRQ states?

In cdns_i2s_sc_driver, there is no .remove callback. If the driver is
unbound via sysfs or module removal, the driver core resumes the device (if
suspended) before unbind, but the manually enabled clocks are never
disabled.

Additionally, this could leave the hardware IRQ disabled permanently if
unbound while runtime-suspended, which triggers a kernel warning on
free_irq() and prevents the IRQ from being reused. Should a .remove callback
be added to properly release these resources?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928053613.2158446-1-joakim.zhang@cixtech.com?part=2