[alsa-devel] Bad PCM stream after a suspend/resume cycle
Takashi Iwai
tiwai at suse.de
Wed Jan 17 07:05:47 PST 2018
On Wed, 17 Jan 2018 15:08:27 +0100,
Mirza Krak wrote:
>
> On 12 January 2018 at 19:04, Takashi Iwai <tiwai at suse.de> wrote:
> > On Fri, 12 Jan 2018 14:36:45 +0100,
> > Mirza Krak wrote:
> >>
>
> < snip >
>
> >
> > The EBADFD is already indicating a fatal error, and something is wrong
> > in the driver. Basically the driver should suspend the stream via
> > snd_pcm_suspend*() call in the PM resume ops. Most likely your driver
> > misses that.
> >
> > That said, it's specific to your using driver, and you'd need to take
> > a look at that code there.
>
> Thank you for you patience with me.
>
> I have looked further in to my hardware drivers and I can not really
> see any faults here.
>
> But I did some further testing and applying the following diff on
> aplay "resolves" the problem (v1.1.4 of alsa-utils):
>
> diff --git a/aplay/aplay.c b/aplay/aplay.c
> index f793c82..040cec1 100644
> --- a/aplay/aplay.c
> +++ b/aplay/aplay.c
> @@ -2020,6 +2020,8 @@ static ssize_t pcm_write(u_char *data, size_t count)
> if (test_position)
> do_test_position();
> if (r == -EAGAIN || (r >= 0 && (size_t)r < count)) {
> + if (snd_pcm_state(handle) == SND_PCM_STATE_SUSPENDED)
> + suspend();
> if (!test_nowait)
> snd_pcm_wait(handle, 100);
> } else if (r == -EPIPE) {
>
> Which means that there is no error in regard to suspending the stream
> as it properly reports this when checked.
>
> My problem is that this condition:
>
> (r >= 0 && (size_t)r < count)
>
> Is always true after a suspend/resume cycle. Which normally does not
> result in a "resume" call from aplay nor from snd_pcm_recover, which
> means that it will try to write data to a suspended stream which
> results in -EBADFD due to this (from alsa-lib):
>
> snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer,
> snd_pcm_uframes_t size)
> {
> < snip >
>
> if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
> return -EBADFD;
> return _snd_pcm_writei(pcm, buffer, size);
> }
>
> Because SUSPENDED is not part of the P_STATE_RUNNABLE, which maybe it should be?
OK, that's a bug in alsa-lib, then. The sanity check is good, but it
returns the inconsistent error code that doesn't match with the PCM
state.
Could you try the patch below?
thanks,
Takashi
-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] pcm: Return the consistent error code for unexpected PCM
states
Some PCM functions have the sanity check of the expected PCM states,
and most of them return -EBADFD if the current state doesn't match.
This is bad for some programs like aplay that expect the function
returning a proper code corresponding to the state, e.g. -ESTRPIPE for
the suspend.
This patch is an attempt to address such inconsistencies. The sanity
checker bad_pcm_state() now returns the error code instead of bool, so
that the caller can pass the returned code as is. And it calls a new
helper, pcm_state_to_error(), for obtaining the error code to certain
known PCM error state.
While we're at it, use the new pcm_state_to_error() for simplifying
the existing code to retrieve the error code, too.
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
src/pcm/pcm.c | 170 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 98 insertions(+), 72 deletions(-)
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index e9ebf383c31b..69d7d66500ea 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -657,6 +657,21 @@ playback devices.
#include "pcm_local.h"
#ifndef DOC_HIDDEN
+/* return specific error codes for known bad PCM states */
+static int pcm_state_to_error(snd_pcm_state_t state)
+{
+ switch (state) {
+ case SND_PCM_STATE_XRUN:
+ return -EPIPE;
+ case SND_PCM_STATE_SUSPENDED:
+ return -ESTRPIPE;
+ case SND_PCM_STATE_DISCONNECTED:
+ return -ENODEV;
+ default:
+ return 0;
+ }
+}
+
#define P_STATE(x) (1U << SND_PCM_STATE_ ## x)
#define P_STATE_RUNNABLE (P_STATE(PREPARED) | \
P_STATE(RUNNING) | \
@@ -667,9 +682,18 @@ playback devices.
/* check whether the PCM is in the unexpected state */
static int bad_pcm_state(snd_pcm_t *pcm, unsigned int supported_states)
{
+ snd_pcm_state_t state;
+ int err;
+
if (pcm->own_state_check)
return 0; /* don't care, the plugin checks by itself */
- return !(supported_states & (1U << snd_pcm_state(pcm)));
+ state = snd_pcm_state(pcm);
+ if (supported_states & (1U << state))
+ return 0; /* OK */
+ err = pcm_state_to_error(state);
+ if (err < 0)
+ return err;
+ return -EBADFD;
}
#endif
@@ -1143,8 +1167,9 @@ int snd_pcm_prepare(snd_pcm_t *pcm)
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, ~P_STATE(DISCONNECTED)))
- return -EBADFD;
+ err = bad_pcm_state(pcm, ~P_STATE(DISCONNECTED));
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
err = pcm->fast_ops->prepare(pcm->fast_op_arg);
snd_pcm_unlock(pcm);
@@ -1191,8 +1216,9 @@ int snd_pcm_start(snd_pcm_t *pcm)
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE(PREPARED)))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE(PREPARED));
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
err = __snd_pcm_start(pcm);
snd_pcm_unlock(pcm);
@@ -1221,9 +1247,10 @@ int snd_pcm_drop(snd_pcm_t *pcm)
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE | P_STATE(SETUP) |
- P_STATE(SUSPENDED)))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE | P_STATE(SETUP) |
+ P_STATE(SUSPENDED));
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
err = pcm->fast_ops->drop(pcm->fast_op_arg);
snd_pcm_unlock(pcm);
@@ -1247,13 +1274,16 @@ int snd_pcm_drop(snd_pcm_t *pcm)
*/
int snd_pcm_drain(snd_pcm_t *pcm)
{
+ int err;
+
assert(pcm);
if (CHECK_SANITY(! pcm->setup)) {
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
/* lock handled in the callback */
return pcm->fast_ops->drain(pcm->fast_op_arg);
}
@@ -1279,8 +1309,9 @@ int snd_pcm_pause(snd_pcm_t *pcm, int enable)
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
err = pcm->fast_ops->pause(pcm->fast_op_arg, enable);
snd_pcm_unlock(pcm);
@@ -1301,14 +1332,16 @@ int snd_pcm_pause(snd_pcm_t *pcm, int enable)
snd_pcm_sframes_t snd_pcm_rewindable(snd_pcm_t *pcm)
{
snd_pcm_sframes_t result;
+ int err;
assert(pcm);
if (CHECK_SANITY(! pcm->setup)) {
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
result = pcm->fast_ops->rewindable(pcm->fast_op_arg);
snd_pcm_unlock(pcm);
@@ -1327,6 +1360,7 @@ snd_pcm_sframes_t snd_pcm_rewindable(snd_pcm_t *pcm)
snd_pcm_sframes_t snd_pcm_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
{
snd_pcm_sframes_t result;
+ int err;
assert(pcm);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1335,8 +1369,9 @@ snd_pcm_sframes_t snd_pcm_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
}
if (frames == 0)
return 0;
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
result = pcm->fast_ops->rewind(pcm->fast_op_arg, frames);
snd_pcm_unlock(pcm);
@@ -1357,14 +1392,16 @@ snd_pcm_sframes_t snd_pcm_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
snd_pcm_sframes_t snd_pcm_forwardable(snd_pcm_t *pcm)
{
snd_pcm_sframes_t result;
+ int err;
assert(pcm);
if (CHECK_SANITY(! pcm->setup)) {
SNDMSG("PCM not set up");
return -EIO;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
result = pcm->fast_ops->forwardable(pcm->fast_op_arg);
snd_pcm_unlock(pcm);
@@ -1387,6 +1424,7 @@ snd_pcm_sframes_t snd_pcm_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
#endif
{
snd_pcm_sframes_t result;
+ int err;
assert(pcm);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1395,8 +1433,9 @@ snd_pcm_sframes_t snd_pcm_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
}
if (frames == 0)
return 0;
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
result = pcm->fast_ops->forward(pcm->fast_op_arg, frames);
snd_pcm_unlock(pcm);
@@ -1425,6 +1464,8 @@ use_default_symbol_version(__snd_pcm_forward, snd_pcm_forward, ALSA_0.9.0rc8);
*/
snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_uframes_t size)
{
+ int err;
+
assert(pcm);
assert(size == 0 || buffer);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1435,8 +1476,9 @@ snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_ufr
SNDMSG("invalid access type %s", snd_pcm_access_name(pcm->access));
return -EINVAL;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
return _snd_pcm_writei(pcm, buffer, size);
}
@@ -1461,6 +1503,8 @@ snd_pcm_sframes_t snd_pcm_writei(snd_pcm_t *pcm, const void *buffer, snd_pcm_ufr
*/
snd_pcm_sframes_t snd_pcm_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
{
+ int err;
+
assert(pcm);
assert(size == 0 || bufs);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1471,8 +1515,9 @@ snd_pcm_sframes_t snd_pcm_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t
SNDMSG("invalid access type %s", snd_pcm_access_name(pcm->access));
return -EINVAL;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
return _snd_pcm_writen(pcm, bufs, size);
}
@@ -1497,6 +1542,8 @@ snd_pcm_sframes_t snd_pcm_writen(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t
*/
snd_pcm_sframes_t snd_pcm_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t size)
{
+ int err;
+
assert(pcm);
assert(size == 0 || buffer);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1507,8 +1554,9 @@ snd_pcm_sframes_t snd_pcm_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t
SNDMSG("invalid access type %s", snd_pcm_access_name(pcm->access));
return -EINVAL;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
return _snd_pcm_readi(pcm, buffer, size);
}
@@ -1533,6 +1581,8 @@ snd_pcm_sframes_t snd_pcm_readi(snd_pcm_t *pcm, void *buffer, snd_pcm_uframes_t
*/
snd_pcm_sframes_t snd_pcm_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t size)
{
+ int err;
+
assert(pcm);
assert(size == 0 || bufs);
if (CHECK_SANITY(! pcm->setup)) {
@@ -1543,8 +1593,9 @@ snd_pcm_sframes_t snd_pcm_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_uframes_t s
SNDMSG("invalid access type %s", snd_pcm_access_name(pcm->access));
return -EINVAL;
}
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
return _snd_pcm_readn(pcm, bufs, size);
}
@@ -2695,18 +2746,12 @@ int snd_pcm_wait(snd_pcm_t *pcm, int timeout)
/* locked version */
int __snd_pcm_wait_in_lock(snd_pcm_t *pcm, int timeout)
{
+ int err;
+
if (!snd_pcm_may_wait_for_avail_min(pcm, snd_pcm_mmap_avail(pcm))) {
/* check more precisely */
- switch (__snd_pcm_state(pcm)) {
- case SND_PCM_STATE_XRUN:
- return -EPIPE;
- case SND_PCM_STATE_SUSPENDED:
- return -ESTRPIPE;
- case SND_PCM_STATE_DISCONNECTED:
- return -ENODEV;
- default:
- return 1;
- }
+ err = pcm_state_to_error(__snd_pcm_state(pcm));
+ return err < 0 ? err : 1;
}
return snd_pcm_wait_nocheck(pcm, timeout);
}
@@ -2753,16 +2798,8 @@ int snd_pcm_wait_nocheck(snd_pcm_t *pcm, int timeout)
return err;
if (revents & (POLLERR | POLLNVAL)) {
/* check more precisely */
- switch (__snd_pcm_state(pcm)) {
- case SND_PCM_STATE_XRUN:
- return -EPIPE;
- case SND_PCM_STATE_SUSPENDED:
- return -ESTRPIPE;
- case SND_PCM_STATE_DISCONNECTED:
- return -ENODEV;
- default:
- return -EIO;
- }
+ err = pcm_state_to_error(__snd_pcm_state(pcm));
+ return err < 0 ? err : -EIO;
}
} while (!(revents & (POLLIN | POLLOUT)));
#if 0 /* very useful code to test poll related problems */
@@ -7010,8 +7047,9 @@ int snd_pcm_mmap_begin(snd_pcm_t *pcm,
{
int err;
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
err = __snd_pcm_mmap_begin(pcm, areas, offset, frames);
snd_pcm_unlock(pcm);
@@ -7106,9 +7144,11 @@ snd_pcm_sframes_t snd_pcm_mmap_commit(snd_pcm_t *pcm,
snd_pcm_uframes_t frames)
{
snd_pcm_sframes_t result;
+ int err;
- if (bad_pcm_state(pcm, P_STATE_RUNNABLE))
- return -EBADFD;
+ err = bad_pcm_state(pcm, P_STATE_RUNNABLE);
+ if (err < 0)
+ return err;
snd_pcm_lock(pcm);
result = __snd_pcm_mmap_commit(pcm, offset, frames);
snd_pcm_unlock(pcm);
@@ -7204,17 +7244,10 @@ snd_pcm_sframes_t snd_pcm_read_areas(snd_pcm_t *pcm, const snd_pcm_channel_area_
case SND_PCM_STATE_DRAINING:
case SND_PCM_STATE_PAUSED:
break;
- case SND_PCM_STATE_XRUN:
- err = -EPIPE;
- goto _end;
- case SND_PCM_STATE_SUSPENDED:
- err = -ESTRPIPE;
- goto _end;
- case SND_PCM_STATE_DISCONNECTED:
- err = -ENODEV;
- goto _end;
default:
- err = -EBADFD;
+ err = pcm_state_to_error(state);
+ if (!err)
+ err = -EBADFD;
goto _end;
}
avail = __snd_pcm_avail_update(pcm);
@@ -7280,17 +7313,10 @@ snd_pcm_sframes_t snd_pcm_write_areas(snd_pcm_t *pcm, const snd_pcm_channel_area
if (err < 0)
goto _end;
break;
- case SND_PCM_STATE_XRUN:
- err = -EPIPE;
- goto _end;
- case SND_PCM_STATE_SUSPENDED:
- err = -ESTRPIPE;
- goto _end;
- case SND_PCM_STATE_DISCONNECTED:
- err = -ENODEV;
- goto _end;
default:
- err = -EBADFD;
+ err = pcm_state_to_error(state);
+ if (!err)
+ err = -EBADFD;
goto _end;
}
avail = __snd_pcm_avail_update(pcm);
--
2.15.1
More information about the Linux-rockchip
mailing list