linux-kernel - RE: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <NTZPR01MB09567BE742A91B8C9E02EF4F9F08A@NTZPR01MB0956.CHNPR01.prod.partner.outlook.cn>
Date: Tue, 16 Apr 2024 07:23:57 +0000
From: Xingyu Wu <xingyu.wu@...rfivetech.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>, Liam Girdwood
	<lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Claudiu Beznea
	<Claudiu.Beznea@...rochip.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi
 Iwai <tiwai@...e.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor.dooley@...rochip.com>
CC: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>
Subject:
 RE: 回复: [PATCH v2 2/2] ASoC: cdns: Add drivers of Cadence Multi-Channel I2S Controller

On 02/04/2024 21:57, Pierre-Louis Bossart wrote:
> 
> 
> >>
> >>> +#define PERIODS_MIN		2
> >>> +
> >>> +static unsigned int cdns_i2s_pcm_tx(struct cdns_i2s_dev *dev,
> >>> +				    struct snd_pcm_runtime *runtime,
> >>> +				    unsigned int tx_ptr, bool *period_elapsed,
> >>> +				    snd_pcm_format_t format)
> >>> +{
> >>> +	unsigned int period_pos = tx_ptr % runtime->period_size;
> >>
> >> not following what the modulo is for, usually it's modulo the buffer size?
> >
> > This is to see if the new data is divisible by period_size and to
> > determine whether it is enough for a period_size in the later loop.
> 
> That didn't answer to my question, the position is usually between
> 0..buffer_size.1.

Yes, this position will be used later in the cdns_i2s_pcm_pointer().
But this cdns_i2s_pcm_tx() is called by I2S hardware interrupt which
would be frequently called several times each period. The period_pos
is used to determine whether there is enough a period_size to call
snd_pcm_period_elapsed().

> 
> Doing increments on a modulo value then comparisons as done below seems
> rather questionable.
> 
> >>> +
> >>> +		iowrite32(data[0], dev->base + CDNS_FIFO_MEM);
> >>> +		iowrite32(data[1], dev->base + CDNS_FIFO_MEM);
> >>> +		period_pos++;
> >>> +		if (++tx_ptr >= runtime->buffer_size)
> >>> +			tx_ptr = 0;
> >>> +	}
> >>> +
> >>> +	*period_elapsed = period_pos >= runtime->period_size;
> >>> +	return tx_ptr;
> >>> +}
> 
> >>> +	pm_runtime_enable(&pdev->dev);
> >>> +	if (pm_runtime_enabled(&pdev->dev))
> >>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>
> >> that sequence looks suspicious.... Why would you suspend immediately
> >> during the probe? You're probably missing all the autosuspend stuff?
> >
> > Since I have enabled clocks before, and the device is in the suspend
> > state after pm_runtime_enable(), I need to disable clocks in
> > cdns_i2s_runtime_suspend() to match the suspend state.
> 
> That is very odd on two counts
> a) if you haven't enabled the clocks, why do you need to disbale them?
> b) if you do a pm_runtime_enable(), then the branch if
> (pm_runtime_enabled) is always true.
> 

a) It must enable clocks first to read and write registers when I2S probe.
Then it is done to probe, the clocks are still enabled and the state of pm
is suspend. So it need to be disabled to match the state and will resume
and be enabled by ALSA.

b) Because CONFIG_PM would be disabled and pm_runtime_enabled()
return false , then it is no need to disable clock and I2S still can work.

> 
> >
> >>
> >>> +
> >>> +	dev_dbg(&pdev->dev, "I2S supports %d stereo channels with %s.\n",
> >>> +		i2s->max_channels, ((i2s->irq < 0) ? "dma" : "interrupt"));
> >>> +
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int cdns_i2s_remove(struct platform_device *pdev) {
> >>> +	pm_runtime_disable(&pdev->dev);
> >>> +	if (!pm_runtime_status_suspended(&pdev->dev))
> >>> +		cdns_i2s_runtime_suspend(&pdev->dev);
> >>
> >> ... and this one too. Once you've disabled pm_runtime, checking the
> >> status is irrelevant...
> >
> > I think the clocks need to be always enabled after probe if disable
> > pm_runtime, and should be disabled when remove. This will do that.
> 
> if you are disabling pm_runtime, then the pm_runtime state becames invalid.
> When pm_runtime_disable() is added in remove operations, it's mainly to
> prevent the device from suspending.

Should I use the pm_runtime_enabled() before the pm_runtime_disable()?

Best regards,
Xingyu Wu