[alsa-devel] [PATCH 0/5] usb-audio endpoint code cleanup
Hi,
while debugging the recent usb-audio bugs, I noticed some function calls in endpoint handling are a bit messy, based on the old infrastructure. Here is a series of small cleanup patches.
Takashi
The async unlink behavior has been working over years. The option was provided only as a workaround for 2.4.x kernel. Let's get rid of it.
Signed-off-by: Takashi Iwai tiwai@suse.de --- Documentation/sound/alsa/ALSA-Configuration.txt | 3 --- sound/usb/card.c | 7 ------- sound/usb/endpoint.c | 2 +- sound/usb/usbaudio.h | 1 - 4 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/Documentation/sound/alsa/ALSA-Configuration.txt b/Documentation/sound/alsa/ALSA-Configuration.txt index d90d8ec..b9cfd33 100644 --- a/Documentation/sound/alsa/ALSA-Configuration.txt +++ b/Documentation/sound/alsa/ALSA-Configuration.txt @@ -1905,7 +1905,6 @@ Prior to version 0.9.0rc4 options had a 'snd_' prefix. This was removed. vid - Vendor ID for the device (optional) pid - Product ID for the device (optional) nrpacks - Max. number of packets per URB (default: 8) - async_unlink - Use async unlink mode (default: yes) device_setup - Device specific magic number (optional) - Influence depends on the device - Default: 0x0000 @@ -1917,8 +1916,6 @@ Prior to version 0.9.0rc4 options had a 'snd_' prefix. This was removed. NB: nrpacks parameter can be modified dynamically via sysfs. Don't put the value over 20. Changing via sysfs has no sanity check. - NB: async_unlink=0 would cause Oops. It remains just for - debugging purpose (if any). NB: ignore_ctl_error=1 may help when you get an error at accessing the mixer element such as URB error -22. This happens on some buggy USB device or the controller. diff --git a/sound/usb/card.c b/sound/usb/card.c index dbf7999..ccf95cf 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -25,9 +25,6 @@ * * NOTES: * - * - async unlink should be used for avoiding the sleep inside lock. - * 2.4.22 usb-uhci seems buggy for async unlinking and results in - * oops. in such a cse, pass async_unlink=0 option. * - the linked URBs would be preferred but not used so far because of * the instability of unlinking. * - type II is not supported properly. there is no device which supports @@ -83,7 +80,6 @@ static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;/* Enable this card * static int vid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; static int pid[SNDRV_CARDS] = { [0 ... (SNDRV_CARDS-1)] = -1 }; static int nrpacks = 8; /* max. number of packets per urb */ -static bool async_unlink = 1; static int device_setup[SNDRV_CARDS]; /* device parameter for this card */ static bool ignore_ctl_error;
@@ -99,8 +95,6 @@ module_param_array(pid, int, NULL, 0444); MODULE_PARM_DESC(pid, "Product ID for the USB audio device."); module_param(nrpacks, int, 0644); MODULE_PARM_DESC(nrpacks, "Max. number of packets per URB."); -module_param(async_unlink, bool, 0444); -MODULE_PARM_DESC(async_unlink, "Use async unlink mode."); module_param_array(device_setup, int, NULL, 0444); MODULE_PARM_DESC(device_setup, "Specific device setup (if needed)."); module_param(ignore_ctl_error, bool, 0444); @@ -345,7 +339,6 @@ static int snd_usb_audio_create(struct usb_device *dev, int idx, chip->card = card; chip->setup = device_setup[idx]; chip->nrpacks = nrpacks; - chip->async_unlink = async_unlink; chip->probing = 1;
chip->usb_id = USB_ID(le16_to_cpu(dev->descriptor.idVendor), diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 51a9aa3..d7382a5 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -523,7 +523,7 @@ static int deactivate_urbs(struct snd_usb_endpoint *ep, int force, int can_sleep if (!force && ep->chip->shutdown) /* to be sure... */ return -EBADFD;
- async = !can_sleep && ep->chip->async_unlink; + async = !can_sleep;
clear_bit(EP_FLAG_RUNNING, &ep->flags);
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h index ef42797..1ac3fd9 100644 --- a/sound/usb/usbaudio.h +++ b/sound/usb/usbaudio.h @@ -56,7 +56,6 @@ struct snd_usb_audio {
int setup; /* from the 'device_setup' module param */ int nrpacks; /* from the 'nrpacks' module param */ - int async_unlink; /* from the 'async_unlink' module param */
struct usb_host_interface *ctrl_intf; /* the audio control interface */ };
Reduce the redundant arguments for snd_usb_endpoint_start() and snd_usb_endpoint_stop(). Also replaced from int to bool.
No functional changes by this commit.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/endpoint.c | 17 ++++++++--------- sound/usb/endpoint.h | 5 ++--- sound/usb/pcm.c | 25 +++++++++++-------------- 3 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index d7382a5..4d50bbe 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -515,7 +515,7 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep) /* * unlink active urbs. */ -static int deactivate_urbs(struct snd_usb_endpoint *ep, int force, int can_sleep) +static int deactivate_urbs(struct snd_usb_endpoint *ep, bool force, bool can_sleep) { unsigned int i; int async; @@ -561,7 +561,7 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force) ep->prepare_data_urb = NULL;
/* stop urbs */ - deactivate_urbs(ep, force, 1); + deactivate_urbs(ep, force, true); wait_clear_urbs(ep);
for (i = 0; i < ep->nurbs; i++) @@ -824,7 +824,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, * * Returns an error if the URB submission failed, 0 in all other cases. */ -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep) +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) { int err; unsigned int i; @@ -837,7 +837,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep) return 0;
/* just to be sure */ - deactivate_urbs(ep, 0, can_sleep); + deactivate_urbs(ep, false, can_sleep); if (can_sleep) wait_clear_urbs(ep);
@@ -891,7 +891,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep) __error: clear_bit(EP_FLAG_RUNNING, &ep->flags); ep->use_count--; - deactivate_urbs(ep, 0, 0); + deactivate_urbs(ep, false, false); return -EPIPE; }
@@ -906,8 +906,7 @@ __error: * * Must be balanced to calls of snd_usb_endpoint_start(). */ -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, - int force, int can_sleep, int wait) +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait) { if (!ep) return; @@ -916,7 +915,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, return;
if (--ep->use_count == 0) { - deactivate_urbs(ep, force, can_sleep); + deactivate_urbs(ep, false, wait); ep->data_subs = NULL; ep->sync_slave = NULL; ep->retire_data_urb = NULL; @@ -947,7 +946,7 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) if (!ep) return -EINVAL;
- deactivate_urbs(ep, 1, 1); + deactivate_urbs(ep, true, true); wait_clear_urbs(ep);
if (ep->use_count != 0) diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index 3d4c970..f1e451d 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -16,9 +16,8 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, struct audioformat *fmt, struct snd_usb_endpoint *sync_ep);
-int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep); -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, - int force, int can_sleep, int wait); +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index e7329d0..d90604a 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -211,7 +211,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, } }
-static int start_endpoints(struct snd_usb_substream *subs, int can_sleep) +static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) { int err;
@@ -263,16 +263,13 @@ static int start_endpoints(struct snd_usb_substream *subs, int can_sleep) return 0; }
-static void stop_endpoints(struct snd_usb_substream *subs, - int force, int can_sleep, int wait) +static void stop_endpoints(struct snd_usb_substream *subs, bool wait) { if (test_and_clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) - snd_usb_endpoint_stop(subs->sync_endpoint, - force, can_sleep, wait); + snd_usb_endpoint_stop(subs->sync_endpoint, wait);
if (test_and_clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags)) - snd_usb_endpoint_stop(subs->data_endpoint, - force, can_sleep, wait); + snd_usb_endpoint_stop(subs->data_endpoint, wait); }
static int deactivate_endpoints(struct snd_usb_substream *subs) @@ -444,7 +441,7 @@ static int configure_endpoint(struct snd_usb_substream *subs) int ret;
/* format changed */ - stop_endpoints(subs, 0, 0, 0); + stop_endpoints(subs, false); ret = snd_usb_endpoint_set_params(subs->data_endpoint, subs->pcm_format, subs->channels, @@ -530,7 +527,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) subs->period_bytes = 0; down_read(&subs->stream->chip->shutdown_rwsem); if (!subs->stream->chip->shutdown) { - stop_endpoints(subs, 0, 1, 1); + stop_endpoints(subs, true); deactivate_endpoints(subs); } up_read(&subs->stream->chip->shutdown_rwsem); @@ -605,7 +602,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) /* for playback, submit the URBs now; otherwise, the first hwptr_done * updates for all URBs would happen at the same time when starting */ if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) - ret = start_endpoints(subs, 1); + ret = start_endpoints(subs, true);
unlock: up_read(&subs->stream->chip->shutdown_rwsem); @@ -1010,7 +1007,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) struct snd_usb_stream *as = snd_pcm_substream_chip(substream); struct snd_usb_substream *subs = &as->substream[direction];
- stop_endpoints(subs, 0, 0, 0); + stop_endpoints(subs, false);
if (!as->chip->shutdown && subs->interface >= 0) { usb_set_interface(subs->dev, subs->interface, 0); @@ -1245,7 +1242,7 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: - stop_endpoints(subs, 0, 0, 0); + stop_endpoints(subs, false); subs->running = 0; return 0; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: @@ -1266,7 +1263,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream
switch (cmd) { case SNDRV_PCM_TRIGGER_START: - err = start_endpoints(subs, 0); + err = start_endpoints(subs, false); if (err < 0) return err;
@@ -1274,7 +1271,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream subs->running = 1; return 0; case SNDRV_PCM_TRIGGER_STOP: - stop_endpoints(subs, 0, 0, 0); + stop_endpoints(subs, false); subs->running = 0; return 0; case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
For further code simplification, drop the conditional call for usb_kill_urb() with can_wait argument in deactivate_urbs(), and use only usb_unlink_urb() and wait_clear_urbs() pairs.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/endpoint.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 4d50bbe..6db2143 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -515,33 +515,24 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep) /* * unlink active urbs. */ -static int deactivate_urbs(struct snd_usb_endpoint *ep, bool force, bool can_sleep) +static int deactivate_urbs(struct snd_usb_endpoint *ep, bool force) { unsigned int i; - int async;
if (!force && ep->chip->shutdown) /* to be sure... */ return -EBADFD;
- async = !can_sleep; - clear_bit(EP_FLAG_RUNNING, &ep->flags);
INIT_LIST_HEAD(&ep->ready_playback_urbs); ep->next_packet_read_pos = 0; ep->next_packet_write_pos = 0;
- if (!async && in_interrupt()) - return 0; - for (i = 0; i < ep->nurbs; i++) { if (test_bit(i, &ep->active_mask)) { if (!test_and_set_bit(i, &ep->unlink_mask)) { struct urb *u = ep->urb[i].urb; - if (async) - usb_unlink_urb(u); - else - usb_kill_urb(u); + usb_unlink_urb(u); } } } @@ -561,7 +552,7 @@ static void release_urbs(struct snd_usb_endpoint *ep, int force) ep->prepare_data_urb = NULL;
/* stop urbs */ - deactivate_urbs(ep, force, true); + deactivate_urbs(ep, force); wait_clear_urbs(ep);
for (i = 0; i < ep->nurbs; i++) @@ -837,7 +828,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) return 0;
/* just to be sure */ - deactivate_urbs(ep, false, can_sleep); + deactivate_urbs(ep, false); if (can_sleep) wait_clear_urbs(ep);
@@ -891,7 +882,7 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) __error: clear_bit(EP_FLAG_RUNNING, &ep->flags); ep->use_count--; - deactivate_urbs(ep, false, false); + deactivate_urbs(ep, false); return -EPIPE; }
@@ -915,7 +906,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait) return;
if (--ep->use_count == 0) { - deactivate_urbs(ep, false, wait); + deactivate_urbs(ep, false); ep->data_subs = NULL; ep->sync_slave = NULL; ep->retire_data_urb = NULL; @@ -946,7 +937,7 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep) if (!ep) return -EINVAL;
- deactivate_urbs(ep, true, true); + deactivate_urbs(ep, true); wait_clear_urbs(ep);
if (ep->use_count != 0)
As we are stopping the endpoints asynchronously now, it's better to trigger the stop of both data and sync endpoints and wait for pending stopping operations, instead of the sequential trigger-and-wait procedure.
So the wait argument in snd_usb_endpoint_stop() is dropped, and it's expected that the caller synchronizes explicitly by calling snd_usb_endpoint_sync_pending_stop(). (Actually there is only one place calling this, so it was safe to change.)
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/endpoint.c | 11 +++++------ sound/usb/endpoint.h | 2 +- sound/usb/pcm.c | 9 +++++++-- 3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 6db2143..f487d26 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -896,8 +896,11 @@ __error: * actually be deactivated. * * Must be balanced to calls of snd_usb_endpoint_start(). + * + * The caller needs to synchronize the pending stop operation via + * snd_usb_endpoint_sync_pending_stop(). */ -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait) +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) { if (!ep) return; @@ -911,11 +914,7 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait) ep->sync_slave = NULL; ep->retire_data_urb = NULL; ep->prepare_data_urb = NULL; - - if (wait) - wait_clear_urbs(ep); - else - set_bit(EP_FLAG_STOPPING, &ep->flags); + set_bit(EP_FLAG_STOPPING, &ep->flags); } }
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index f1e451d..447902d 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -17,7 +17,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sync_ep);
int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); -void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, bool wait); +void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index d90604a..4750d3d 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -266,10 +266,15 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) static void stop_endpoints(struct snd_usb_substream *subs, bool wait) { if (test_and_clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) - snd_usb_endpoint_stop(subs->sync_endpoint, wait); + snd_usb_endpoint_stop(subs->sync_endpoint);
if (test_and_clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags)) - snd_usb_endpoint_stop(subs->data_endpoint, wait); + snd_usb_endpoint_stop(subs->data_endpoint); + + if (wait) { + snd_usb_endpoint_sync_pending_stop(subs->sync_endpoint); + snd_usb_endpoint_sync_pending_stop(subs->data_endpoint); + } }
static int deactivate_endpoints(struct snd_usb_substream *subs)
PCM hw_free and close should wait until all the pending stop operations have been finished. Basically only PCM trigger callback should use non-wait calls.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 4750d3d..bc3c9ac 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -446,7 +446,7 @@ static int configure_endpoint(struct snd_usb_substream *subs) int ret;
/* format changed */ - stop_endpoints(subs, false); + stop_endpoints(subs, true); ret = snd_usb_endpoint_set_params(subs->data_endpoint, subs->pcm_format, subs->channels, @@ -1012,7 +1012,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) struct snd_usb_stream *as = snd_pcm_substream_chip(substream); struct snd_usb_substream *subs = &as->substream[direction];
- stop_endpoints(subs, false); + stop_endpoints(subs, true);
if (!as->chip->shutdown && subs->interface >= 0) { usb_set_interface(subs->dev, subs->interface, 0);
participants (1)
-
Takashi Iwai