[alsa-devel] [PATCH v2 1/4] ALSA: compress: Fix regression on compressed capture streams
A previous fix to the stop handling on compressed capture streams causes some knock on issues. The previous fix updated snd_compr_drain_notify to set the state back to PREPARED for capture streams. This causes some issues however as the handling for snd_compr_poll differs between the two states and some user-space applications were relying on the poll failing after the stream had been stopped.
To correct this regression whilst still fixing the original problem the patch was addressing, update the capture handling to skip the PREPARED state rather than skipping the SETUP state as it has done until now.
Fixes: 4f2ab5e1d13d ("ALSA: compress: Fix stop handling on compressed capture streams") Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com ---
No changes since v1.
Thanks, Charles
include/sound/compress_driver.h | 5 +---- sound/core/compress_offload.c | 16 +++++++++++----- 2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index c5188ff724d12..bc88d6f964da9 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -173,10 +173,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream) if (snd_BUG_ON(!stream)) return;
- if (stream->direction == SND_COMPRESS_PLAYBACK) - stream->runtime->state = SNDRV_PCM_STATE_SETUP; - else - stream->runtime->state = SNDRV_PCM_STATE_PREPARED; + stream->runtime->state = SNDRV_PCM_STATE_SETUP;
wake_up(&stream->runtime->sleep); } diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 99b8821587053..d79aee6b9edd2 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -574,10 +574,7 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) stream->metadata_set = false; stream->next_track = false;
- if (stream->direction == SND_COMPRESS_PLAYBACK) - stream->runtime->state = SNDRV_PCM_STATE_SETUP; - else - stream->runtime->state = SNDRV_PCM_STATE_PREPARED; + stream->runtime->state = SNDRV_PCM_STATE_SETUP; } else { return -EPERM; } @@ -693,8 +690,17 @@ static int snd_compr_start(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED) + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_SETUP: + if (stream->direction != SND_COMPRESS_CAPTURE) + return -EPERM; + break; + case SNDRV_PCM_STATE_PREPARED: + break; + default: return -EPERM; + } + retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_START); if (!retval) stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
Currently, whilst in SNDRV_PCM_STATE_OPEN it is possible to call snd_compr_stop, snd_compr_drain and snd_compr_partial_drain, which allow a transition to SNDRV_PCM_STATE_SETUP. The stream should only be able to move to the setup state once it has received a SNDRV_COMPRESS_SET_PARAMS ioctl. Fix this issue by not allowing those ioctls whilst in the open state.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com ---
No changes since v1.
Thanks, Charles
sound/core/compress_offload.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index d79aee6b9edd2..40dae723c59db 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -711,9 +711,15 @@ static int snd_compr_stop(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || - stream->runtime->state == SNDRV_PCM_STATE_SETUP) + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_OPEN: + case SNDRV_PCM_STATE_SETUP: + case SNDRV_PCM_STATE_PREPARED: return -EPERM; + default: + break; + } + retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP); if (!retval) { snd_compr_drain_notify(stream); @@ -801,9 +807,14 @@ static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || - stream->runtime->state == SNDRV_PCM_STATE_SETUP) + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_OPEN: + case SNDRV_PCM_STATE_SETUP: + case SNDRV_PCM_STATE_PREPARED: return -EPERM; + default: + break; + }
retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN); if (retval) { @@ -840,9 +851,16 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) static int snd_compr_partial_drain(struct snd_compr_stream *stream) { int retval; - if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED || - stream->runtime->state == SNDRV_PCM_STATE_SETUP) + + switch (stream->runtime->state) { + case SNDRV_PCM_STATE_OPEN: + case SNDRV_PCM_STATE_SETUP: + case SNDRV_PCM_STATE_PREPARED: return -EPERM; + default: + break; + } + /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
On Mon, 22 Jul 2019 11:24:34 +0200, Charles Keepax wrote:
Currently, whilst in SNDRV_PCM_STATE_OPEN it is possible to call snd_compr_stop, snd_compr_drain and snd_compr_partial_drain, which allow a transition to SNDRV_PCM_STATE_SETUP. The stream should only be able to move to the setup state once it has received a SNDRV_COMPRESS_SET_PARAMS ioctl. Fix this issue by not allowing those ioctls whilst in the open state.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Applied, thanks.
Takashi
No changes since v1.
Thanks, Charles
sound/core/compress_offload.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index d79aee6b9edd2..40dae723c59db 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -711,9 +711,15 @@ static int snd_compr_stop(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)
- switch (stream->runtime->state) {
- case SNDRV_PCM_STATE_OPEN:
- case SNDRV_PCM_STATE_SETUP:
- case SNDRV_PCM_STATE_PREPARED: return -EPERM;
- default:
break;- }
- retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP); if (!retval) { snd_compr_drain_notify(stream);
@@ -801,9 +807,14 @@ static int snd_compr_drain(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)
switch (stream->runtime->state) {
case SNDRV_PCM_STATE_OPEN:
case SNDRV_PCM_STATE_SETUP:
case SNDRV_PCM_STATE_PREPARED: return -EPERM;
default:
break;}
retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN); if (retval) {
@@ -840,9 +851,16 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) static int snd_compr_partial_drain(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
stream->runtime->state == SNDRV_PCM_STATE_SETUP)
- switch (stream->runtime->state) {
- case SNDRV_PCM_STATE_OPEN:
- case SNDRV_PCM_STATE_SETUP:
- case SNDRV_PCM_STATE_PREPARED: return -EPERM;
- default:
break;- }
- /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
-- 2.11.0
Partial drain and next track are intended for gapless playback and don't really have an obvious interpretation for a capture stream, so makes sense to not allow those operations on capture streams.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com ---
Changes since v1: - Continue to allow drain on capture streams.
Thanks, Charles
sound/core/compress_offload.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 40dae723c59db..6cf5b8440cf30 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -834,6 +834,10 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) return -EPERM;
+ /* next track doesn't have any meaning for capture streams */ + if (stream->direction == SND_COMPRESS_CAPTURE) + return -EPERM; + /* you can signal next track if this is intended to be a gapless stream * and current track metadata is set */ @@ -861,6 +865,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) break; }
+ /* partial drain doesn't have any meaning for capture streams */ + if (stream->direction == SND_COMPRESS_CAPTURE) + return -EPERM; + /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
On Mon, 22 Jul 2019 11:24:35 +0200, Charles Keepax wrote:
Partial drain and next track are intended for gapless playback and don't really have an obvious interpretation for a capture stream, so makes sense to not allow those operations on capture streams.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Applied, thanks.
Takashi
Changes since v1:
- Continue to allow drain on capture streams.
Thanks, Charles
sound/core/compress_offload.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 40dae723c59db..6cf5b8440cf30 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -834,6 +834,10 @@ static int snd_compr_next_track(struct snd_compr_stream *stream) if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) return -EPERM;
- /* next track doesn't have any meaning for capture streams */
- if (stream->direction == SND_COMPRESS_CAPTURE)
return -EPERM;- /* you can signal next track if this is intended to be a gapless stream
*/
- and current track metadata is set
@@ -861,6 +865,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) break; }
- /* partial drain doesn't have any meaning for capture streams */
- if (stream->direction == SND_COMPRESS_CAPTURE)
return -EPERM;- /* stream can be drained only when next track has been signalled */ if (stream->next_track == false) return -EPERM;
-- 2.11.0
Draining makes little sense in the situation of hardware overrun, as the hardware will have consumed all its available samples. Additionally, draining whilst the stream is paused would presumably get stuck as no data is being consumed on the DSP side.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com ---
No changes since v1.
Thanks, Charles
sound/core/compress_offload.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 6cf5b8440cf30..41905afada63f 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -811,7 +811,10 @@ static int snd_compr_drain(struct snd_compr_stream *stream) case SNDRV_PCM_STATE_OPEN: case SNDRV_PCM_STATE_SETUP: case SNDRV_PCM_STATE_PREPARED: + case SNDRV_PCM_STATE_PAUSED: return -EPERM; + case SNDRV_PCM_STATE_XRUN: + return -EPIPE; default: break; } @@ -860,7 +863,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) case SNDRV_PCM_STATE_OPEN: case SNDRV_PCM_STATE_SETUP: case SNDRV_PCM_STATE_PREPARED: + case SNDRV_PCM_STATE_PAUSED: return -EPERM; + case SNDRV_PCM_STATE_XRUN: + return -EPIPE; default: break; }
On Mon, 22 Jul 2019 11:24:36 +0200, Charles Keepax wrote:
Draining makes little sense in the situation of hardware overrun, as the hardware will have consumed all its available samples. Additionally, draining whilst the stream is paused would presumably get stuck as no data is being consumed on the DSP side.
Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Applied, thanks.
Takashi
No changes since v1.
Thanks, Charles
sound/core/compress_offload.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 6cf5b8440cf30..41905afada63f 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -811,7 +811,10 @@ static int snd_compr_drain(struct snd_compr_stream *stream) case SNDRV_PCM_STATE_OPEN: case SNDRV_PCM_STATE_SETUP: case SNDRV_PCM_STATE_PREPARED:
- case SNDRV_PCM_STATE_PAUSED: return -EPERM;
- case SNDRV_PCM_STATE_XRUN:
return -EPIPE;@@ -860,7 +863,10 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream) case SNDRV_PCM_STATE_OPEN: case SNDRV_PCM_STATE_SETUP: case SNDRV_PCM_STATE_PREPARED:
- case SNDRV_PCM_STATE_PAUSED: return -EPERM;
- case SNDRV_PCM_STATE_XRUN:
return -EPIPE;-- 2.11.0
On 22-07-19, 10:24, Charles Keepax wrote:
A previous fix to the stop handling on compressed capture streams causes some knock on issues. The previous fix updated snd_compr_drain_notify to set the state back to PREPARED for capture streams. This causes some issues however as the handling for snd_compr_poll differs between the two states and some user-space applications were relying on the poll failing after the stream had been stopped.
To correct this regression whilst still fixing the original problem the patch was addressing, update the capture handling to skip the PREPARED state rather than skipping the SETUP state as it has done until now.
For the whole series:
Acked-by: Vinod Koul vkoul@kernel.org
On Mon, 22 Jul 2019 11:24:33 +0200, Charles Keepax wrote:
A previous fix to the stop handling on compressed capture streams causes some knock on issues. The previous fix updated snd_compr_drain_notify to set the state back to PREPARED for capture streams. This causes some issues however as the handling for snd_compr_poll differs between the two states and some user-space applications were relying on the poll failing after the stream had been stopped.
To correct this regression whilst still fixing the original problem the patch was addressing, update the capture handling to skip the PREPARED state rather than skipping the SETUP state as it has done until now.
Fixes: 4f2ab5e1d13d ("ALSA: compress: Fix stop handling on compressed capture streams") Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com
Applied, thanks.
Takashi
No changes since v1.
Thanks, Charles
include/sound/compress_driver.h | 5 +---- sound/core/compress_offload.c | 16 +++++++++++----- 2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index c5188ff724d12..bc88d6f964da9 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -173,10 +173,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream) if (snd_BUG_ON(!stream)) return;
- if (stream->direction == SND_COMPRESS_PLAYBACK)
stream->runtime->state = SNDRV_PCM_STATE_SETUP;- else
stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
stream->runtime->state = SNDRV_PCM_STATE_SETUP;
wake_up(&stream->runtime->sleep);
} diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 99b8821587053..d79aee6b9edd2 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -574,10 +574,7 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) stream->metadata_set = false; stream->next_track = false;
if (stream->direction == SND_COMPRESS_PLAYBACK)stream->runtime->state = SNDRV_PCM_STATE_SETUP;elsestream->runtime->state = SNDRV_PCM_STATE_PREPARED;
stream->runtime->state = SNDRV_PCM_STATE_SETUP;@@ -693,8 +690,17 @@ static int snd_compr_start(struct snd_compr_stream *stream) { int retval;
- if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED)
- switch (stream->runtime->state) {
- case SNDRV_PCM_STATE_SETUP:
if (stream->direction != SND_COMPRESS_CAPTURE)return -EPERM;break;- case SNDRV_PCM_STATE_PREPARED:
break;- default: return -EPERM;
- }
- retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_START); if (!retval) stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
-- 2.11.0
participants (3)
-
Charles Keepax -
Takashi Iwai -
Vinod Koul