[PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Tue, 12 Oct 2021 20:02:07 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> >> index a144a3f68e9e..e839459916ca 100644
> >> --- a/sound/core/pcm_lib.c
> >> +++ b/sound/core/pcm_lib.c
> >> @@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
> >> snd_pcm_substream *substream,
> >>  {
> >>         struct snd_pcm_runtime *runtime = substream->runtime;
> >>         snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> >> +       snd_pcm_sframes_t diff;
> >>         int ret;
> >>
> >>         if (old_appl_ptr == appl_ptr)
> >>                 return 0;
> >>
> >> +       /*
> >> +        * check if a rewind is requested by the application, after
> >> +        * verifying the new appl_ptr is in the 0..boundary range
> >> +        */
> >> +       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> >> +               if (appl_ptr >= runtime->boundary)
> >> +                       appl_ptr -= runtime->boundary;
> > 
> > The boundary check can (or should) be done unconditionally.
> > It was too naive to assume a sane appl_ptr passed always.
> > And, it can rather return an error.  So,
> > 
> > 	if (appl_ptr >= runtime->boundary)
> > 		return -EINVAL;
> 
> ok, but that would be a separate patch then since it impacts all users,
> even without the NO_REWINDS.

Makes sense.

> > 	/* check if a rewind is requested by the application */
> > 	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> > 		diff = appl_ptr - old_appl_ptr;
> > 		....
> > 
> >> +               if (diff >= 0) {
> >> +                       if (diff > runtime->buffer_size)
> >> +                               return 0;
> >> +               } else {
> >> +                       if (runtime->boundary + diff > runtime->buffer_size)
> >> +                               return 0;
> > 
> > I'm not sure whether we should return 0 here.  In snd_pcm_rewind() it
> > returns 0 due to application breakage, though.
> 
> We could return -EINVAL indeed, that would keep the work-around in place
> for PulseAudio.
> Even for other uses, it's not so bad: the selection of NO_REWINDS is an
> opt-in, and if a rewind still occurs a big fail would help detect a
> configuration issue.

Yeah, that was my gut feeling ,too.


Takashi