RE: [PATCH alsa-lib 3/3] pcm: direct: Check xrun/suspend before the slave hwptr update

["S.J. Wang" <shengjiu.wang@xxxxxxx>]

Thanks.

> 
> The xrun/suspend may happen at any time and we should check it right
> before the slave hwptr update.  Otherwise the hwptr value may be screwed

I think should be "after the slave hwptr update".  If hwptr is screwed,  means
Suspend happened then check_xrun() can return.

> and get unexpected large read/write.
> 
> Reported-by: S.J. Wang <shengjiu.wang@xxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  src/pcm/pcm_dmix.c   | 4 ++--
>  src/pcm/pcm_dshare.c | 4 ++--
>  src/pcm/pcm_dsnoop.c | 6 +++---
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index
> d00d53bef604..111fea157228 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -426,11 +426,11 @@ static int snd_pcm_dmix_sync_ptr(snd_pcm_t
> *pcm)
>         snd_pcm_direct_t *dmix = pcm->private_data;
>         int err;
> 
> +       if (dmix->slowptr)
> +               snd_pcm_hwsync(dmix->spcm);
>         err = snd_pcm_direct_check_xrun(dmix, pcm);
>         if (err < 0)
>                 return err;
> -       if (dmix->slowptr)
> -               snd_pcm_hwsync(dmix->spcm);
> 
>         return snd_pcm_dmix_sync_ptr0(pcm, *dmix->spcm->hw.ptr);  } diff --

Better to get slave_hw_ptr before check_xrun(), like this:

--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -424,15 +424,17 @@ static int snd_pcm_dmix_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_ptr
 static int snd_pcm_dmix_sync_ptr(snd_pcm_t *pcm)
 {
        snd_pcm_direct_t *dmix = pcm->private_data;
+       snd_pcm_uframes_t slave_hw_ptr;
        int err;

        if (dmix->slowptr)
                snd_pcm_hwsync(dmix->spcm);
+       slave_hw_ptr = *dmix->spcm->hw.ptr;
        err = snd_pcm_direct_check_xrun(dmix, pcm);
        if (err < 0)
                return err;

-       return snd_pcm_dmix_sync_ptr0(pcm, *dmix->spcm->hw.ptr);
+       return snd_pcm_dmix_sync_ptr0(pcm, slave_hw_ptr);
 }


> git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index
> 0ff43a90d270..c46e8d6a53da 100644
> --- a/src/pcm/pcm_dshare.c
> +++ b/src/pcm/pcm_dshare.c
> @@ -201,11 +201,11 @@ static int snd_pcm_dshare_sync_ptr(snd_pcm_t
> *pcm)
>         snd_pcm_direct_t *dshare = pcm->private_data;
>         int err;
> 
> +       if (dshare->slowptr)
> +               snd_pcm_hwsync(dshare->spcm);
>         err = snd_pcm_direct_check_xrun(dshare, pcm);
>         if (err < 0)
>                 return err;
> -       if (dshare->slowptr)
> -               snd_pcm_hwsync(dshare->spcm);
> 
>         return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);  } diff

--- a/src/pcm/pcm_dshare.c
+++ b/src/pcm/pcm_dshare.c
@@ -199,15 +199,17 @@ static int snd_pcm_dshare_sync_ptr0(snd_pcm_t *pcm, snd_pcm_uframes_t slave_hw_p
 static int snd_pcm_dshare_sync_ptr(snd_pcm_t *pcm)
 {
        snd_pcm_direct_t *dshare = pcm->private_data;
+       snd_pcm_uframes_t slave_hw_ptr;
        int err;

        if (dshare->slowptr)
                snd_pcm_hwsync(dshare->spcm);
+       slave_hw_ptr = *dshare->spcm->hw.ptr;
        err = snd_pcm_direct_check_xrun(dshare, pcm);
        if (err < 0)
                return err;

-       return snd_pcm_dshare_sync_ptr0(pcm, *dshare->spcm->hw.ptr);
+       return snd_pcm_dshare_sync_ptr0(pcm, slave_hw_ptr);

> --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index
> 729ff447b41f..9abbbef2c1b6 100644
> --- a/src/pcm/pcm_dsnoop.c
> +++ b/src/pcm/pcm_dsnoop.c
> @@ -134,14 +134,14 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t
> *pcm)
>         snd_pcm_sframes_t diff;
>         int err;
> 
> -       err = snd_pcm_direct_check_xrun(dsnoop, pcm);
> -       if (err < 0)
> -               return err;
>         if (dsnoop->slowptr)
>                 snd_pcm_hwsync(dsnoop->spcm);
>         old_slave_hw_ptr = dsnoop->slave_hw_ptr;
>         snoop_timestamp(pcm);
>         slave_hw_ptr = dsnoop->slave_hw_ptr;
> +       err = snd_pcm_direct_check_xrun(dsnoop, pcm);
> +       if (err < 0)
> +               return err;
>         diff = pcm_frame_diff(slave_hw_ptr, old_slave_hw_ptr, dsnoop-
> >slave_boundary);
>         if (diff == 0)          /* fast path */
>                 return 0;
> --
> 2.34.1