diff mbox

[6/9] rate: handle negative values from snd_pcm_mmap_playback_hw_avail

Message ID 54185CD3.3030701@gmail.com (mailing list archive)
State Accepted
Delegated to: Takashi Iwai
Headers show

Commit Message

Alexander Patrakov Sept. 16, 2014, 3:52 p.m. UTC 
15.09.2014 16:14, Takashi Iwai wrote:
> 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.

I have sent the fix to the list, but nobody reacted. Resending as an 
attachment to this message.

Comments

Jaroslav Kysela Sept. 16, 2014, 5:26 p.m. UTC | #1 
Date 16.9.2014 17:52, Alexander E. Patrakov wrote:
> 15.09.2014 16:14, Takashi Iwai wrote:
>> 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.
> 
> I have sent the fix to the list, but nobody reacted. Resending as an 
> attachment to this message.

Applied now. Thanks.

                         Jaroslav

> 
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Takashi Iwai Sept. 16, 2014, 7:18 p.m. UTC | #2 
At Tue, 16 Sep 2014 21:52:51 +0600,
Alexander E. Patrakov wrote:
> 
> 15.09.2014 16:14, Takashi Iwai wrote:
> > 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.
> 
> I have sent the fix to the list, but nobody reacted. Resending as an 
> attachment to this message.

I'm traveling in this week, so please don't expect reactions in light
speed.


Takashi
diff mbox

Patch

From 81d3b1b107e0f55f041eb9df8f6c3edd3e6450f4 Mon Sep 17 00:00:00 2001
From: "Alexander E. Patrakov" <patrakov@gmail.com>
Date: Mon, 15 Sep 2014 20:17:47 +0600
Subject: [PATCH] pcm, rate: 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.

Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com>
---

As suggested by Takashi Iwai.

 src/pcm/pcm_rate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 736d558..c76db25 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1069,7 +1069,10 @@  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) /* can't happen on healthy drivers */
+		return -EBADFD;
+
+	if (avail == 0) {
 		/* postpone the trigger since we have no data committed yet */
 		rate->start_pending = 1;
 		return 0;
-- 
2.1.0