linux-kernel - Re: [PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream start

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]

Date:   Fri, 8 Mar 2019 12:58:06 -0800
From:   Nicolin Chen <nicoleotsuka@...il.com>
To:     Daniel Baluta <daniel.baluta@....com>
Cc:     "broonie@...nel.org" <broonie@...nel.org>,
        "festevam@...il.com" <festevam@...il.com>,
        "timur@...nel.org" <timur@...nel.org>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "S.j. Wang" <shengjiu.wang@....com>
Subject: Re: [PATCH v2] ASoC: fsl: sai: Fix underrrun for playback stream
 start

On Fri, Mar 08, 2019 at 05:39:30PM +0000, Daniel Baluta wrote:

> @@ -542,6 +544,11 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		for (i = 0; tx && i < channels; i++)
> +			regmap_write(sai->regmap, FSL_SAI_TDR, 0x0);

Though it could reduce underrrun for sure, I don't feel it's a
thorough fix, especially when dealing with a duplex case where
RX is the first stream while the driver enables both TE and RE
at the same time. So the transmitter might have started before
TX DMA request gets enabled.

We actually have something similar in the ESAI driver as that
one is well documented. But SAI is apparently more tricky at
the trigger() function. And the paragraph you pasted from the
RM doesn't explicitly sound like we should do this.

Btw, Instead of writing dummy data, have you tried polling the
WFP pointer of the TFR0 register to make sure data is copied
by the DMA?

> +		if (tx)
> +			udelay(10);

Any justification for the delay? Missing a line of comments? I
guess it's to address the 3-bit clocks timing. However, putting
it before enabling DMA request doesn't make much sense to me.

> +
>  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
>  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
>  
> -- 
> 2.17.1
>