* [PATCH 0/3] ALSA: a few more USB-audio fixes
@ 2021-02-06 20:30 Takashi Iwai
2021-02-06 20:30 ` [PATCH 1/3] ALSA: usb-audio: Handle invalid running state at releasing EP Takashi Iwai
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Takashi Iwai @ 2021-02-06 20:30 UTC (permalink / raw
To: alsa-devel
Hi,
here is a small patch set for USB-audio. The goal of this patch set
is to cover a small race window at the endpoint state transition, and
the preparation for the upcoming patch set for PCM sync_stop fixes.
Takashi
===
Takashi Iwai (3):
ALSA: usb-audio: Handle invalid running state at releasing EP
ALSA: usb-audio: More strict state change in EP
ALSA: usb-audio: Don't avoid stopping the stream at disconnection
sound/usb/card.h | 2 +-
sound/usb/endpoint.c | 85 +++++++++++++++++++++++++-------------------
sound/usb/pcm.c | 5 +--
3 files changed, 51 insertions(+), 41 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/3] ALSA: usb-audio: Handle invalid running state at releasing EP
2021-02-06 20:30 [PATCH 0/3] ALSA: a few more USB-audio fixes Takashi Iwai
@ 2021-02-06 20:30 ` Takashi Iwai
2021-02-06 20:30 ` [PATCH 2/3] ALSA: usb-audio: More strict state change in EP Takashi Iwai
2021-02-06 20:30 ` [PATCH 3/3] ALSA: usb-audio: Don't avoid stopping the stream at disconnection Takashi Iwai
2 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2021-02-06 20:30 UTC (permalink / raw
To: alsa-devel
When we stop an endpoint in release_urbs(), it ignores the
inconsistent endpoint state and tries to release the resources.
This shouldn't happen in theory, but it's still safer to abort the
release and let the caller proper error handling.
Also, stop_and_unlink_urbs() called from release_urbs() does two step
works, and it's more straightforward to split this to two functions
again, so that the call from the PCM trigger won't take the path with
sleeping.
This patch modifies the EP management code to adapt two points above.
Fixes: d0f09d1e4a88 ("ALSA: usb-audio: Refactoring endpoint URB deactivation")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/usb/endpoint.c | 40 +++++++++++++++++++++-------------------
1 file changed, 21 insertions(+), 19 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 4d1c678a0d80..e102c024c21f 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -868,24 +868,22 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep)
}
/*
- * Stop and unlink active urbs.
+ * Stop active urbs
*
- * This function checks and clears EP_FLAG_RUNNING state.
- * When @wait_sync is set, it waits until all pending URBs are killed.
+ * This function moves the EP to STOPPING state if it's being RUNNING.
*/
-static int stop_and_unlink_urbs(struct snd_usb_endpoint *ep, bool force,
- bool wait_sync)
+static int stop_urbs(struct snd_usb_endpoint *ep, bool force)
{
unsigned int i;
if (!force && atomic_read(&ep->chip->shutdown)) /* to be sure... */
return -EBADFD;
- if (atomic_read(&ep->running))
+ if (!force && atomic_read(&ep->running))
return -EBUSY;
if (!test_and_clear_bit(EP_FLAG_RUNNING, &ep->flags))
- goto out;
+ return 0;
set_bit(EP_FLAG_STOPPING, &ep->flags);
INIT_LIST_HEAD(&ep->ready_playback_urbs);
@@ -901,24 +899,25 @@ static int stop_and_unlink_urbs(struct snd_usb_endpoint *ep, bool force,
}
}
- out:
- if (wait_sync)
- return wait_clear_urbs(ep);
return 0;
}
/*
* release an endpoint's urbs
*/
-static void release_urbs(struct snd_usb_endpoint *ep, int force)
+static int release_urbs(struct snd_usb_endpoint *ep, bool force)
{
- int i;
+ int i, err;
/* route incoming urbs to nirvana */
snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
- /* stop urbs */
- stop_and_unlink_urbs(ep, force, true);
+ /* stop and unlink urbs */
+ err = stop_urbs(ep, force);
+ if (err)
+ return err;
+
+ wait_clear_urbs(ep);
for (i = 0; i < ep->nurbs; i++)
release_urb_ctx(&ep->urb[i]);
@@ -928,6 +927,7 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force)
ep->syncbuf = NULL;
ep->nurbs = 0;
+ return 0;
}
/*
@@ -1118,7 +1118,7 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep)
return 0;
out_of_memory:
- release_urbs(ep, 0);
+ release_urbs(ep, false);
return -ENOMEM;
}
@@ -1162,7 +1162,7 @@ static int sync_ep_set_params(struct snd_usb_endpoint *ep)
return 0;
out_of_memory:
- release_urbs(ep, 0);
+ release_urbs(ep, false);
return -ENOMEM;
}
@@ -1180,7 +1180,9 @@ static int snd_usb_endpoint_set_params(struct snd_usb_audio *chip,
int err;
/* release old buffers, if any */
- release_urbs(ep, 0);
+ err = release_urbs(ep, false);
+ if (err < 0)
+ return err;
ep->datainterval = fmt->datainterval;
ep->maxpacksize = fmt->maxpacksize;
@@ -1433,7 +1435,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep)
WRITE_ONCE(ep->sync_source->sync_sink, NULL);
if (!atomic_dec_return(&ep->running))
- stop_and_unlink_urbs(ep, false, false);
+ stop_urbs(ep, false);
}
/**
@@ -1446,7 +1448,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep)
*/
void snd_usb_endpoint_release(struct snd_usb_endpoint *ep)
{
- release_urbs(ep, 1);
+ release_urbs(ep, true);
}
/**
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/3] ALSA: usb-audio: More strict state change in EP
2021-02-06 20:30 [PATCH 0/3] ALSA: a few more USB-audio fixes Takashi Iwai
2021-02-06 20:30 ` [PATCH 1/3] ALSA: usb-audio: Handle invalid running state at releasing EP Takashi Iwai
@ 2021-02-06 20:30 ` Takashi Iwai
2021-02-06 20:30 ` [PATCH 3/3] ALSA: usb-audio: Don't avoid stopping the stream at disconnection Takashi Iwai
2 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2021-02-06 20:30 UTC (permalink / raw
To: alsa-devel
The endpoint management has bit flags to indicate the current state,
and we're dealing two things: the running bit and the stopping bit.
There is a thin window in transition from the running to the stopping
in stop_urbs(), and as long as the bit flags are used, it's difficult
to plug.
This patch modifies the state management code to use the atomic int
and follow the explicit three states, STOPPED, RUNNING and STOPPING.
The state change is done via atomic_cmpxhg() for avoiding possible
races, and check the state change more strictly. The unexpected state
change is now handled as an error.
Fixes: d0f09d1e4a88 ("ALSA: usb-audio: Refactoring endpoint URB deactivation")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/usb/card.h | 2 +-
sound/usb/endpoint.c | 42 ++++++++++++++++++++++++++++--------------
2 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 37091b117614..a741e7da83a2 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -71,7 +71,7 @@ struct snd_usb_endpoint {
unsigned char altsetting; /* corresponding alternate setting */
unsigned char ep_idx; /* endpoint array index */
- unsigned long flags; /* running bit flags */
+ atomic_t state; /* running state */
void (*prepare_data_urb) (struct snd_usb_substream *subs,
struct urb *urb);
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index e102c024c21f..4390075b2c6f 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -21,8 +21,11 @@
#include "clock.h"
#include "quirks.h"
-#define EP_FLAG_RUNNING 1
-#define EP_FLAG_STOPPING 2
+enum {
+ EP_STATE_STOPPED,
+ EP_STATE_RUNNING,
+ EP_STATE_STOPPING,
+};
/* interface refcounting */
struct snd_usb_iface_ref {
@@ -115,6 +118,16 @@ static const char *usb_error_string(int err)
}
}
+static inline bool ep_state_running(struct snd_usb_endpoint *ep)
+{
+ return atomic_read(&ep->state) == EP_STATE_RUNNING;
+}
+
+static inline bool ep_state_update(struct snd_usb_endpoint *ep, int old, int new)
+{
+ return atomic_cmpxchg(&ep->state, old, new) == old;
+}
+
/**
* snd_usb_endpoint_implicit_feedback_sink: Report endpoint usage type
*
@@ -393,7 +406,7 @@ next_packet_fifo_dequeue(struct snd_usb_endpoint *ep)
*/
static void queue_pending_output_urbs(struct snd_usb_endpoint *ep)
{
- while (test_bit(EP_FLAG_RUNNING, &ep->flags)) {
+ while (ep_state_running(ep)) {
unsigned long flags;
struct snd_usb_packet_info *packet;
@@ -454,13 +467,13 @@ static void snd_complete_urb(struct urb *urb)
if (unlikely(atomic_read(&ep->chip->shutdown)))
goto exit_clear;
- if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ if (unlikely(!ep_state_running(ep)))
goto exit_clear;
if (usb_pipeout(ep->pipe)) {
retire_outbound_urb(ep, ctx);
/* can be stopped during retire callback */
- if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ if (unlikely(!ep_state_running(ep)))
goto exit_clear;
if (snd_usb_endpoint_implicit_feedback_sink(ep)) {
@@ -474,12 +487,12 @@ static void snd_complete_urb(struct urb *urb)
prepare_outbound_urb(ep, ctx);
/* can be stopped during prepare callback */
- if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ if (unlikely(!ep_state_running(ep)))
goto exit_clear;
} else {
retire_inbound_urb(ep, ctx);
/* can be stopped during retire callback */
- if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags)))
+ if (unlikely(!ep_state_running(ep)))
goto exit_clear;
prepare_inbound_urb(ep, ctx);
@@ -835,7 +848,7 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep)
unsigned long end_time = jiffies + msecs_to_jiffies(1000);
int alive;
- if (!test_bit(EP_FLAG_STOPPING, &ep->flags))
+ if (atomic_read(&ep->state) != EP_STATE_STOPPING)
return 0;
do {
@@ -850,10 +863,11 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep)
usb_audio_err(ep->chip,
"timeout: still %d active urbs on EP #%x\n",
alive, ep->ep_num);
- clear_bit(EP_FLAG_STOPPING, &ep->flags);
- ep->sync_sink = NULL;
- snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
+ if (ep_state_update(ep, EP_STATE_STOPPING, EP_STATE_STOPPED)) {
+ ep->sync_sink = NULL;
+ snd_usb_endpoint_set_callback(ep, NULL, NULL, NULL);
+ }
return 0;
}
@@ -882,10 +896,9 @@ static int stop_urbs(struct snd_usb_endpoint *ep, bool force)
if (!force && atomic_read(&ep->running))
return -EBUSY;
- if (!test_and_clear_bit(EP_FLAG_RUNNING, &ep->flags))
+ if (!ep_state_update(ep, EP_STATE_RUNNING, EP_STATE_STOPPING))
return 0;
- set_bit(EP_FLAG_STOPPING, &ep->flags);
INIT_LIST_HEAD(&ep->ready_playback_urbs);
ep->next_packet_head = 0;
ep->next_packet_queued = 0;
@@ -1362,7 +1375,8 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep)
* from that context.
*/
- set_bit(EP_FLAG_RUNNING, &ep->flags);
+ if (!ep_state_update(ep, EP_STATE_STOPPED, EP_STATE_RUNNING))
+ goto __error;
if (snd_usb_endpoint_implicit_feedback_sink(ep)) {
for (i = 0; i < ep->nurbs; i++) {
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 3/3] ALSA: usb-audio: Don't avoid stopping the stream at disconnection
2021-02-06 20:30 [PATCH 0/3] ALSA: a few more USB-audio fixes Takashi Iwai
2021-02-06 20:30 ` [PATCH 1/3] ALSA: usb-audio: Handle invalid running state at releasing EP Takashi Iwai
2021-02-06 20:30 ` [PATCH 2/3] ALSA: usb-audio: More strict state change in EP Takashi Iwai
@ 2021-02-06 20:30 ` Takashi Iwai
2 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2021-02-06 20:30 UTC (permalink / raw
To: alsa-devel
In the later patch, we're going to issue the PCM sync_stop calls at
disconnection. But currently the USB-audio driver can't handle it
because it has a check of shutdown flag for stopping the URBs. This
is basically superfluous (the stopping URBs are safe at disconnection
state), so let's drop the check.
Fixes: dc5eafe7787c ("ALSA: usb-audio: Support PCM sync_stop")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/usb/endpoint.c | 3 ---
sound/usb/pcm.c | 5 +----
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 4390075b2c6f..102d53515a76 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -890,9 +890,6 @@ static int stop_urbs(struct snd_usb_endpoint *ep, bool force)
{
unsigned int i;
- if (!force && atomic_read(&ep->chip->shutdown)) /* to be sure... */
- return -EBADFD;
-
if (!force && atomic_read(&ep->running))
return -EBUSY;
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index dcadf8f164b2..bf5a0f3c1fad 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -270,10 +270,7 @@ static int snd_usb_pcm_sync_stop(struct snd_pcm_substream *substream)
{
struct snd_usb_substream *subs = substream->runtime->private_data;
- if (!snd_usb_lock_shutdown(subs->stream->chip)) {
- sync_pending_stops(subs);
- snd_usb_unlock_shutdown(subs->stream->chip);
- }
+ sync_pending_stops(subs);
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-06 20:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-06 20:30 [PATCH 0/3] ALSA: a few more USB-audio fixes Takashi Iwai
2021-02-06 20:30 ` [PATCH 1/3] ALSA: usb-audio: Handle invalid running state at releasing EP Takashi Iwai
2021-02-06 20:30 ` [PATCH 2/3] ALSA: usb-audio: More strict state change in EP Takashi Iwai
2021-02-06 20:30 ` [PATCH 3/3] ALSA: usb-audio: Don't avoid stopping the stream at disconnection Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.