linux-kernel - Re: [RESEND PATCH] ALSA: dmaengine_pcm: terminate dmaengine before synchronize

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: <CAA+D8APZ8-3NFceuQeTnEL-K4reUGGfrgyG63jyjydFA6o_4MA@mail.gmail.com>
Date: Fri, 21 Jun 2024 10:21:19 +0800
From: Shengjiu Wang <shengjiu.wang@...il.com>
To: Takashi Iwai <tiwai@...e.de>
Cc: Shengjiu Wang <shengjiu.wang@....com>, lars@...afoo.de, perex@...ex.cz, tiwai@...e.com, 
	broonie@...nel.org, linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org, 
	alsa-devel@...a-project.org
Subject: Re: [RESEND PATCH] ALSA: dmaengine_pcm: terminate dmaengine before synchronize

On Thu, Jun 20, 2024 at 3:56 PM Takashi Iwai <tiwai@...e.de> wrote:
>
> On Thu, 20 Jun 2024 04:40:18 +0200,
> Shengjiu Wang wrote:
> >
> > When dmaengine supports pause function, in suspend state,
> > dmaengine_pause() is called instead of dmaengine_terminate_async(),
> >
> > In end of playback stream, the runtime->state will go to
> > SNDRV_PCM_STATE_DRAINING, if system suspend & resume happen
> > at this time, application will not resume playback stream, the
> > stream will be closed directly, the dmaengine_terminate_async()
> > will not be called before the dmaengine_synchronize(), which
> > violates the call sequence for dmaengine_synchronize().
>
> Hmm, I can't follow this state change.
> Do you mean that:
> - snd_pcm_drain() is performed for a playback stream
> - while draining operation, the system goes to suspend
> - the system resumes (but the application doesn't call resume yet)
> - The stream is closed (without calling resume)
> ??

yes. this is the case.

>
> If so, it's rather an inconsistent PCM state in the core side, and can
> be fixed by a simple call like below:
>
> -- 8< --
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2700,6 +2700,7 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
>         if (substream->ref_count > 0)
>                 return;
>
> +       snd_pcm_resume(substream);
>         snd_pcm_drop(substream);
>         if (substream->hw_opened) {
>                 if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
> -- 8< --
>
> This will be no-op for the PCM device without SNDRV_PCM_INFO_RESUME.
>
> But, this may need more rework, too; admittedly it imposes the
> unnecessary resume of the stream although it shall be stopped and
> closed immediately after that.  We may have some optimization in
> addition.

The suspended_state is not cleared that the resume may be called again
at the end of stream.

Will you push the code?

Best regards
Shengjiu Wang

>
>
> thanks,
>
> Takashi