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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 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 unbind
- [Medium] ASoC: cdns: devm_pm_runtime_enable() ordered before IRQ in cdns_i2s_mc_probe()
- [Medium] ASoC: cdns: inverted DATA_WS_DEL bit for DSP_A and DSP_B in cdns_i2s_mc_set_fmt()
--

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

ASoC: cdns: Add Cadence I2S-MC controller driver

This commit adds an ASoC driver for the Cadence I2S-MC multi-channel
controller in the CIX SKY1 audio subsystem, supporting up to 16 channels
across 8 configurable data pins with independent transmit and receive DAIs.

> 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

[ ... ]

> +static int cdns_i2s_mc_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 ctrl = 0, dev_conf = 0;
> +
> +	dev_dbg(dev, "fmt = 0x%x\n", fmt);

[ ... ]

> +		/* DAI hardware audio formats */
> +		switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +		case SND_SOC_DAIFMT_I2S:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_DATA_WS_DEL |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_RIGHT_J:
> +			dev_conf |= I2S_DEV_CONF_TRAN_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_I2S_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_LEFT_J:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR | I2S_DEV_CONF_TRAN_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_DSP_A:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR | I2S_DEV_CONF_TRAN_DATA_WS_DEL |
> +				    I2S_DEV_CONF_TRAN_WS_DSP_MODE;
> +			dev_conf &= ~I2S_DEV_CONF_TRAN_APB_ALIGN_LR;
> +			break;
> +		case SND_SOC_DAIFMT_DSP_B:
> +			dev_conf |= I2S_DEV_CONF_TRAN_I2S_ALIGN_LR | I2S_DEV_CONF_TRAN_WS_DSP_MODE;
> +			dev_conf &= ~(I2S_DEV_CONF_TRAN_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_TRAN_DATA_WS_DEL);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}

[ ... ]

> +		/* DAI hardware audio formats */
> +		switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +		case SND_SOC_DAIFMT_I2S:
> +			dev_conf |= I2S_DEV_CONF_REC_I2S_ALIGN_LR;
> +			dev_conf &= ~(I2S_DEV_CONF_REC_APB_ALIGN_LR | I2S_DEV_CONF_REC_DATA_WS_DEL |
> +				      I2S_DEV_CONF_REC_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_RIGHT_J:
> +			dev_conf |= I2S_DEV_CONF_REC_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_REC_I2S_ALIGN_LR |
> +				      I2S_DEV_CONF_REC_APB_ALIGN_LR |
> +				      I2S_DEV_CONF_REC_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_LEFT_J:
> +			dev_conf |= I2S_DEV_CONF_REC_I2S_ALIGN_LR | I2S_DEV_CONF_REC_DATA_WS_DEL;
> +			dev_conf &= ~(I2S_DEV_CONF_REC_APB_ALIGN_LR | I2S_DEV_CONF_REC_WS_DSP_MODE);
> +			break;
> +		case SND_SOC_DAIFMT_DSP_A:
> +			dev_conf |= I2S_DEV_CONF_REC_I2S_ALIGN_LR | I2S_DEV_CONF_REC_DATA_WS_DEL |
> +				    I2S_DEV_CONF_REC_WS_DSP_MODE;
> +			dev_conf &= ~I2S_DEV_CONF_REC_APB_ALIGN_LR;
> +			break;
> +		case SND_SOC_DAIFMT_DSP_B:
> +			dev_conf |= I2S_DEV_CONF_REC_I2S_ALIGN_LR | I2S_DEV_CONF_REC_WS_DSP_MODE;
> +			dev_conf &= ~(I2S_DEV_CONF_REC_APB_ALIGN_LR | I2S_DEV_CONF_REC_DATA_WS_DEL);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}

[Severity: Medium]
Are the data-to-word-select delay bits in dev_conf inverted between the
dsp_a and dsp_b cases in cdns_i2s_mc_set_fmt() for both the transmit and
receive DAIs?

In the standard i2s case above, clearing the delay bit in dev_conf selects a
1-bit-clock delay after the word-select transition, whereas in the left_j
and right_j cases, setting the delay bit selects a 0-bit-clock delay.

However, the dsp_a case sets the delay bit (0-bit delay) and the dsp_b case
clears it (1-bit delay), which shifts transmitted and received samples by
one bit clock cycle relative to the standard dsp_a (1-bit delay) and dsp_b
(0-bit delay) timings.

[ ... ]

> +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]
As reported by Sashiko on v4 [1], what happens if playback on i2s-mc-aif1
and capture on i2s-mc-aif2 are configured concurrently with sample rates from
different clock families (for example, 48 kHz on one DAI and 44.1 kHz on the
other)?

Because cdns_i2s_mc_dai[] exposes two independent DAIs sharing the single
i2s_mc_priv->clk_i2s clock without cross-DAI rate family constraints or an
active-stream check, calling clk_set_parent(i2s_mc_priv->clk_i2s, clk_parent)
in cdns_i2s_mc_hw_params() switches the parent clock underneath an already
active stream on the other DAI.

Can this corrupt the running stream's sample rate divider in the sample rate
register and violate the minimum functional-clock to bit-clock ratio?

[ ... ]

> +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]
As reported by Sashiko on v4 [2], since devm_pm_runtime_enable() is
registered before i2s_mc_priv->irq is initialized and before
devm_request_irq() is called in cdns_i2s_mc_probe(), devres teardown on
driver unbind or late probe failure will free the interrupt before disabling
runtime power management.

During teardown, can cdns_i2s_mc_runtime_resume() or
cdns_i2s_mc_runtime_suspend() still run and call enable_irq() or
disable_irq() on i2s_mc_priv->irq after it has already been freed?

> +
> +	/*
> +	 * 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]
As reported by Sashiko on v4 [3], does cdns_i2s_mc_probe() leak the prepared
and enabled clock references on i2s_mc_priv->clk_hst and
i2s_mc_priv->clk_i2s when runtime power management is disabled in the kernel
configuration or prevented via sysfs?

Specifically:

1. If devm_snd_soc_register_component() fails when pm_runtime_put_sync() did
not suspend the device, cdns_i2s_mc_probe() returns without calling
cdns_i2s_mc_clks_disable(i2s_mc_priv).

2. Because cdns_i2s_mc_driver defines no remove callback and registers no
devm action to disable the clocks, unbinding the driver while the device is
runtime-active or when power management is disabled leaves clk_hst and
clk_i2s enabled when devm_clk_get() releases them.

3. On the earlier error paths after pm_runtime_set_active(&pdev->dev),
including err_disable_clks, pm_runtime_set_suspended(&pdev->dev) is not
called, leaving the runtime power management state marked active with clocks
disabled.

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

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