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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ASoC: cdns: unbalanced clock enable in cdns_i2s_sc_probe() and on unbind
- [Medium] ASoC: cdns: wrong devres ordering for IRQ and PM in cdns_i2s_sc_probe()
- [Medium] ASoC: cdns: cdns_i2s_sc_set_tdm_slot() rejects slots == 0 to disable TDM
--

commit 8608e01e4e2208a8765fd0b404456a7e98eea27f
Author: Joakim Zhang <joakim.zhang@cixtech.com>

ASoC: cdns: Add Cadence I2S-SC controller driver

This commit adds an ASoC driver for the Cadence I2S-SC controller found in
the CIX SKY1 audio subsystem, supporting full-duplex stereo I2S and up to
16-slot TDM modes with dmaengine PCM and runtime/system PM support.

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

[ ... ]

> +static int cdns_i2s_sc_set_tdm_slot(struct snd_soc_dai *cpu_dai, unsigned int tx_mask,
> +				    unsigned int rx_mask, int slots, int slot_width)
> +{
> +	struct cdns_i2s_sc_priv *i2s_sc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_sc_priv->dev;
> +
> +	dev_dbg(dev, "tx_mask = 0x%x, rx_mask = 0x%x, slots = %d, slot_width = %d\n",
> +		tx_mask, rx_mask, slots, slot_width);
> +
> +	if (slots < 1 || slots > 16) {
> +		dev_err(i2s_sc_priv->dev, "TDM mode supports 1 to 16 slots\n");
> +		return -EINVAL;
> +	}
> +
> +	if (slot_width < 8 || slot_width > 32) {
> +		dev_err(i2s_sc_priv->dev, "TDM slot width must be 8 to 32 bits\n");
> +		return -EINVAL;
> +	}
> +
> +	if ((tx_mask >> slots) || (rx_mask >> slots)) {
> +		dev_err(i2s_sc_priv->dev, "TDM slot mask has bits outside the slots\n");
> +		return -EINVAL;
> +	}
> +
> +	i2s_sc_priv->tdm_config.slots = slots;
> +	i2s_sc_priv->tdm_config.slot_width = slot_width;
> +	i2s_sc_priv->tdm_config.rx_mask = rx_mask;
> +	i2s_sc_priv->tdm_config.tx_mask = tx_mask;
> +
> +	i2s_sc_priv->is_tdm_mode = true;
> +
> +	return 0;
> +}

[Severity: Medium]
As reported by Sashiko on v4 [1], snd_soc_dai_set_tdm_slot() allows callers
to disable TDM mode by passing slots == 0, in which case tx_mask, rx_mask,
and slot_width are ignored.

In cdns_i2s_sc_set_tdm_slot(), checking slots < 1 rejects slots == 0 with
-EINVAL, and nothing ever clears i2s_sc_priv->is_tdm_mode back to false.

Should cdns_i2s_sc_set_tdm_slot() handle !slots by setting
i2s_sc_priv->is_tdm_mode = false and returning 0?

[ ... ]

> +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;

[ ... ]

> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable runtime PM\n");
> +	}
> +
> +	ret = cdns_i2s_sc_clks_enable(i2s_sc_priv);
> +	if (ret) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> +	}
> +
> +	cdns_i2s_sc_rst(i2s_sc_priv);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto err_disable_clks;
> +	}
> +	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);
> +		goto err_disable_clks;
> +	}

[Severity: Medium]
As reported by Sashiko on v4 [2], devm_pm_runtime_enable() is registered in
cdns_i2s_sc_probe() before cdns_i2s_sc_clks_enable(), before
i2s_sc_priv->irq is initialized, and before devm_request_irq().

Because devres cleans up in reverse registration order, devm_request_irq()
will free the IRQ before devm_pm_runtime_enable() disables runtime PM on
unbind or probe failure. While runtime PM is still enabled after the IRQ is
freed, can cdns_i2s_sc_runtime_suspend() or cdns_i2s_sc_runtime_resume() run
and call disable_irq() or enable_irq() on i2s_sc_priv->irq?

Similarly, if cdns_i2s_sc_clks_enable(), platform_get_irq(), or
devm_request_irq() fails in cdns_i2s_sc_probe(), pm_runtime_put_noidle()
drops the usage count to zero while runtime PM is still enabled and marked
active, before devres disables runtime PM.

Would it be safer to request the IRQ and enable clocks (for example via
devm_clk_get_enabled() or a devm action) before enabling runtime PM?

> +
> +	/*
> +	 * 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);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev, &cdns_i2s_sc_component,
> +					      &cdns_i2s_sc_dai, 1);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register soc component\n");
> +
> +	return 0;
> +
> +err_disable_clks:
> +	cdns_i2s_sc_clks_disable(i2s_sc_priv);
> +	pm_runtime_put_noidle(&pdev->dev);
> +	return ret;
> +}

[ ... ]

> +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: Medium]
As reported by Sashiko on v4 [3], clk_hst and clk_i2s are acquired with
devm_clk_get() and enabled via cdns_i2s_sc_clks_enable() in
cdns_i2s_sc_probe(), relying on pm_runtime_put_sync() to invoke
cdns_i2s_sc_runtime_suspend() and disable the clocks.

When CONFIG_PM is disabled, or when runtime suspend is prevented (such as
via sysfs power/control), pm_runtime_put_sync() leaves clk_hst and clk_i2s
prepared and enabled. If devm_snd_soc_register_component() then fails, or
when cdns_i2s_sc_driver is unbound while runtime-active or with !CONFIG_PM
(since there is no .remove callback and devm_pm_runtime_enable() only calls
pm_runtime_disable()), won't clk_hst and clk_i2s remain prepared and
enabled, leaking their clock enable references?

Also, on the error paths in cdns_i2s_sc_probe() before pm_runtime_put_sync(),
should pm_runtime_set_suspended(&pdev->dev) be called to undo
pm_runtime_set_active(&pdev->dev)?

[1] https://lore.kernel.org/r/20261019122048.1107179-3-joakim.zhang@cixtech.com
[2] https://lore.kernel.org/r/20261019122048.1107179-3-joakim.zhang@cixtech.com
[3] https://lore.kernel.org/r/20261019122048.1107179-3-joakim.zhang@cixtech.com

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