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

On Thu, 10 Mar 2022 10:35:25 +0100, S.J. Wang wrote:

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@nxp.com Signed-off-by: Takashi Iwai tiwai@suse.de

---

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);
  

}

Makes sense. I'll respin v2 with those changes.

thanks,

Takashi