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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] ASoC: cdns: inverted clock provider/consumer configuration
- [High] ASoC: cdns: unprotected clock reparenting disrupts concurrent streams
- [High] ASoC: cdns: PM usage counter and clock leaks in probe error paths and unbind
- [High] ASoC: cdns: synchronizing unit resets bypassed by incomplete regmap mask
- [High] ASoC: cdns: unsafe read-modify-write on interrupt status register
- [High] ASoC: cdns: inconsistent DATA_WS_DEL alignment logic across formats
- [Medium] ASoC: cdns: unbalanced disable_irq() corrupts global IRQ descriptor
--

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

ASoC: cdns: Add Cadence I2S-MC controller driver

Add support for the Cadence I2S-MC multi-channel controller found in
the CIX SKY1 audio subsystem. The driver registers the dmaengine PCM
helper, selects the audio reference clock parent and implements runtime
PM and system suspend/resume.

> diff --git a/sound/soc/cdns/cdns-i2s-mc.c b/sound/soc/cdns/cdns-i2s-mc.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/cdns/cdns-i2s-mc.c
[ ... ]
> +static irqreturn_t cdns_i2s_mc_isr(int irq, void *devid)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = (struct cdns_i2s_mc_priv *)devid;
> +	struct device *dev = i2s_mc_priv->dev;
> +	u32 stat;
> +
> +	if (regmap_read(i2s_mc_priv->regmap, I2S_INTR_STAT, &stat))
> +		return IRQ_NONE;
> +
> +	stat &= I2S_INTR_STAT_TDATA_UNDERR | I2S_INTR_STAT_RDATA_OVERR;
> +	if (!stat)
> +		return IRQ_NONE;
> +
> +	/* Clear the status */
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_INTR_STAT, stat, 0);

[Severity: High]
Does this read-modify-write cycle correctly handle write-1-to-clear 
semantics? Using regmap_update_bits() reads the register, writes 0 to the
handled bits, and writes the preserved 1s to unhandled bits. If the
register is write-1-to-clear, this could fail to clear handled interrupts
and erroneously clear newly arrived interrupts without handling them. Could
a direct regmap_write() be used here instead?

> +
> +	/* Transmitter status */
> +	if (stat & I2S_INTR_STAT_TDATA_UNDERR)
> +		dev_dbg(dev, "isr: tx data underrun\n");
[ ... ]
> +static int cdns_i2s_mc_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int 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;

[Severity: High]
Is this delay phase mapping contradictory? The I2S_DEV_CONF_TRAN_DATA_WS_DEL
bit is cleared for SND_SOC_DAIFMT_I2S (1-clock delay) but set for
SND_SOC_DAIFMT_DSP_A (also 1-clock delay). Conversely, it is set for
SND_SOC_DAIFMT_LEFT_J (0-clock delay) but cleared for
SND_SOC_DAIFMT_DSP_B (also 0-clock delay). This seems to result in
misaligned phase for at least two of the supported formats.

> +		default:
> +			return -EINVAL;
> +		}
> +
> +		/* DAI hardware clock masters */
> +		switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
> +		case SND_SOC_DAIFMT_CBC_CFC:
> +			ctrl &= ~I2S_CTRL_T_MS;
> +			break;

[Severity: High]
Does this invert the master/slave configuration? For SND_SOC_DAIFMT_CBC_CFC
(Codec Bit/Frame Clock Consumer), the CPU DAI must act as the clock
provider (Master), but this clears the I2S_CTRL_T_MS bit.

[ ... ]
> +	} else if (cpu_dai->id == I2S_MC_AIF2) {
[ ... ]
> +		/* DAI hardware clock masters */
> +		switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
> +		case SND_SOC_DAIFMT_CBC_CFC:
> +			ctrl &= ~I2S_CTRL_R_MS;
> +			break;

[Severity: High]
Similarly, this clears the master bit for the receiver DAI when it should
be set for SND_SOC_DAIFMT_CBC_CFC.

[ ... ]
> +static int cdns_i2s_mc_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *cpu_dai)
> +{
[ ... ]
> +	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);

