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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] driver: devres component unregistration races with manual PM disable
- [High] irq: regcache_cache_only enabled before irq disable causes storm
- [High] driver: missing NULL check for device_get_match_data()
- [High] irq: interrupt enabled before clocks are initialized
- [High] ALSA: prepare modifies shared clock state without lock
- [Medium] clk: clocks leak if CONFIG_PM is disabled
- [Medium] irq: regmap_update_bits on volatile status register drops interrupts
- [Low] pm: unconditionally assigned dev_pm_ops causes dead code
--

commit 85f61640451174adf94c765b39619f4cf7a43543
Author: Joakim Zhang <joakim.zhang@cixtech.com>
Subject: ASoC: cdns: Add Cadence I2S-SC controller driver

Adds ALSA SoC support for the Cadence I2S-SC controller in CIX SKY1. Implements
standard I2S and TDM modes, sharing transceiver start/stop state. Registers a
DMA engine PCM helper and handles PM operations.

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

[ ... ]

> +static irqreturn_t cdns_i2s_sc_isr(int irq, void *devid)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv = (struct cdns_i2s_sc_priv *)devid;
> +	struct device *dev = i2s_sc_priv->dev;
> +	u32 stat;
> +
> +	regmap_read(i2s_sc_priv->regmap, I2S_STAT, &stat);
> +
> +	stat &= I2S_STAT_TDATA_UNDERR | I2S_STAT_RDATA_OVERR;

[Severity: High]
Does this code use uninitialized stack memory?

If regmap_read() fails with -EBUSY (e.g. if the cache is in cache-only mode),
it will leave stat uninitialized. The ISR then checks stat, and if the
uninitialized memory happens to match the interrupt bits, it will attempt to
handle it and return IRQ_HANDLED, potentially leading to an unrecoverable
interrupt storm if the hardware interrupt is never cleared.

> +	if (!stat)
> +		return IRQ_NONE;
> +
> +	/* Clear the status */
> +	regmap_update_bits(i2s_sc_priv->regmap, I2S_STAT, stat, 0);

[Severity: Medium]
Will this read-modify-write drop concurrent interrupts?

I2S_STAT is a volatile hardware status register. If the hardware asserts a
new interrupt bit after regmap_update_bits() reads the register but before it
writes it back, the modified value written back will contain a 0 for that new
bit. Depending on whether the register is write-0-to-clear or write-1-to-clear,
this can accidentally clear the newly asserted interrupt or untouched bits,
causing them to be lost.

> +
> +	/* Transmitter status */
> +	if (stat & I2S_STAT_TDATA_UNDERR)
> +		dev_dbg(dev, "isr: tx data underrun\n");
> +
> +	/* Receiver status */
> +	if (stat & I2S_STAT_RDATA_OVERR)
> +		dev_dbg(dev, "isr: rx data overrun\n");
> +
> +	return IRQ_HANDLED;
> +}

[ ... ]

> +static int cdns_i2s_sc_prepare(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +
> +	/* Transceiver clock enable */
> +	regmap_update_bits(i2s_sc_priv->regmap, I2S_CTRL, I2S_CTRL_I2S_STB, 0);

[Severity: High]
Can concurrent streams unexpectedly modify the shared transceiver clock state
here?

cdns_i2s_sc_prepare() modifies the shared I2S_CTRL_I2S_STB bit and resets
the FIFO without holding i2s_sc_priv->lock. ALSA permits concurrent prepare
on one stream (like Capture) and trigger(STOP) on another (like Playback).

If Playback runs trigger(STOP) concurrently, it takes the lock and can disable
the transceiver clock immediately after Capture prepare enabled it. Capture
prepare would then attempt to reset the FIFO with the clock disabled, violating
hardware constraints.

> +
> +	/*
> +	 * FIFO reset should meet two requirements
> +	 * 1) after i2s_stb clock on
> +	 * 2) before dma ready to receive trigger
> +	 */
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		/*
> +		 * Transmitter FIFO reset
> +		 * When LOW, FIFO pointer is reset to zero. Threshold levels for
> +		 * FIFO are unchanged. This bit is automatically set to HIGH after
> +		 * one clock cycle.
> +		 * Deassert then assert this bit here, since I2S_CTRL register is not
> +		 * volatile, would not read from hardware any longer. If not, it would
> +		 * clear tx fifo every time when write this register.
> +		 */
> +		regmap_update_bits(i2s_sc_priv->regmap, I2S_CTRL, I2S_CTRL_FIFO_RST, 0);
> +		regmap_update_bits(i2s_sc_priv->regmap, I2S_CTRL,
> +				   I2S_CTRL_FIFO_RST, I2S_CTRL_FIFO_RST);
> +
> +		/* Transmitter FIFO threshold set */
> +		regmap_write(i2s_sc_priv->regmap, I2S_FIFO_AEMPTY,
> +			     i2s_sc_priv->devtype_data->tx_fifo_aempty_threshold);

[Severity: High]
Does this code trigger a NULL pointer dereference if the driver is bound
manually?

device_get_match_data() can return NULL if the driver is bound manually via
sysfs without a matching OF device node. Since i2s_sc_priv->devtype_data isn't
checked for NULL during probe, dereferencing it here to read FIFO threshold
values would cause an immediate kernel panic.

> +		regmap_write(i2s_sc_priv->regmap, I2S_FIFO_AFULL,
> +			     i2s_sc_priv->devtype_data->tx_fifo_afull_threshold);
> +	} else {

[ ... ]

> +static int cdns_i2s_sc_probe(struct platform_device *pdev)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv;
> +	struct resource *res;
> +	void __iomem *base;
> +	int i, irq, ret;
> +
> +	i2s_sc_priv = devm_kzalloc(&pdev->dev, sizeof(*i2s_sc_priv), GFP_KERNEL);
> +	if (!i2s_sc_priv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&i2s_sc_priv->lock);
> +
> +	platform_set_drvdata(pdev, i2s_sc_priv);
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	i2s_sc_priv->dev = &pdev->dev;
> +	i2s_sc_priv->devtype_data = device_get_match_data(&pdev->dev);

[ ... ]

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +	i2s_sc_priv->irq = irq;
> +
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_i2s_sc_isr, 0,
> +			       dev_name(&pdev->dev), i2s_sc_priv);

