| Message ID | 1410633021-20395-7-git-send-email-patrakov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Sun, 14 Sep 2014 00:30:18 +0600, Alexander E. Patrakov wrote: > > Such negative returns are possible during an underrun if xrun detection > is disabled. > > So, don't store the result in an unsigned variable (where it will > overflow), and postpone the trigger in such case, too. > > Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> > --- > > The patch is only compile-tested and the second hunk may well be wrong. > > There are also similar issues in pcm_share.c, but, as I don't completely > understand the code there and cannot test that plugin at all due to > unrelated crashes, there will be no patch from me. In general, hw_avail must not be negative before starting the stream. If it is, then it means most likely the driver problem, so we should return error immediately instead. Takashi > > src/pcm/pcm_rate.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c > index b436a8e..736d558 100644 > --- a/src/pcm/pcm_rate.c > +++ b/src/pcm/pcm_rate.c > @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm) > static int snd_pcm_rate_start(snd_pcm_t *pcm) > { > snd_pcm_rate_t *rate = pcm->private_data; > - snd_pcm_uframes_t avail; > + snd_pcm_sframes_t avail; > > if (pcm->stream == SND_PCM_STREAM_CAPTURE) > return snd_pcm_start(rate->gen.slave); > @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm) > gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type); > > avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave); > - if (avail == 0) { > + if (avail <= 0) { > /* postpone the trigger since we have no data committed yet */ > rate->start_pending = 1; > return 0; > -- > 2.1.0 >
15.09.2014 14:49, Takashi Iwai ?????: > At Sun, 14 Sep 2014 00:30:18 +0600, > Alexander E. Patrakov wrote: >> >> Such negative returns are possible during an underrun if xrun detection >> is disabled. >> >> So, don't store the result in an unsigned variable (where it will >> overflow), and postpone the trigger in such case, too. >> >> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> >> --- >> >> The patch is only compile-tested and the second hunk may well be wrong. >> >> There are also similar issues in pcm_share.c, but, as I don't completely >> understand the code there and cannot test that plugin at all due to >> unrelated crashes, there will be no patch from me. > > In general, hw_avail must not be negative before starting the stream. > If it is, then it means most likely the driver problem, so we should > return error immediately instead. Thanks for the review. Would -EBADFD be a correct error code, or do you want something different? or maybe even an assert? > > > Takashi > >> >> src/pcm/pcm_rate.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c >> index b436a8e..736d558 100644 >> --- a/src/pcm/pcm_rate.c >> +++ b/src/pcm/pcm_rate.c >> @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm) >> static int snd_pcm_rate_start(snd_pcm_t *pcm) >> { >> snd_pcm_rate_t *rate = pcm->private_data; >> - snd_pcm_uframes_t avail; >> + snd_pcm_sframes_t avail; >> >> if (pcm->stream == SND_PCM_STREAM_CAPTURE) >> return snd_pcm_start(rate->gen.slave); >> @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm) >> gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type); >> >> avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave); >> - if (avail == 0) { >> + if (avail <= 0) { >> /* postpone the trigger since we have no data committed yet */ >> rate->start_pending = 1; >> return 0; >> -- >> 2.1.0 >>
At Mon, 15 Sep 2014 16:03:57 +0600, Alexander E. Patrakov wrote: > > 15.09.2014 14:49, Takashi Iwai ?????: > > At Sun, 14 Sep 2014 00:30:18 +0600, > > Alexander E. Patrakov wrote: > >> > >> Such negative returns are possible during an underrun if xrun detection > >> is disabled. > >> > >> So, don't store the result in an unsigned variable (where it will > >> overflow), and postpone the trigger in such case, too. > >> > >> Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> > >> --- > >> > >> The patch is only compile-tested and the second hunk may well be wrong. > >> > >> There are also similar issues in pcm_share.c, but, as I don't completely > >> understand the code there and cannot test that plugin at all due to > >> unrelated crashes, there will be no patch from me. > > > > In general, hw_avail must not be negative before starting the stream. > > If it is, then it means most likely the driver problem, so we should > > return error immediately instead. > > Thanks for the review. Would -EBADFD be a correct error code, or do you > want something different? or maybe even an assert? I'd take either EPIPE or EBADFD. An assert would be an overkill, IMO. Takashi > > > > > > > Takashi > > > >> > >> src/pcm/pcm_rate.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c > >> index b436a8e..736d558 100644 > >> --- a/src/pcm/pcm_rate.c > >> +++ b/src/pcm/pcm_rate.c > >> @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm) > >> static int snd_pcm_rate_start(snd_pcm_t *pcm) > >> { > >> snd_pcm_rate_t *rate = pcm->private_data; > >> - snd_pcm_uframes_t avail; > >> + snd_pcm_sframes_t avail; > >> > >> if (pcm->stream == SND_PCM_STREAM_CAPTURE) > >> return snd_pcm_start(rate->gen.slave); > >> @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm) > >> gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type); > >> > >> avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave); > >> - if (avail == 0) { > >> + if (avail <= 0) { > >> /* postpone the trigger since we have no data committed yet */ > >> rate->start_pending = 1; > >> return 0; > >> -- > >> 2.1.0 > >> > > > -- > Alexander E. Patrakov >
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index b436a8e..736d558 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -1058,7 +1058,7 @@ static snd_pcm_state_t snd_pcm_rate_state(snd_pcm_t *pcm) static int snd_pcm_rate_start(snd_pcm_t *pcm) { snd_pcm_rate_t *rate = pcm->private_data; - snd_pcm_uframes_t avail; + snd_pcm_sframes_t avail; if (pcm->stream == SND_PCM_STREAM_CAPTURE) return snd_pcm_start(rate->gen.slave); @@ -1069,7 +1069,7 @@ static int snd_pcm_rate_start(snd_pcm_t *pcm) gettimestamp(&rate->trigger_tstamp, pcm->tstamp_type); avail = snd_pcm_mmap_playback_hw_avail(rate->gen.slave); - if (avail == 0) { + if (avail <= 0) { /* postpone the trigger since we have no data committed yet */ rate->start_pending = 1; return 0;
Such negative returns are possible during an underrun if xrun detection is disabled. So, don't store the result in an unsigned variable (where it will overflow), and postpone the trigger in such case, too. Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> --- The patch is only compile-tested and the second hunk may well be wrong. There are also similar issues in pcm_share.c, but, as I don't completely understand the code there and cannot test that plugin at all due to unrelated crashes, there will be no patch from me. src/pcm/pcm_rate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)