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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ASoC: cdns: shared clk_i2s reparented in cdns_i2s_mc_hw_params() while active
- [Medium] ASoC: cdns: clock and runtime PM leak in cdns_i2s_mc_probe() and on remove
- [Medium] ASoC: cdns: devm_pm_runtime_enable() ordered before IRQ in cdns_i2s_mc_probe()
--

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

ASoC: cdns: Add Cadence I2S-MC controller driver

This commit adds ASoC driver support for the Cadence I2S-MC multi-channel
controller found in the CIX SKY1 audio subsystem. It exposes playback and
capture DAIs using dmaengine PCM, manages audio reference clock parenting,
and handles runtime and system power management.

> diff --git a/sound/soc/cdns/cdns-i2s-mc.c b/sound/soc/cdns/cdns-i2s-mc.c
> new file mode 100644
> index 0000000000000..defbb93adfe76
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-mc.c
> @@ -0,0 +1,1128 @@

[ ... ]

> +static int cdns_i2s_mc_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 rate, sample_rate = 0;
> +	u32 slots, slot_width, resolution, ctrl;
> +	unsigned long i2s_clk_rate;
> +	u8 pin_tx_num, pin_rx_num;
> +	struct clk *clk_parent;
> +	bool is_master_mode;
> +	int ret;
> +
> +	rate = params_rate(params);
> +	slot_width = i2s_mc_priv->devtype_data->data_width;
> +	dev_dbg(dev, "rate = %d, slot_width = %d\n", rate, slot_width);
> +
> +	/* switch clk mux to select the appropriate clk parent */
> +	if (rate % 8000 == 0) {
> +		/* Sampling rate is a multiple of 8KHz, select "audio-clk0" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK0];
> +
> +		ret = clk_set_parent(i2s_mc_priv->clk_i2s, clk_parent);
> +	} else if (rate % 11025 == 0) {
> +		/* Sampling rate is a multiple of 11.025KHz, select "audio-clk2" */
> +		clk_parent = i2s_mc_priv->clks[AUDIO_CLK2];
> +
> +		ret = clk_set_parent(i2s_mc_priv->clk_i2s, clk_parent);
> +	} else {
> +		dev_err(i2s_mc_priv->dev, "Invalid sample rate\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]
Can switching the parent of i2s_mc_priv->clk_i2s here corrupt an already
active stream on the other DAI?

The driver exposes two separate DAIs in cdns_i2s_mc_dai[] (i2s-mc-aif1 for
playback and i2s-mc-aif2 for capture), which can be opened and configured
independently. Both DAIs share the single functional clock
i2s_mc_priv->clk_i2s.

If one DAI is running at a multiple of 8 kHz (using clks[AUDIO_CLK0]) and
userspace calls snd_pcm_hw_params() -> cdns_i2s_mc_hw_params() on the second
DAI with a multiple of 11.025 kHz (using clks[AUDIO_CLK2]), clk_set_parent()
will change the parent of clk_i2s underneath the active stream. That changes
i2s_clk_rate while the first stream is active, invalidating its programmed
I2S_SRR divider (I2S_SRR_TSAMPLE_RATE or I2S_SRR_RSAMPLE_RATE) and
potentially violating I2S_CLK_RATIO_MIN_MASTER or I2S_CLK_RATIO_MIN_SLAVE.

Should cdns_i2s_mc_hw_params() check whether the other stream is already
configured with an incompatible clock parent, or constrain the two DAIs to
the same rate family when both are active?

[ ... ]

> +static int cdns_i2s_mc_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	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_mc_clks_enable(i2s_mc_priv);
> +	if (ret) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> +	}
> +
> +	cdns_i2s_mc_rst(i2s_mc_priv);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto err_disable_clks;
> +	}
> +	i2s_mc_priv->irq = irq;
> +
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_i2s_mc_isr, 0,
> +			       dev_name(&pdev->dev), i2s_mc_priv);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
> +		goto err_disable_clks;
> +	}

[Severity: Medium]
Is the devres registration order between devm_pm_runtime_enable() and
devm_request_irq() safe during teardown?

In cdns_i2s_mc_probe(), devm_pm_runtime_enable() is registered before
i2s_mc_priv->irq is assigned and before devm_request_irq() is called.
Because devres unwinds in reverse order, on driver unbind or if
devm_snd_soc_register_component() fails, devm_irq_release() calls free_irq()
before pm_runtime_disable_action() disables runtime PM.

Since cdns_i2s_mc_runtime_suspend() calls disable_irq(i2s_mc_priv->irq) and
cdns_i2s_mc_runtime_resume() calls enable_irq(i2s_mc_priv->irq), a runtime
PM transition occurring before runtime PM is disabled can call enable_irq()
or disable_irq() on an already-freed IRQ. It also leaves the IRQ disabled
when devm_irq_release() frees it.

Should platform_get_irq() and devm_request_irq() be moved before
devm_pm_runtime_enable()?

> +
> +	/*
> +	 * 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_mc_component,
> +					      cdns_i2s_mc_dai, ARRAY_SIZE(cdns_i2s_mc_dai));
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register soc component\n");
> +
> +	return 0;
> +
> +err_disable_clks:
> +	cdns_i2s_mc_clks_disable(i2s_mc_priv);
> +	pm_runtime_put_noidle(&pdev->dev);
> +	return ret;
> +}

[Severity: Medium]
Does cdns_i2s_mc_probe() leak the prepare/enable counts on clk_hst and
clk_i2s when devm_snd_soc_register_component() fails or when the driver is
unbound?

In cdns_i2s_mc_probe(), clk_hst and clk_i2s are enabled via
cdns_i2s_mc_clks_enable(), and the driver relies on pm_runtime_put_sync()
running cdns_i2s_mc_runtime_suspend() to disable them. There is no devm
action or .remove callback in cdns_i2s_mc_driver to disable the clocks.

When CONFIG_PM is disabled (where pm_runtime_put_sync() is a no-op and the
clocks stay powered) or when runtime suspend is prevented (for example via
sysfs power/control), if devm_snd_soc_register_component() fails,
cdns_i2s_mc_probe() returns without calling cdns_i2s_mc_clks_disable().
The same leak occurs on driver unbind because devm_pm_runtime_enable() only
calls pm_runtime_disable() and does not invoke cdns_i2s_mc_runtime_suspend()
when the device is still runtime-active.

Also, on the error paths after pm_runtime_set_active(&pdev->dev) (when
devm_pm_runtime_enable(), cdns_i2s_mc_clks_enable(), platform_get_irq(), or
devm_request_irq() fails), pm_runtime_set_suspended(&pdev->dev) is not
called, leaving the runtime PM status marked active while the clocks are
disabled.

[ ... ]

> +static struct platform_driver cdns_i2s_mc_driver = {
> +	.probe = cdns_i2s_mc_probe,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = pm_ptr(&cdns_i2s_mc_pm_ops),
> +		.of_match_table = cdns_i2s_mc_of_match,
> +	},
> +};

[ ... ]

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