[Severity: High]
Is it safe to request and enable the interrupt before the hardware clocks are
initialized?

devm_request_irq() automatically unmasks the IRQ at the interrupt controller.
If a pending interrupt is asserted by the hardware (e.g. from a warm boot), the
ISR will execute immediately. Since the device clocks haven't been enabled yet
via cdns_i2s_sc_clks_enable(), the ISR's MMIO reads via regmap will typically
cause a severe bus stall or external abort, panicking the kernel during boot.

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2s_sc_priv->playback_dma_data.addr = res->start + I2S_FIFO_ADDRESS;

[ ... ]

> +	ret = cdns_i2s_sc_clks_enable(i2s_sc_priv);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> +		pm_runtime_disable(&pdev->dev);
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return ret;
> +	}
> +
> +	cdns_i2s_sc_rst(i2s_sc_priv);
> +
> +	/*
> +	 * Let pm_runtime_put_sync() disable the clocks, and take regcache effect.
> +	 * If CONFIG_PM is not enabled, the clock will stay powered.
> +	 */
> +	pm_runtime_put_sync(&pdev->dev);

[Severity: Medium]
Will this cause a resource leak if CONFIG_PM is disabled?

The driver explicitly enables clocks via cdns_i2s_sc_clks_enable() earlier
in probe, and relies on pm_runtime_put_sync() here (and 
pm_runtime_force_suspend() in remove()) to invoke the suspend callback and 
disable the clocks.

If CONFIG_PM is disabled, both of these functions become no-ops. The suspend
callback is never invoked, leaving the clocks permanently enabled.

> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &cdns_i2s_sc_component,
> +					      &cdns_i2s_sc_dai, 1);

[Severity: High]
Does registering the component with devres create a race condition with driver
removal?

The ALSA component is registered via devm_snd_soc_register_component(), meaning
it will be unregistered automatically after cdns_i2s_sc_remove() returns.

However, cdns_i2s_sc_remove() manually forces suspend and disables PM:

    static void cdns_i2s_sc_remove(struct platform_device *pdev)
    {
        if (!pm_runtime_status_suspended(&pdev->dev))
            pm_runtime_force_suspend(&pdev->dev);
        pm_runtime_disable(&pdev->dev);
    }

When devres eventually unregisters the ALSA component, the core may invoke
teardown actions (e.g. trigger(STOP)) on the DAI. These will attempt to access
the hardware or regmap while the device is in a powered-down, cache-only state,
potentially leading to bus hangs or panics.

> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "Failed to register soc component\n");
> +		pm_runtime_disable(&pdev->dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cdns_i2s_sc_remove(struct platform_device *pdev)
> +{
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		pm_runtime_force_suspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +}
> +
> +static int __maybe_unused cdns_i2s_sc_runtime_suspend(struct device *dev)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv = dev_get_drvdata(dev);
> +
> +	regcache_cache_only(i2s_sc_priv->regmap, true);
> +	disable_irq(i2s_sc_priv->irq);

[Severity: High]
Does setting cache-only mode before disabling the IRQ open a race window?

If an interrupt fires after regcache_cache_only(..., true) but before
disable_irq() completes, the ISR will attempt to read the hardware via regmap.
This read will fail with -EBUSY, leaving the stat variable uninitialized
and causing the interrupt storm issue mentioned earlier.

The same ordering race exists in cdns_i2s_sc_runtime_resume(), where
enable_irq() is called before cache-only mode is lifted.

> +	cdns_i2s_sc_clks_disable(i2s_sc_priv);
> +
> +	return 0;
> +}

[ ... ]

> +static struct platform_driver cdns_i2s_sc_driver = {
> +	.probe = cdns_i2s_sc_probe,
> +	.remove = cdns_i2s_sc_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = &cdns_i2s_sc_pm_ops,

[Severity: Low]
Is this unconditional assignment of dev_pm_ops causing dead code?

Assigning &cdns_i2s_sc_pm_ops unconditionally forces the struct and its callbacks
to be linked into the kernel even when CONFIG_PM is disabled. Using pm_ptr()
here prevents this.

> +		.of_match_table = cdns_i2s_sc_of_match,
> +	},
> +};
> +module_platform_driver(cdns_i2s_sc_driver);

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