From patchwork Wed Jan 17 15:05:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 10169437 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 45901601D3 for ; Wed, 17 Jan 2018 15:06:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 335A52623D for ; Wed, 17 Jan 2018 15:06:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 27945283A5; Wed, 17 Jan 2018 15:06:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 16B70283B0 for ; Wed, 17 Jan 2018 15:06:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+hsDClLgspreQgdmiJQ1WYTwiS6NTsZOGCB+9OZPLE8=; b=IUNeMYBmNC/HGI FfoABtrs5KLQuwMSKDxgoVKRieZrHftnBbD1QxOcIbej0ePj1K6oXAq6qX+VuuJ8wKyqzrIFALewa od6YUx8AEwcPiexL7i5Ukauq23I47ijPNPCudjjX8AaZWohhRqlnGQRWNNkfx+qOZCQJhqaWpd5lU 1He2ucNlGe2JuF+3LK2za0/TuKWdII3q5XZbWl8Vp1PHCkRELxpt1cWbAXpYHvT7L5NSLntpTqCE2 CSVrFJ/oCbPKn4hlkJsbndhzbM7E5/NMfHcf+MPxhHk9MqimArHOXIo3B9zrGDEIxV4nhOcJzyicq BVYP8TGRuwI32csPufYA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1ebpHy-00022F-Tl; Wed, 17 Jan 2018 15:06:06 +0000 Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.89 #1 (Red Hat Linux)) id 1ebpHu-00021D-Qq for linux-rockchip@lists.infradead.org; Wed, 17 Jan 2018 15:06:05 +0000 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 713B5AD28; Wed, 17 Jan 2018 15:05:48 +0000 (UTC) Date: Wed, 17 Jan 2018 16:05:47 +0100 Message-ID: From: Takashi Iwai To: Mirza Krak Subject: Re: [alsa-devel] Bad PCM stream after a suspend/resume cycle In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org, Heiko =?UTF-8?B?U3TDvGJuZXI=?= Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 17 Jan 2018 15:08:27 +0100, Mirza Krak wrote: > > On 12 January 2018 at 19:04, Takashi Iwai 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 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 Tested-by: Mirza Krak --- 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);