[Severity: High]
Can this reparenting disrupt a concurrent audio stream? Since both playback
(I2S_MC_AIF1) and capture (I2S_MC_AIF2) share the single clk_i2s clock,
starting a new stream will dynamically reparent the bit clock while the
companion stream might be active, potentially causing data corruption.

[ ... ]
> +	regmap_read(i2s_mc_priv->regmap, I2S_CTRL, &ctrl);
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		is_master_mode = ctrl & I2S_CTRL_T_MS;
> +	else
> +		is_master_mode = ctrl & I2S_CTRL_R_MS;
> +
> +	if (is_master_mode) {
> +		if (i2s_clk_rate < rate * 2 * slot_width * I2S_CLK_RATIO_MIN_MASTER) {

[Severity: High]
Because I2S_CTRL_T_MS and I2S_CTRL_R_MS are erroneously cleared for
CBC_CFC in cdns_i2s_mc_set_fmt(), does this validate the clock frequency
against the wrong constraints? The inverted logic cascades here, checking
the slave mode multipliers when it should be checking the master mode.

[ ... ]
> +static int cdns_i2s_mc_dai_probe(struct snd_soc_dai *cpu_dai)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> +	u32 ctrl = 0, cid_ctrl = 0;
> +
> +	/*
> +	 * Transceiver disable
> +	 * Transceiver clock disable
> +	 * All interrupts masked
> +	 * Reset for transmitter synchronizing unit
> +	 * Reset for receiver synchronizing unit
> +	 */
> +	ctrl |= FIELD_PREP(I2S_CTRL_I2S_EN, 0);
> +	ctrl &= ~(I2S_CTRL_TSYNC_RST | I2S_CTRL_RSYNC_RST);
> +	cid_ctrl |= I2S_CID_CTRL_STROBE_TS | I2S_CID_CTRL_STROBE_RS;
> +	cid_ctrl &= ~I2S_CID_CTRL_INTREQ_MASK;
> +
> +	regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL, I2S_CTRL_I2S_EN, ctrl);

[Severity: High]
Does this mask inadvertently bypass the hardware reset sequence? The
I2S_CTRL_I2S_EN mask only covers bits 0-7 (GENMASK(7, 0)), completely
filtering out the I2S_CTRL_TSYNC_RST (bit 25) and I2S_CTRL_RSYNC_RST
(bit 26) modifications made to ctrl just above.

[ ... ]
> +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)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = cdns_i2s_mc_clks_enable(i2s_mc_priv);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "Failed to enable clocks\n");
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return ret;
> +	}
> +
> +	cdns_i2s_mc_rst(i2s_mc_priv);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;

[Severity: High]
Does this error path leak the PM usage counter and leave the clocks enabled?
The pm_runtime_get_noresume() and cdns_i2s_mc_clks_enable() calls complete
successfully above, but this return does not clean them up.

> +	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);
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return ret;
> +	}

[Severity: High]
Similar to the issue above, this drops the PM counter but appears to miss
calling cdns_i2s_mc_clks_disable(), which permanently leaks the enabled
clocks.

[ ... ]
> +static int cdns_i2s_mc_runtime_suspend(struct device *dev)
> +{
> +	struct cdns_i2s_mc_priv *i2s_mc_priv = dev_get_drvdata(dev);
> +
> +	/*
> +	 * Disable the IRQ before switching the regcache to cache-only, so an
> +	 * in-flight handler can never access the registers through a
> +	 * cache-only regmap. disable_irq() waits for the handler to finish.
> +	 */
> +	disable_irq(i2s_mc_priv->irq);

[Severity: Medium]
Could this lead to an unbalanced IRQ disable depth? If the probe sequence
fails later (e.g., at devm_snd_soc_register_component) or if the device
is unbound while suspended, devm will free the IRQ while it is still deeply
disabled. Freeing a disabled IRQ does not reverse the global descriptor's
disable depth counter, leaving it permanently disabled for future requesters.

[ ... ]
> +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,
> +	},
> +};

[Severity: High]
Is this missing a .remove callback? Without one, unbinding the module
will leave the clocks running permanently and leak hardware resources.

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