[alsa-devel] [PATCH 00/16] Yet more cleanups of line6 drivers
Hi,
here is a series of cleanup patches for line6 stuff. They are applied on top of my previous patches. The latest patchset is found in test/line6 branch of sound.git tree.
Takashi
Takashi Iwai (16): ALSA: line6: Minor refactoring ALSA: line6: Fix memory leak at probe error path ALSA: line6: Remove unused line6_nop_read() ALSA: line6: Reduce superfluous spinlock in midi.c ALSA: line6: Fix missing error handling in line6_pcm_acquire() ALSA: line6: Use logical OR ALSA: line6: Fix the error recovery in line6_pcm_acquire() ALSA: line6: Drop superfluous spinlock for trigger ALSA: line6: Use incremental loop ALSA: line6: Drop voodoo workarounds ALSA: line6: Rearrange PCM structure ALSA: line6: Consolidate URB unlink and sync helpers ALSA: line6: Use dev_err() ALSA: line6: Consolidate PCM stream buffer allocation and free ALSA: line6: Do clipping in volume / monitor manipulations ALSA: line6: Skip volume manipulation during silence copying
sound/usb/line6/capture.c | 122 ++++++-------------------- sound/usb/line6/capture.h | 5 -- sound/usb/line6/driver.c | 157 ++++++++++++++++----------------- sound/usb/line6/driver.h | 2 - sound/usb/line6/midi.c | 18 ++-- sound/usb/line6/midi.h | 7 +- sound/usb/line6/pcm.c | 210 +++++++++++++++++++++++++++------------------ sound/usb/line6/pcm.h | 163 ++++++++++------------------------- sound/usb/line6/playback.c | 159 ++++++++++------------------------ sound/usb/line6/playback.h | 5 -- 10 files changed, 329 insertions(+), 519 deletions(-)
Split some codes in the lengthy line6_probe().
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 94 +++++++++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 45 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index e7f9a99e1949..b783c0788e45 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -448,6 +448,52 @@ static void line6_destruct(struct snd_card *card) usb_put_dev(usbdev); }
+/* get data from endpoint descriptor (see usb_maxpacket): */ +static void line6_get_interval(struct usb_line6 *line6) +{ + struct usb_device *usbdev = line6->usbdev; + struct usb_host_endpoint *ep; + unsigned pipe = usb_rcvintpipe(usbdev, line6->properties->ep_ctrl_r); + unsigned epnum = usb_pipeendpoint(pipe); + + ep = usbdev->ep_in[epnum]; + if (ep) { + line6->interval = ep->desc.bInterval; + line6->max_packet_size = le16_to_cpu(ep->desc.wMaxPacketSize); + } else { + dev_err(line6->ifcdev, + "endpoint not available, using fallback values"); + line6->interval = LINE6_FALLBACK_INTERVAL; + line6->max_packet_size = LINE6_FALLBACK_MAXPACKETSIZE; + } +} + +static int line6_init_cap_control(struct usb_line6 *line6) +{ + int ret; + + /* initialize USB buffers: */ + line6->buffer_listen = kmalloc(LINE6_BUFSIZE_LISTEN, GFP_KERNEL); + if (!line6->buffer_listen) + return -ENOMEM; + + line6->buffer_message = kmalloc(LINE6_MESSAGE_MAXLEN, GFP_KERNEL); + if (!line6->buffer_message) + return -ENOMEM; + + line6->urb_listen = usb_alloc_urb(0, GFP_KERNEL); + if (!line6->urb_listen) + return -ENOMEM; + + ret = line6_start_listen(line6); + if (ret < 0) { + dev_err(line6->ifcdev, "cannot start listening: %d\n", ret); + return ret; + } + + return 0; +} + /* Probe USB device. */ @@ -485,24 +531,7 @@ int line6_probe(struct usb_interface *interface, line6->usbdev = usbdev; line6->ifcdev = &interface->dev;
- /* get data from endpoint descriptor (see usb_maxpacket): */ - { - struct usb_host_endpoint *ep; - unsigned pipe = usb_rcvintpipe(usbdev, properties->ep_ctrl_r); - unsigned epnum = usb_pipeendpoint(pipe); - ep = usbdev->ep_in[epnum]; - - if (ep != NULL) { - line6->interval = ep->desc.bInterval; - line6->max_packet_size = - le16_to_cpu(ep->desc.wMaxPacketSize); - } else { - line6->interval = LINE6_FALLBACK_INTERVAL; - line6->max_packet_size = LINE6_FALLBACK_MAXPACKETSIZE; - dev_err(line6->ifcdev, - "endpoint not available, using fallback values"); - } - } + line6_get_interval(line6);
ret = snd_card_new(line6->ifcdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, @@ -525,34 +554,9 @@ int line6_probe(struct usb_interface *interface, usb_get_dev(usbdev);
if (properties->capabilities & LINE6_CAP_CONTROL) { - /* initialize USB buffers: */ - line6->buffer_listen = - kmalloc(LINE6_BUFSIZE_LISTEN, GFP_KERNEL); - if (line6->buffer_listen == NULL) { - ret = -ENOMEM; - goto err_destruct; - } - - line6->buffer_message = - kmalloc(LINE6_MESSAGE_MAXLEN, GFP_KERNEL); - if (line6->buffer_message == NULL) { - ret = -ENOMEM; + ret = line6_init_cap_control(line6); + if (ret < 0) goto err_destruct; - } - - line6->urb_listen = usb_alloc_urb(0, GFP_KERNEL); - - if (line6->urb_listen == NULL) { - ret = -ENOMEM; - goto err_destruct; - } - - ret = line6_start_listen(line6); - if (ret < 0) { - dev_err(&interface->dev, "%s: usb_submit_urb failed\n", - __func__); - goto err_destruct; - } }
/* initialize device data based on device: */
Fix memory leak at probe error path by rearranging the call order in line6_destruct() so that the common destructor is always called. Also this simplifies the error path to a single goto label.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 53 ++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index b783c0788e45..2eed6fbd99c4 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,10 +507,32 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
+ ret = snd_card_new(line6->ifcdev, + SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, + THIS_MODULE, 0, &card); + if (ret < 0) { + kfree(line6); + return ret; + } + + line6->card = card; + strcpy(card->id, line6->properties->id); + strcpy(card->driver, DRIVER_NAME); + strcpy(card->shortname, line6->properties->name); + sprintf(card->longname, "Line 6 %s at USB %s", line6->properties->name, + dev_name(line6->ifcdev)); + card->private_data = line6; + card->private_free = line6_destruct; + + usb_set_intfdata(interface, line6); + + /* increment reference counters: */ + usb_get_dev(usbdev); + /* we don't handle multiple configurations */ if (usbdev->descriptor.bNumConfigurations != 1) { ret = -ENODEV; - goto err_put; + goto error; }
/* initialize device info: */ @@ -523,7 +545,7 @@ int line6_probe(struct usb_interface *interface, properties->altsetting); if (ret < 0) { dev_err(&interface->dev, "set_interface failed\n"); - goto err_put; + goto error; }
/* store basic data: */ @@ -533,36 +555,16 @@ int line6_probe(struct usb_interface *interface,
line6_get_interval(line6);
- ret = snd_card_new(line6->ifcdev, - SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, - THIS_MODULE, 0, &card); - if (ret < 0) - goto err_put; - - line6->card = card; - strcpy(card->id, line6->properties->id); - strcpy(card->driver, DRIVER_NAME); - strcpy(card->shortname, line6->properties->name); - sprintf(card->longname, "Line 6 %s at USB %s", line6->properties->name, - dev_name(line6->ifcdev)); - card->private_data = line6; - card->private_free = line6_destruct; - - usb_set_intfdata(interface, line6); - - /* increment reference counters: */ - usb_get_dev(usbdev); - if (properties->capabilities & LINE6_CAP_CONTROL) { ret = line6_init_cap_control(line6); if (ret < 0) - goto err_destruct; + goto error; }
/* initialize device data based on device: */ ret = private_init(interface, line6); if (ret < 0) - goto err_destruct; + goto error;
/* creation of additional special files should go here */
@@ -571,11 +573,10 @@ int line6_probe(struct usb_interface *interface,
return 0;
- err_destruct: + error: if (line6->disconnect) line6->disconnect(interface); snd_card_free(card); - err_put: return ret; } EXPORT_SYMBOL_GPL(line6_probe);
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@suse.de wrote:
--- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,10 +507,32 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
ret = snd_card_new(line6->ifcdev,SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,THIS_MODULE, 0, &card);
The `ifcdev' member has not been initialized yet. You need something like the following squashed in:
8------------------------------------------------------8<
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 2eed6fb..25d6b0f 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,6 +507,11 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
+ /* store basic data: */ + line6->properties = properties; + line6->usbdev = usbdev; + line6->ifcdev = &interface->dev; + ret = snd_card_new(line6->ifcdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, THIS_MODULE, 0, &card); @@ -548,11 +553,6 @@ int line6_probe(struct usb_interface *interface, goto error; }
- /* store basic data: */ - line6->properties = properties; - line6->usbdev = usbdev; - line6->ifcdev = &interface->dev; - line6_get_interval(line6);
if (properties->capabilities & LINE6_CAP_CONTROL) {
At Sun, 25 Jan 2015 02:06:19 -0600, Chris Rorvick wrote:
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@suse.de wrote:
--- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,10 +507,32 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
ret = snd_card_new(line6->ifcdev,SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,THIS_MODULE, 0, &card);The `ifcdev' member has not been initialized yet.
Good catch.
You need something like the following squashed in:
8------------------------------------------------------8<
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 2eed6fb..25d6b0f 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,6 +507,11 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
/* store basic data: */line6->properties = properties;line6->usbdev = usbdev;line6->ifcdev = &interface->dev;ret = snd_card_new(line6->ifcdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, THIS_MODULE, 0, &card);@@ -548,11 +553,6 @@ int line6_probe(struct usb_interface *interface, goto error; }
/* store basic data: */line6->properties = properties;line6->usbdev = usbdev;line6->ifcdev = &interface->dev;line6_get_interval(line6); if (properties->capabilities & LINE6_CAP_CONTROL) {
Right, but there will be another round of cleanup patches and the allocation of line6 will be dropped there, so I took an easier fixup, just use &interface->dev to the argument.
The revised patch is found below. The test/line6 branch was updated with this (and more fixes I'll send shortly later).
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: line6: Fix memory leak at probe error path
Fix memory leak at probe error path by rearranging the call order in line6_destruct() so that the common destructor is always called. Also this simplifies the error path to a single goto label.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 59 ++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index b783c0788e45..bf9630cd2395 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -507,39 +507,20 @@ int line6_probe(struct usb_interface *interface, int interface_number; int ret;
- /* we don't handle multiple configurations */ - if (usbdev->descriptor.bNumConfigurations != 1) { - ret = -ENODEV; - goto err_put; - } - - /* initialize device info: */ - dev_info(&interface->dev, "Line 6 %s found\n", properties->name); - - /* query interface number */ - interface_number = interface->cur_altsetting->desc.bInterfaceNumber; - - ret = usb_set_interface(usbdev, interface_number, - properties->altsetting); + ret = snd_card_new(&interface->dev, + SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, + THIS_MODULE, 0, &card); if (ret < 0) { - dev_err(&interface->dev, "set_interface failed\n"); - goto err_put; + kfree(line6); + return ret; }
/* store basic data: */ + line6->card = card; line6->properties = properties; line6->usbdev = usbdev; line6->ifcdev = &interface->dev;
- line6_get_interval(line6); - - ret = snd_card_new(line6->ifcdev, - SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, - THIS_MODULE, 0, &card); - if (ret < 0) - goto err_put; - - line6->card = card; strcpy(card->id, line6->properties->id); strcpy(card->driver, DRIVER_NAME); strcpy(card->shortname, line6->properties->name); @@ -553,16 +534,37 @@ int line6_probe(struct usb_interface *interface, /* increment reference counters: */ usb_get_dev(usbdev);
+ /* we don't handle multiple configurations */ + if (usbdev->descriptor.bNumConfigurations != 1) { + ret = -ENODEV; + goto error; + } + + /* initialize device info: */ + dev_info(&interface->dev, "Line 6 %s found\n", properties->name); + + /* query interface number */ + interface_number = interface->cur_altsetting->desc.bInterfaceNumber; + + ret = usb_set_interface(usbdev, interface_number, + properties->altsetting); + if (ret < 0) { + dev_err(&interface->dev, "set_interface failed\n"); + goto error; + } + + line6_get_interval(line6); + if (properties->capabilities & LINE6_CAP_CONTROL) { ret = line6_init_cap_control(line6); if (ret < 0) - goto err_destruct; + goto error; }
/* initialize device data based on device: */ ret = private_init(interface, line6); if (ret < 0) - goto err_destruct; + goto error;
/* creation of additional special files should go here */
@@ -571,11 +573,10 @@ int line6_probe(struct usb_interface *interface,
return 0;
- err_destruct: + error: if (line6->disconnect) line6->disconnect(interface); snd_card_free(card); - err_put: return ret; } EXPORT_SYMBOL_GPL(line6_probe);
The function isn't used any longer after rewriting from sysfs to leds class in toneport.c.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 10 ---------- sound/usb/line6/driver.h | 2 -- 2 files changed, 12 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 2eed6fbd99c4..d6023c9a8a61 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -413,16 +413,6 @@ int line6_read_serial_number(struct usb_line6 *line6, int *serial_number) EXPORT_SYMBOL_GPL(line6_read_serial_number);
/* - No operation (i.e., unsupported). -*/ -ssize_t line6_nop_read(struct device *dev, struct device_attribute *attr, - char *buf) -{ - return 0; -} -EXPORT_SYMBOL_GPL(line6_nop_read); - -/* Card destructor. */ static void line6_destruct(struct snd_card *card) diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h index efd58ac3215b..a6c0b2f08ba8 100644 --- a/sound/usb/line6/driver.h +++ b/sound/usb/line6/driver.h @@ -162,8 +162,6 @@ struct usb_line6 {
extern char *line6_alloc_sysex_buffer(struct usb_line6 *line6, int code1, int code2, int size); -extern ssize_t line6_nop_read(struct device *dev, - struct device_attribute *attr, char *buf); extern int line6_read_data(struct usb_line6 *line6, int address, void *data, size_t datalen); extern int line6_read_serial_number(struct usb_line6 *line6,
The midi_transmit_lock is used always inside the send_urb_lock, thus it doesn't play any role. Let's kill it. Also, rename "send_urb_lock" as a more simple name "lock" since this is the only lock for midi.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/midi.c | 18 ++++++------------ sound/usb/line6/midi.h | 7 +------ 2 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c index b5a58a7fe11a..beeedf9a2cbe 100644 --- a/sound/usb/line6/midi.c +++ b/sound/usb/line6/midi.c @@ -45,12 +45,9 @@ static void line6_midi_transmit(struct snd_rawmidi_substream *substream) line6_rawmidi_substream_midi(substream)->line6; struct snd_line6_midi *line6midi = line6->line6midi; struct midi_buffer *mb = &line6midi->midibuf_out; - unsigned long flags; unsigned char chunk[LINE6_FALLBACK_MAXPACKETSIZE]; int req, done;
- spin_lock_irqsave(&line6->line6midi->midi_transmit_lock, flags); - for (;;) { req = min(line6_midibuf_bytes_free(mb), line6->max_packet_size); done = snd_rawmidi_transmit_peek(substream, chunk, req); @@ -71,8 +68,6 @@ static void line6_midi_transmit(struct snd_rawmidi_substream *substream)
send_midi_async(line6, chunk, done); } - - spin_unlock_irqrestore(&line6->line6midi->midi_transmit_lock, flags); }
/* @@ -92,7 +87,7 @@ static void midi_sent(struct urb *urb) if (status == -ESHUTDOWN) return;
- spin_lock_irqsave(&line6->line6midi->send_urb_lock, flags); + spin_lock_irqsave(&line6->line6midi->lock, flags); num = --line6->line6midi->num_active_send_urbs;
if (num == 0) { @@ -103,12 +98,12 @@ static void midi_sent(struct urb *urb) if (num == 0) wake_up(&line6->line6midi->send_wait);
- spin_unlock_irqrestore(&line6->line6midi->send_urb_lock, flags); + spin_unlock_irqrestore(&line6->line6midi->lock, flags); }
/* Send an asynchronous MIDI message. - Assumes that line6->line6midi->send_urb_lock is held + Assumes that line6->line6midi->lock is held (i.e., this function is serialized). */ static int send_midi_async(struct usb_line6 *line6, unsigned char *data, @@ -166,12 +161,12 @@ static void line6_midi_output_trigger(struct snd_rawmidi_substream *substream, line6_rawmidi_substream_midi(substream)->line6;
line6->line6midi->substream_transmit = substream; - spin_lock_irqsave(&line6->line6midi->send_urb_lock, flags); + spin_lock_irqsave(&line6->line6midi->lock, flags);
if (line6->line6midi->num_active_send_urbs == 0) line6_midi_transmit(substream);
- spin_unlock_irqrestore(&line6->line6midi->send_urb_lock, flags); + spin_unlock_irqrestore(&line6->line6midi->lock, flags); }
static void line6_midi_output_drain(struct snd_rawmidi_substream *substream) @@ -281,8 +276,7 @@ int line6_init_midi(struct usb_line6 *line6) rmidi->private_free = snd_line6_midi_free;
init_waitqueue_head(&line6midi->send_wait); - spin_lock_init(&line6midi->send_urb_lock); - spin_lock_init(&line6midi->midi_transmit_lock); + spin_lock_init(&line6midi->lock); line6midi->line6 = line6;
err = line6_midibuf_init(&line6midi->midibuf_in, MIDI_BUFFER_SIZE, 0); diff --git a/sound/usb/line6/midi.h b/sound/usb/line6/midi.h index ba6bf3828aa5..9d9467b2613c 100644 --- a/sound/usb/line6/midi.h +++ b/sound/usb/line6/midi.h @@ -40,14 +40,9 @@ struct snd_line6_midi { int num_active_send_urbs;
/** - Spin lock to protect updates of send_urb. - */ - spinlock_t send_urb_lock; - - /** Spin lock to protect MIDI buffer handling. */ - spinlock_t midi_transmit_lock; + spinlock_t lock;
/** Wait queue for MIDI transmission.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 8a6059adef69..826158fe9149 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -133,7 +133,8 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) */ if (line6pcm->active_urb_in | line6pcm->unlink_urb_in) { dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n"); - return -EBUSY; + err = -EBUSY; + goto pcm_acquire_error; }
line6pcm->count_in = 0;
Fixed a few places using bits OR wrongly for condition checks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 826158fe9149..f740b4490d75 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -131,7 +131,7 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) a bug, we therefore report an error if capturing is restarted too soon. */ - if (line6pcm->active_urb_in | line6pcm->unlink_urb_in) { + if (line6pcm->active_urb_in || line6pcm->unlink_urb_in) { dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n"); err = -EBUSY; goto pcm_acquire_error; @@ -166,7 +166,7 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) /* See comment above regarding PCM restart. */ - if (line6pcm->active_urb_out | line6pcm->unlink_urb_out) { + if (line6pcm->active_urb_out || line6pcm->unlink_urb_out) { dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n"); return -EBUSY; }
line6_pcm_acquire() tries to restore the newly obtained resources at the error path. But some flags aren't recorded and released properly when the corresponding buffer is already present. These bits have to be cleared in the error recovery, too.
Also, "flags_final" can be initialized to zero since we pass only the subset of "channels" bits.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pcm.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index f740b4490d75..9a2a15f4c4e4 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -106,7 +106,7 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) flags_new = flags_old | channels; } while (cmpxchg(&line6pcm->flags, flags_old, flags_new) != flags_old);
- flags_final = flags_old; + flags_final = 0;
line6pcm->prev_fbuf = NULL;
@@ -120,9 +120,9 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) err = -ENOMEM; goto pcm_acquire_error; } - - flags_final |= channels & LINE6_BITS_CAPTURE_BUFFER; } + + flags_final |= channels & LINE6_BITS_CAPTURE_BUFFER; }
if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_STREAM)) { @@ -157,9 +157,9 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) err = -ENOMEM; goto pcm_acquire_error; } - - flags_final |= channels & LINE6_BITS_PLAYBACK_BUFFER; } + + flags_final |= channels & LINE6_BITS_PLAYBACK_BUFFER; }
if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_STREAM)) { @@ -187,7 +187,7 @@ pcm_acquire_error: If not all requested resources/streams could be obtained, release those which were successfully obtained (if any). */ - line6_pcm_release(line6pcm, flags_final & channels); + line6_pcm_release(line6pcm, flags_final); return err; } EXPORT_SYMBOL_GPL(line6_pcm_acquire);
The trigger callback is already spinlocked, so we need no more lock here (even for the linked substreams). Let's drop it.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pcm.c | 23 ++++++----------------- sound/usb/line6/pcm.h | 5 ----- 2 files changed, 6 insertions(+), 22 deletions(-)
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 9a2a15f4c4e4..adbcac46b785 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -226,9 +226,8 @@ int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream); struct snd_pcm_substream *s; - int err; + int err = 0;
- spin_lock(&line6pcm->lock_trigger); clear_bit(LINE6_INDEX_PREPARED, &line6pcm->flags);
snd_pcm_group_for_each_entry(s, substream) { @@ -237,32 +236,23 @@ int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd) switch (s->stream) { case SNDRV_PCM_STREAM_PLAYBACK: err = snd_line6_playback_trigger(line6pcm, cmd); - - if (err < 0) { - spin_unlock(&line6pcm->lock_trigger); - return err; - } - break;
case SNDRV_PCM_STREAM_CAPTURE: err = snd_line6_capture_trigger(line6pcm, cmd); - - if (err < 0) { - spin_unlock(&line6pcm->lock_trigger); - return err; - } - break;
default: dev_err(line6pcm->line6->ifcdev, "Unknown stream direction %d\n", s->stream); + err = -EINVAL; + break; } + if (err < 0) + break; }
- spin_unlock(&line6pcm->lock_trigger); - return 0; + return err; }
/* control info callback */ @@ -427,7 +417,6 @@ int line6_init_pcm(struct usb_line6 *line6,
spin_lock_init(&line6pcm->lock_audio_out); spin_lock_init(&line6pcm->lock_audio_in); - spin_lock_init(&line6pcm->lock_trigger); line6pcm->impulse_period = LINE6_IMPULSE_DEFAULT_PERIOD;
line6->line6pcm = line6pcm; diff --git a/sound/usb/line6/pcm.h b/sound/usb/line6/pcm.h index c742b33666eb..a84753ee0fa2 100644 --- a/sound/usb/line6/pcm.h +++ b/sound/usb/line6/pcm.h @@ -308,11 +308,6 @@ struct snd_line6_pcm { spinlock_t lock_audio_in;
/** - Spin lock to protect trigger. - */ - spinlock_t lock_trigger; - - /** PCM playback volume (left and right). */ int volume_playback[2];
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@suse.de wrote:
The trigger callback is already spinlocked, so we need no more lock here (even for the linked substreams). Let's drop it.
Are they linked? It doesn't look like they are for my TonePort UX2. I assumed this is why the trigger lock existed, to synchronize the trigger callbacks between playback and capture because the group lock was *not* being acquired.
And the purpose of this synchronization I assumed was to really synchronize the calls to line6_pcm_acquire/release(). But then hw_params() and hw_free() are calling into these seemingly without any lock.
And line6_pcm_acquire/release() are updating line6pcm->flags atomically, but elsewhere we're just using test_and_set_bit() and such.
All of this looks racy to me. Am I missing something?
Finally, what is the point of dispatching through this rather than to the respective playback/capture callback directly? Would this come into play if I was seeing linked substreams?
Sorry, this is way beyond the scope of this patch. But I've been spending some time trying to sort this out and haven't answered the above questions. If I'm missing anything, a pointer to the relevant source would be greatly appreciated!
Regards,
Chris
At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote:
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@suse.de wrote:
The trigger callback is already spinlocked, so we need no more lock here (even for the linked substreams). Let's drop it.
Are they linked? It doesn't look like they are for my TonePort UX2.
They can be linked when specified by application via snd_pcm_link(). It's not always linked.
I assumed this is why the trigger lock existed, to synchronize the trigger callbacks between playback and capture because the group lock was *not* being acquired.
For the trigger part, both LINE6_BITS_PLAYBACK_STREAM and LINE6_BITS_CAPTURE_STREAM can be dealt really independently. Thus there is no reason to protect the parallel triggers for both directions.
And, a stream lock is always held for the trigger callback (in addition to a group lock if linked).
And the purpose of this synchronization I assumed was to really synchronize the calls to line6_pcm_acquire/release(). But then hw_params() and hw_free() are calling into these seemingly without any lock.
Right. And, reading the code more carefully, you'll find that a lock can't help there at all. The current line6_pcm_acquire() and line6_pcm_release() are already racy. At the beginning of each function, it tries to be consistent about the bit flags. But that's all. It sets to the new bits there, and checks the diff between the old and the new. This doesn't protect against the renewal of the same bit while the bit is being handled.
For example, suppose a task A calls line6_pcm_release(*_BUFFER). It clears the *_BUFFER bits and then tries to free a buffer via kfree(). During it, suppose a task B calls line6_pcm_acquire() with the same *_BUFFER flag. The bits was already cleared, so it sets again and processes the *_BUFFER part, where it allocates a buffer via kmalloc(). If the task B is executed before kfree() call of task A, kmalloc() may be performed and replace line6pcm->buffer_out. Then task B calls kfree() with the newly allocated one. Ouch.
And line6_pcm_acquire/release() are updating line6pcm->flags atomically, but elsewhere we're just using test_and_set_bit() and such.
All of this looks racy to me. Am I missing something?
Yes, the code *is* already racy :) And the lock doesn't anything there. It protects the parallel call of playback and capture directions, but these parallel triggers are actually no problem, so it just gives the unnecessary protection.
Finally, what is the point of dispatching through this rather than to the respective playback/capture callback directly? Would this come into play if I was seeing linked substreams?
I can only guess: the reason is likely to simplify the call for IMPULSE or MONITOR cases. In the current form, they can be a simple line6_pcm_acquire() or _release() call with the combined bit flags. If the handling of streams are separated, it'll need multiple calls.
I don't mean that the current form is good, though. Rather I find it makes thing unnecessarily too complex, and as already mentioned, it's racy, doesn't work as expected. So I think we should straighten the code later, in anyway.
Sorry, this is way beyond the scope of this patch. But I've been spending some time trying to sort this out and haven't answered the above questions. If I'm missing anything, a pointer to the relevant source would be greatly appreciated!
HTH,
Takashi
On Tue, Jan 27, 2015 at 4:27 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote:
All of this looks racy to me. Am I missing something?
Yes, the code *is* already racy :)
OK, good to know I'm not seeing ghosts. :-)
Sorry, this is way beyond the scope of this patch. But I've been spending some time trying to sort this out and haven't answered the above questions. If I'm missing anything, a pointer to the relevant source would be greatly appreciated!
HTH,
Yes, very much. Thanks!
Regards,
Chris
At Tue, 27 Jan 2015 06:48:22 -0600, Chris Rorvick wrote:
On Tue, Jan 27, 2015 at 4:27 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote:
All of this looks racy to me. Am I missing something?
Yes, the code *is* already racy :)
OK, good to know I'm not seeing ghosts. :-)
Hehe.
I wrote a few more patches to cover the races in a bit better way (although it's not perfect yet). I'm going to send to ML soon.
Could you give tested-by tag if the latest test/line6 branch works -- at least, no serious regression is seen?
thanks,
Takashi
On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai tiwai@suse.de wrote:
Could you give tested-by tag if the latest test/line6 branch works -- at least, no serious regression is seen?
The "Fix racy loopback handling" is broken due to the typo, but that drops out later and does not affect the final tree. So, very briefly ...
Tested-by: Chris Rorvick chris@rorvick.com
At Wed, 28 Jan 2015 00:03:17 -0600, Chris Rorvick wrote:
On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai tiwai@suse.de wrote:
Could you give tested-by tag if the latest test/line6 branch works -- at least, no serious regression is seen?
The "Fix racy loopback handling" is broken due to the typo, but that drops out later and does not affect the final tree. So, very briefly ...
Tested-by: Chris Rorvick chris@rorvick.com
Thanks! I updated the branch and now merged to for-next.
Takashi
On Wed, Jan 28, 2015 at 12:26 AM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 28 Jan 2015 00:03:17 -0600, Chris Rorvick wrote:
On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai tiwai@suse.de wrote:
Could you give tested-by tag if the latest test/line6 branch works -- at least, no serious regression is seen?
The "Fix racy loopback handling" is broken due to the typo, but that drops out later and does not affect the final tree. So, very briefly ...
Tested-by: Chris Rorvick chris@rorvick.com
Thanks! I updated the branch and now merged to for-next.
You're welcome, looks good! Nice to see the playback/capture buffer/stream flags stuff cleaned up.
Chris
Using a decremental loop without particular reasons worsens the readability a lot. Use incremental loops instead.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/capture.c | 4 ++-- sound/usb/line6/pcm.c | 6 +++--- sound/usb/line6/playback.c | 12 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c index 5a010ba163fa..97283e631e45 100644 --- a/sound/usb/line6/capture.c +++ b/sound/usb/line6/capture.c @@ -91,7 +91,7 @@ void line6_unlink_audio_in_urbs(struct snd_line6_pcm *line6pcm) { unsigned int i;
- for (i = LINE6_ISO_BUFFERS; i--;) { + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { if (test_bit(i, &line6pcm->active_urb_in)) { if (!test_and_set_bit(i, &line6pcm->unlink_urb_in)) { struct urb *u = line6pcm->urb_audio_in[i]; @@ -114,7 +114,7 @@ void line6_wait_clear_audio_in_urbs(struct snd_line6_pcm *line6pcm)
do { alive = 0; - for (i = LINE6_ISO_BUFFERS; i--;) { + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { if (test_bit(i, &line6pcm->active_urb_in)) alive++; } diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index adbcac46b785..43474c4ebb6b 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -273,7 +273,7 @@ static int snd_line6_control_playback_get(struct snd_kcontrol *kcontrol, int i; struct snd_line6_pcm *line6pcm = snd_kcontrol_chip(kcontrol);
- for (i = 2; i--;) + for (i = 0; i < 2; i++) ucontrol->value.integer.value[i] = line6pcm->volume_playback[i];
return 0; @@ -286,7 +286,7 @@ static int snd_line6_control_playback_put(struct snd_kcontrol *kcontrol, int i, changed = 0; struct snd_line6_pcm *line6pcm = snd_kcontrol_chip(kcontrol);
- for (i = 2; i--;) + for (i = 0; i < 2; i++) if (line6pcm->volume_playback[i] != ucontrol->value.integer.value[i]) { line6pcm->volume_playback[i] = @@ -330,7 +330,7 @@ static void line6_cleanup_pcm(struct snd_pcm *pcm) int i; struct snd_line6_pcm *line6pcm = snd_pcm_chip(pcm);
- for (i = LINE6_ISO_BUFFERS; i--;) { + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { if (line6pcm->urb_audio_out[i]) { usb_kill_urb(line6pcm->urb_audio_out[i]); usb_free_urb(line6pcm->urb_audio_out[i]); diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index 1c9f95a370ff..ab9a83f0f864 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -297,7 +297,7 @@ void line6_unlink_audio_out_urbs(struct snd_line6_pcm *line6pcm) { unsigned int i;
- for (i = LINE6_ISO_BUFFERS; i--;) { + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { if (test_bit(i, &line6pcm->active_urb_out)) { if (!test_and_set_bit(i, &line6pcm->unlink_urb_out)) { struct urb *u = line6pcm->urb_audio_out[i]; @@ -320,7 +320,7 @@ void line6_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm)
do { alive = 0; - for (i = LINE6_ISO_BUFFERS; i--;) { + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { if (test_bit(i, &line6pcm->active_urb_out)) alive++; } @@ -366,14 +366,14 @@ static void audio_out_callback(struct urb *urb) line6pcm->last_frame_out = urb->start_frame;
/* find index of URB */ - for (index = LINE6_ISO_BUFFERS; index--;) + for (index = 0; index < LINE6_ISO_BUFFERS; index++) if (urb == line6pcm->urb_audio_out[index]) break;
- if (index < 0) + if (index >= LINE6_ISO_BUFFERS) return; /* URB has been unlinked asynchronously */
- for (i = LINE6_ISO_PACKETS; i--;) + for (i = 0; i < LINE6_ISO_PACKETS; i++) length += urb->iso_frame_desc[i].length;
spin_lock_irqsave(&line6pcm->lock_audio_out, flags); @@ -390,7 +390,7 @@ static void audio_out_callback(struct urb *urb)
clear_bit(index, &line6pcm->active_urb_out);
- for (i = LINE6_ISO_PACKETS; i--;) + for (i = 0; i < LINE6_ISO_PACKETS; i++) if (urb->iso_frame_desc[i].status == -EXDEV) { shutdown = 1; break;
If the problem still really remains, we should fix it instead of papering over it like this...
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/capture.c | 12 ------------ sound/usb/line6/playback.c | 12 ------------ 2 files changed, 24 deletions(-)
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c index 97283e631e45..bad1b5b02786 100644 --- a/sound/usb/line6/capture.c +++ b/sound/usb/line6/capture.c @@ -297,18 +297,6 @@ static int snd_line6_capture_hw_params(struct snd_pcm_substream *substream, int ret; struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
- /* -- Florian Demski [FD] */ - /* don't ask me why, but this fixes the bug on my machine */ - if (line6pcm == NULL) { - if (substream->pcm == NULL) - return -ENOMEM; - if (substream->pcm->private_data == NULL) - return -ENOMEM; - substream->private_data = substream->pcm->private_data; - line6pcm = snd_pcm_substream_chip(substream); - } - /* -- [FD] end */ - ret = line6_pcm_acquire(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_BUFFER);
if (ret < 0) diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index ab9a83f0f864..7e031b1761aa 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -445,18 +445,6 @@ static int snd_line6_playback_hw_params(struct snd_pcm_substream *substream, int ret; struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
- /* -- Florian Demski [FD] */ - /* don't ask me why, but this fixes the bug on my machine */ - if (line6pcm == NULL) { - if (substream->pcm == NULL) - return -ENOMEM; - if (substream->pcm->private_data == NULL) - return -ENOMEM; - substream->private_data = substream->pcm->private_data; - line6pcm = snd_pcm_substream_chip(substream); - } - /* -- [FD] end */ - ret = line6_pcm_acquire(line6pcm, LINE6_BIT_PCM_ALSA_PLAYBACK_BUFFER);
if (ret < 0)
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@suse.de wrote:
If the problem still really remains, we should fix it instead of papering over it like this...
FWIW, the commit from the original repository was made in 2007. So if the bug is still there, it's an old one.
commit 1e32d598dcf290e5c131a58231c13bfd25812810 Author: fdemski fdemski@87a33c3d-1c25-0410-a0e4-a977fe0255b3 Date: Mon Jul 16 23:51:55 2007 +0000
bug found and fixed -- maybe some alsa specifics -- did not crosscheck substream->private_data seems not to be filled in hw_param function
Introduce a new line6_pcm_stream structure and group individual fields of snd_line6_pcm struct to playback and capture groups.
This patch itself just does rename and nothing else. More meaningful cleanups based on these fields shuffling will follow.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/capture.c | 64 +++++++++--------- sound/usb/line6/pcm.c | 50 +++++++------- sound/usb/line6/pcm.h | 158 +++++++++++++-------------------------------- sound/usb/line6/playback.c | 78 +++++++++++----------- 4 files changed, 142 insertions(+), 208 deletions(-)
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c index bad1b5b02786..439f1941eb56 100644 --- a/sound/usb/line6/capture.c +++ b/sound/usb/line6/capture.c @@ -29,17 +29,17 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm) int ret; struct urb *urb_in;
- spin_lock_irqsave(&line6pcm->lock_audio_in, flags); + spin_lock_irqsave(&line6pcm->in.lock, flags); index = - find_first_zero_bit(&line6pcm->active_urb_in, LINE6_ISO_BUFFERS); + find_first_zero_bit(&line6pcm->in.active_urbs, LINE6_ISO_BUFFERS);
if (index < 0 || index >= LINE6_ISO_BUFFERS) { - spin_unlock_irqrestore(&line6pcm->lock_audio_in, flags); + spin_unlock_irqrestore(&line6pcm->in.lock, flags); dev_err(line6pcm->line6->ifcdev, "no free URB found\n"); return -EINVAL; }
- urb_in = line6pcm->urb_audio_in[index]; + urb_in = line6pcm->in.urbs[index]; urb_size = 0;
for (i = 0; i < LINE6_ISO_PACKETS; ++i) { @@ -51,7 +51,7 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm) }
urb_in->transfer_buffer = - line6pcm->buffer_in + + line6pcm->in.buffer + index * LINE6_ISO_PACKETS * line6pcm->max_packet_size; urb_in->transfer_buffer_length = urb_size; urb_in->context = line6pcm; @@ -59,12 +59,12 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm) ret = usb_submit_urb(urb_in, GFP_ATOMIC);
if (ret == 0) - set_bit(index, &line6pcm->active_urb_in); + set_bit(index, &line6pcm->in.active_urbs); else dev_err(line6pcm->line6->ifcdev, "URB in #%d submission failed (%d)\n", index, ret);
- spin_unlock_irqrestore(&line6pcm->lock_audio_in, flags); + spin_unlock_irqrestore(&line6pcm->in.lock, flags); return 0; }
@@ -92,9 +92,9 @@ void line6_unlink_audio_in_urbs(struct snd_line6_pcm *line6pcm) unsigned int i;
for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->active_urb_in)) { - if (!test_and_set_bit(i, &line6pcm->unlink_urb_in)) { - struct urb *u = line6pcm->urb_audio_in[i]; + if (test_bit(i, &line6pcm->in.active_urbs)) { + if (!test_and_set_bit(i, &line6pcm->in.unlink_urbs)) { + struct urb *u = line6pcm->in.urbs[i];
usb_unlink_urb(u); } @@ -115,7 +115,7 @@ void line6_wait_clear_audio_in_urbs(struct snd_line6_pcm *line6pcm) do { alive = 0; for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->active_urb_in)) + if (test_bit(i, &line6pcm->in.active_urbs)) alive++; } if (!alive) @@ -150,18 +150,18 @@ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize) if (runtime == NULL) return;
- if (line6pcm->pos_in_done + frames > runtime->buffer_size) { + if (line6pcm->in.pos_done + frames > runtime->buffer_size) { /* The transferred area goes over buffer boundary, copy two separate chunks. */ int len;
- len = runtime->buffer_size - line6pcm->pos_in_done; + len = runtime->buffer_size - line6pcm->in.pos_done;
if (len > 0) { memcpy(runtime->dma_area + - line6pcm->pos_in_done * bytes_per_frame, fbuf, + line6pcm->in.pos_done * bytes_per_frame, fbuf, len * bytes_per_frame); memcpy(runtime->dma_area, fbuf + len * bytes_per_frame, (frames - len) * bytes_per_frame); @@ -173,12 +173,12 @@ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize) } else { /* copy single chunk */ memcpy(runtime->dma_area + - line6pcm->pos_in_done * bytes_per_frame, fbuf, fsize); + line6pcm->in.pos_done * bytes_per_frame, fbuf, fsize); }
- line6pcm->pos_in_done += frames; - if (line6pcm->pos_in_done >= runtime->buffer_size) - line6pcm->pos_in_done -= runtime->buffer_size; + line6pcm->in.pos_done += frames; + if (line6pcm->in.pos_done >= runtime->buffer_size) + line6pcm->in.pos_done -= runtime->buffer_size; }
void line6_capture_check_period(struct snd_line6_pcm *line6pcm, int length) @@ -186,17 +186,17 @@ void line6_capture_check_period(struct snd_line6_pcm *line6pcm, int length) struct snd_pcm_substream *substream = get_substream(line6pcm, SNDRV_PCM_STREAM_CAPTURE);
- line6pcm->bytes_in += length; - if (line6pcm->bytes_in >= line6pcm->period_in) { - line6pcm->bytes_in %= line6pcm->period_in; + line6pcm->in.bytes += length; + if (line6pcm->in.bytes >= line6pcm->in.period) { + line6pcm->in.bytes %= line6pcm->in.period; snd_pcm_period_elapsed(substream); } }
void line6_free_capture_buffer(struct snd_line6_pcm *line6pcm) { - kfree(line6pcm->buffer_in); - line6pcm->buffer_in = NULL; + kfree(line6pcm->in.buffer); + line6pcm->in.buffer = NULL; }
/* @@ -209,14 +209,14 @@ static void audio_in_callback(struct urb *urb)
struct snd_line6_pcm *line6pcm = (struct snd_line6_pcm *)urb->context;
- line6pcm->last_frame_in = urb->start_frame; + line6pcm->in.last_frame = urb->start_frame;
/* find index of URB */ for (index = 0; index < LINE6_ISO_BUFFERS; ++index) - if (urb == line6pcm->urb_audio_in[index]) + if (urb == line6pcm->in.urbs[index]) break;
- spin_lock_irqsave(&line6pcm->lock_audio_in, flags); + spin_lock_irqsave(&line6pcm->in.lock, flags);
for (i = 0; i < LINE6_ISO_PACKETS; ++i) { char *fbuf; @@ -249,12 +249,12 @@ static void audio_in_callback(struct urb *urb) line6_capture_copy(line6pcm, fbuf, fsize); }
- clear_bit(index, &line6pcm->active_urb_in); + clear_bit(index, &line6pcm->in.active_urbs);
- if (test_and_clear_bit(index, &line6pcm->unlink_urb_in)) + if (test_and_clear_bit(index, &line6pcm->in.unlink_urbs)) shutdown = 1;
- spin_unlock_irqrestore(&line6pcm->lock_audio_in, flags); + spin_unlock_irqrestore(&line6pcm->in.lock, flags);
if (!shutdown) { submit_audio_in_urb(line6pcm); @@ -309,7 +309,7 @@ static int snd_line6_capture_hw_params(struct snd_pcm_substream *substream, return ret; }
- line6pcm->period_in = params_period_bytes(hw_params); + line6pcm->in.period = params_period_bytes(hw_params); return 0; }
@@ -361,7 +361,7 @@ snd_line6_capture_pointer(struct snd_pcm_substream *substream) { struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
- return line6pcm->pos_in_done; + return line6pcm->in.pos_done; }
/* capture operators */ @@ -386,7 +386,7 @@ int line6_create_audio_in_urbs(struct snd_line6_pcm *line6pcm) struct urb *urb;
/* URB for audio in: */ - urb = line6pcm->urb_audio_in[i] = + urb = line6pcm->in.urbs[i] = usb_alloc_urb(LINE6_ISO_PACKETS, GFP_KERNEL);
if (urb == NULL) diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 43474c4ebb6b..738bfd82cecd 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -112,11 +112,11 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels)
if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_BUFFER)) { /* Invoked multiple times in a row so allocate once only */ - if (!line6pcm->buffer_in) { - line6pcm->buffer_in = + if (!line6pcm->in.buffer) { + line6pcm->in.buffer = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS * line6pcm->max_packet_size, GFP_KERNEL); - if (!line6pcm->buffer_in) { + if (!line6pcm->in.buffer) { err = -ENOMEM; goto pcm_acquire_error; } @@ -131,13 +131,13 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) a bug, we therefore report an error if capturing is restarted too soon. */ - if (line6pcm->active_urb_in || line6pcm->unlink_urb_in) { + if (line6pcm->in.active_urbs || line6pcm->in.unlink_urbs) { dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n"); err = -EBUSY; goto pcm_acquire_error; }
- line6pcm->count_in = 0; + line6pcm->in.count = 0; line6pcm->prev_fsize = 0; err = line6_submit_audio_in_all_urbs(line6pcm);
@@ -149,11 +149,11 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels)
if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_BUFFER)) { /* Invoked multiple times in a row so allocate once only */ - if (!line6pcm->buffer_out) { - line6pcm->buffer_out = + if (!line6pcm->out.buffer) { + line6pcm->out.buffer = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS * line6pcm->max_packet_size, GFP_KERNEL); - if (!line6pcm->buffer_out) { + if (!line6pcm->out.buffer) { err = -ENOMEM; goto pcm_acquire_error; } @@ -166,12 +166,12 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) /* See comment above regarding PCM restart. */ - if (line6pcm->active_urb_out || line6pcm->unlink_urb_out) { + if (line6pcm->out.active_urbs || line6pcm->out.unlink_urbs) { dev_err(line6pcm->line6->ifcdev, "Device not yet ready\n"); return -EBUSY; }
- line6pcm->count_out = 0; + line6pcm->out.count = 0; err = line6_submit_audio_out_all_urbs(line6pcm);
if (err < 0) @@ -331,13 +331,13 @@ static void line6_cleanup_pcm(struct snd_pcm *pcm) struct snd_line6_pcm *line6pcm = snd_pcm_chip(pcm);
for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (line6pcm->urb_audio_out[i]) { - usb_kill_urb(line6pcm->urb_audio_out[i]); - usb_free_urb(line6pcm->urb_audio_out[i]); + if (line6pcm->out.urbs[i]) { + usb_kill_urb(line6pcm->out.urbs[i]); + usb_free_urb(line6pcm->out.urbs[i]); } - if (line6pcm->urb_audio_in[i]) { - usb_kill_urb(line6pcm->urb_audio_in[i]); - usb_free_urb(line6pcm->urb_audio_in[i]); + if (line6pcm->in.urbs[i]) { + usb_kill_urb(line6pcm->in.urbs[i]); + usb_free_urb(line6pcm->in.urbs[i]); } } kfree(line6pcm); @@ -415,8 +415,8 @@ int line6_init_pcm(struct usb_line6 *line6, usb_maxpacket(line6->usbdev, usb_sndisocpipe(line6->usbdev, ep_write), 1));
- spin_lock_init(&line6pcm->lock_audio_out); - spin_lock_init(&line6pcm->lock_audio_in); + spin_lock_init(&line6pcm->out.lock); + spin_lock_init(&line6pcm->in.lock); line6pcm->impulse_period = LINE6_IMPULSE_DEFAULT_PERIOD;
line6->line6pcm = line6pcm; @@ -464,13 +464,13 @@ int snd_line6_prepare(struct snd_pcm_substream *substream) }
if (!test_and_set_bit(LINE6_INDEX_PREPARED, &line6pcm->flags)) { - line6pcm->count_out = 0; - line6pcm->pos_out = 0; - line6pcm->pos_out_done = 0; - line6pcm->bytes_out = 0; - line6pcm->count_in = 0; - line6pcm->pos_in_done = 0; - line6pcm->bytes_in = 0; + line6pcm->out.count = 0; + line6pcm->out.pos = 0; + line6pcm->out.pos_done = 0; + line6pcm->out.bytes = 0; + line6pcm->in.count = 0; + line6pcm->in.pos_done = 0; + line6pcm->in.bytes = 0; }
return 0; diff --git a/sound/usb/line6/pcm.h b/sound/usb/line6/pcm.h index a84753ee0fa2..4c74f4e85074 100644 --- a/sound/usb/line6/pcm.h +++ b/sound/usb/line6/pcm.h @@ -165,115 +165,78 @@ struct line6_pcm_properties { int bytes_per_frame; };
-struct snd_line6_pcm { - /** - Pointer back to the Line 6 driver data structure. - */ - struct usb_line6 *line6; +struct line6_pcm_stream { + /* allocated URBs */ + struct urb *urbs[LINE6_ISO_BUFFERS];
- /** - Properties. - */ - struct line6_pcm_properties *properties; + /* Temporary buffer; + * Since the packet size is not known in advance, this buffer is + * large enough to store maximum size packets. + */ + unsigned char *buffer;
- /** - ALSA pcm stream - */ - struct snd_pcm *pcm; + /* Free frame position in the buffer. */ + snd_pcm_uframes_t pos;
- /** - URBs for audio playback. - */ - struct urb *urb_audio_out[LINE6_ISO_BUFFERS]; + /* Count processed bytes; + * This is modulo period size (to determine when a period is finished). + */ + unsigned bytes;
- /** - URBs for audio capture. - */ - struct urb *urb_audio_in[LINE6_ISO_BUFFERS]; + /* Counter to create desired sample rate */ + unsigned count;
- /** - Temporary buffer for playback. - Since the packet size is not known in advance, this buffer is - large enough to store maximum size packets. - */ - unsigned char *buffer_out; + /* period size in bytes */ + unsigned period;
- /** - Temporary buffer for capture. - Since the packet size is not known in advance, this buffer is - large enough to store maximum size packets. - */ - unsigned char *buffer_in; + /* Processed frame position in the buffer; + * The contents of the ring buffer have been consumed by the USB + * subsystem (i.e., sent to the USB device) up to this position. + */ + snd_pcm_uframes_t pos_done;
- /** - Previously captured frame (for software monitoring). - */ - unsigned char *prev_fbuf; + /* Bit mask of active URBs */ + unsigned long active_urbs;
- /** - Size of previously captured frame (for software monitoring). - */ - int prev_fsize; - - /** - Free frame position in the playback buffer. - */ - snd_pcm_uframes_t pos_out; + /* Bit mask of URBs currently being unlinked */ + unsigned long unlink_urbs;
- /** - Count processed bytes for playback. - This is modulo period size (to determine when a period is - finished). - */ - unsigned bytes_out; + /* Spin lock to protect updates of the buffer positions (not contents) + */ + spinlock_t lock;
- /** - Counter to create desired playback sample rate. - */ - unsigned count_out; - - /** - Playback period size in bytes - */ - unsigned period_out; + int last_frame; +};
+struct snd_line6_pcm { /** - Processed frame position in the playback buffer. - The contents of the output ring buffer have been consumed by - the USB subsystem (i.e., sent to the USB device) up to this - position. + Pointer back to the Line 6 driver data structure. */ - snd_pcm_uframes_t pos_out_done; + struct usb_line6 *line6;
/** - Count processed bytes for capture. - This is modulo period size (to determine when a period is - finished). + Properties. */ - unsigned bytes_in; + struct line6_pcm_properties *properties;
/** - Counter to create desired capture sample rate. + ALSA pcm stream */ - unsigned count_in; + struct snd_pcm *pcm;
- /** - Capture period size in bytes - */ - unsigned period_in; + /* Capture and playback streams */ + struct line6_pcm_stream in; + struct line6_pcm_stream out;
/** - Processed frame position in the capture buffer. - The contents of the output ring buffer have been consumed by - the USB subsystem (i.e., sent to the USB device) up to this - position. + Previously captured frame (for software monitoring). */ - snd_pcm_uframes_t pos_in_done; + unsigned char *prev_fbuf;
/** - Bit mask of active playback URBs. + Size of previously captured frame (for software monitoring). */ - unsigned long active_urb_out; + int prev_fsize;
/** Maximum size of USB packet. @@ -281,33 +244,6 @@ struct snd_line6_pcm { int max_packet_size;
/** - Bit mask of active capture URBs. - */ - unsigned long active_urb_in; - - /** - Bit mask of playback URBs currently being unlinked. - */ - unsigned long unlink_urb_out; - - /** - Bit mask of capture URBs currently being unlinked. - */ - unsigned long unlink_urb_in; - - /** - Spin lock to protect updates of the playback buffer positions (not - contents!) - */ - spinlock_t lock_audio_out; - - /** - Spin lock to protect updates of the capture buffer positions (not - contents!) - */ - spinlock_t lock_audio_in; - - /** PCM playback volume (left and right). */ int volume_playback[2]; @@ -336,8 +272,6 @@ struct snd_line6_pcm { Several status bits (see LINE6_BIT_*). */ unsigned long flags; - - int last_frame_in, last_frame_out; };
extern int line6_init_pcm(struct usb_line6 *line6, diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index 7e031b1761aa..d619c1718306 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -145,17 +145,17 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) (USB_INTERVALS_PER_SECOND / LINE6_ISO_INTERVAL); struct urb *urb_out;
- spin_lock_irqsave(&line6pcm->lock_audio_out, flags); + spin_lock_irqsave(&line6pcm->out.lock, flags); index = - find_first_zero_bit(&line6pcm->active_urb_out, LINE6_ISO_BUFFERS); + find_first_zero_bit(&line6pcm->out.active_urbs, LINE6_ISO_BUFFERS);
if (index < 0 || index >= LINE6_ISO_BUFFERS) { - spin_unlock_irqrestore(&line6pcm->lock_audio_out, flags); + spin_unlock_irqrestore(&line6pcm->out.lock, flags); dev_err(line6pcm->line6->ifcdev, "no free URB found\n"); return -EINVAL; }
- urb_out = line6pcm->urb_audio_out[index]; + urb_out = line6pcm->out.urbs[index]; urb_size = 0;
for (i = 0; i < LINE6_ISO_PACKETS; ++i) { @@ -170,9 +170,9 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) if (fsize == 0) { int n;
- line6pcm->count_out += frame_increment; - n = line6pcm->count_out / frame_factor; - line6pcm->count_out -= n * frame_factor; + line6pcm->out.count += frame_increment; + n = line6pcm->out.count / frame_factor; + line6pcm->out.count -= n * frame_factor; fsize = n * bytes_per_frame; }
@@ -183,14 +183,14 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
if (urb_size == 0) { /* can't determine URB size */ - spin_unlock_irqrestore(&line6pcm->lock_audio_out, flags); + spin_unlock_irqrestore(&line6pcm->out.lock, flags); dev_err(line6pcm->line6->ifcdev, "driver bug: urb_size = 0\n"); return -EINVAL; }
urb_frames = urb_size / bytes_per_frame; urb_out->transfer_buffer = - line6pcm->buffer_out + + line6pcm->out.buffer + index * LINE6_ISO_PACKETS * line6pcm->max_packet_size; urb_out->transfer_buffer_length = urb_size; urb_out->context = line6pcm; @@ -200,19 +200,19 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) struct snd_pcm_runtime *runtime = get_substream(line6pcm, SNDRV_PCM_STREAM_PLAYBACK)->runtime;
- if (line6pcm->pos_out + urb_frames > runtime->buffer_size) { + if (line6pcm->out.pos + urb_frames > runtime->buffer_size) { /* The transferred area goes over buffer boundary, copy the data to the temp buffer. */ int len;
- len = runtime->buffer_size - line6pcm->pos_out; + len = runtime->buffer_size - line6pcm->out.pos;
if (len > 0) { memcpy(urb_out->transfer_buffer, runtime->dma_area + - line6pcm->pos_out * bytes_per_frame, + line6pcm->out.pos * bytes_per_frame, len * bytes_per_frame); memcpy(urb_out->transfer_buffer + len * bytes_per_frame, runtime->dma_area, @@ -223,13 +223,13 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) } else { memcpy(urb_out->transfer_buffer, runtime->dma_area + - line6pcm->pos_out * bytes_per_frame, + line6pcm->out.pos * bytes_per_frame, urb_out->transfer_buffer_length); }
- line6pcm->pos_out += urb_frames; - if (line6pcm->pos_out >= runtime->buffer_size) - line6pcm->pos_out -= runtime->buffer_size; + line6pcm->out.pos += urb_frames; + if (line6pcm->out.pos >= runtime->buffer_size) + line6pcm->out.pos -= runtime->buffer_size; } else { memset(urb_out->transfer_buffer, 0, urb_out->transfer_buffer_length); @@ -265,12 +265,12 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) ret = usb_submit_urb(urb_out, GFP_ATOMIC);
if (ret == 0) - set_bit(index, &line6pcm->active_urb_out); + set_bit(index, &line6pcm->out.active_urbs); else dev_err(line6pcm->line6->ifcdev, "URB out #%d submission failed (%d)\n", index, ret);
- spin_unlock_irqrestore(&line6pcm->lock_audio_out, flags); + spin_unlock_irqrestore(&line6pcm->out.lock, flags); return 0; }
@@ -298,9 +298,9 @@ void line6_unlink_audio_out_urbs(struct snd_line6_pcm *line6pcm) unsigned int i;
for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->active_urb_out)) { - if (!test_and_set_bit(i, &line6pcm->unlink_urb_out)) { - struct urb *u = line6pcm->urb_audio_out[i]; + if (test_bit(i, &line6pcm->out.active_urbs)) { + if (!test_and_set_bit(i, &line6pcm->out.unlink_urbs)) { + struct urb *u = line6pcm->out.urbs[i];
usb_unlink_urb(u); } @@ -321,7 +321,7 @@ void line6_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm) do { alive = 0; for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->active_urb_out)) + if (test_bit(i, &line6pcm->out.active_urbs)) alive++; } if (!alive) @@ -344,8 +344,8 @@ void line6_unlink_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm)
void line6_free_playback_buffer(struct snd_line6_pcm *line6pcm) { - kfree(line6pcm->buffer_out); - line6pcm->buffer_out = NULL; + kfree(line6pcm->out.buffer); + line6pcm->out.buffer = NULL; }
/* @@ -363,11 +363,11 @@ static void audio_out_callback(struct urb *urb) memset(urb->transfer_buffer, 0, urb->transfer_buffer_length); #endif
- line6pcm->last_frame_out = urb->start_frame; + line6pcm->out.last_frame = urb->start_frame;
/* find index of URB */ for (index = 0; index < LINE6_ISO_BUFFERS; index++) - if (urb == line6pcm->urb_audio_out[index]) + if (urb == line6pcm->out.urbs[index]) break;
if (index >= LINE6_ISO_BUFFERS) @@ -376,19 +376,19 @@ static void audio_out_callback(struct urb *urb) for (i = 0; i < LINE6_ISO_PACKETS; i++) length += urb->iso_frame_desc[i].length;
- spin_lock_irqsave(&line6pcm->lock_audio_out, flags); + spin_lock_irqsave(&line6pcm->out.lock, flags);
if (test_bit(LINE6_INDEX_PCM_ALSA_PLAYBACK_STREAM, &line6pcm->flags)) { struct snd_pcm_runtime *runtime = substream->runtime;
- line6pcm->pos_out_done += + line6pcm->out.pos_done += length / line6pcm->properties->bytes_per_frame;
- if (line6pcm->pos_out_done >= runtime->buffer_size) - line6pcm->pos_out_done -= runtime->buffer_size; + if (line6pcm->out.pos_done >= runtime->buffer_size) + line6pcm->out.pos_done -= runtime->buffer_size; }
- clear_bit(index, &line6pcm->active_urb_out); + clear_bit(index, &line6pcm->out.active_urbs);
for (i = 0; i < LINE6_ISO_PACKETS; i++) if (urb->iso_frame_desc[i].status == -EXDEV) { @@ -396,19 +396,19 @@ static void audio_out_callback(struct urb *urb) break; }
- if (test_and_clear_bit(index, &line6pcm->unlink_urb_out)) + if (test_and_clear_bit(index, &line6pcm->out.unlink_urbs)) shutdown = 1;
- spin_unlock_irqrestore(&line6pcm->lock_audio_out, flags); + spin_unlock_irqrestore(&line6pcm->out.lock, flags);
if (!shutdown) { submit_audio_out_urb(line6pcm);
if (test_bit(LINE6_INDEX_PCM_ALSA_PLAYBACK_STREAM, &line6pcm->flags)) { - line6pcm->bytes_out += length; - if (line6pcm->bytes_out >= line6pcm->period_out) { - line6pcm->bytes_out %= line6pcm->period_out; + line6pcm->out.bytes += length; + if (line6pcm->out.bytes >= line6pcm->out.period) { + line6pcm->out.bytes %= line6pcm->out.period; snd_pcm_period_elapsed(substream); } } @@ -457,7 +457,7 @@ static int snd_line6_playback_hw_params(struct snd_pcm_substream *substream, return ret; }
- line6pcm->period_out = params_period_bytes(hw_params); + line6pcm->out.period = params_period_bytes(hw_params); return 0; }
@@ -517,7 +517,7 @@ snd_line6_playback_pointer(struct snd_pcm_substream *substream) { struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream);
- return line6pcm->pos_out_done; + return line6pcm->out.pos_done; }
/* playback operators */ @@ -542,7 +542,7 @@ int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm) struct urb *urb;
/* URB for audio out: */ - urb = line6pcm->urb_audio_out[i] = + urb = line6pcm->out.urbs[i] = usb_alloc_urb(LINE6_ISO_PACKETS, GFP_KERNEL);
if (urb == NULL)
The codes to unlink and sync URBs are identical for both playback and capture streams. Consolidate to single helper functions.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/capture.c | 52 --------------------------- sound/usb/line6/capture.h | 4 --- sound/usb/line6/pcm.c | 90 +++++++++++++++++++++++++++++++++++----------- sound/usb/line6/playback.c | 52 --------------------------- sound/usb/line6/playback.h | 4 --- 5 files changed, 69 insertions(+), 133 deletions(-)
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c index 439f1941eb56..1d477d7a42fb 100644 --- a/sound/usb/line6/capture.c +++ b/sound/usb/line6/capture.c @@ -85,58 +85,6 @@ int line6_submit_audio_in_all_urbs(struct snd_line6_pcm *line6pcm) }
/* - Unlink all currently active capture URBs. -*/ -void line6_unlink_audio_in_urbs(struct snd_line6_pcm *line6pcm) -{ - unsigned int i; - - for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->in.active_urbs)) { - if (!test_and_set_bit(i, &line6pcm->in.unlink_urbs)) { - struct urb *u = line6pcm->in.urbs[i]; - - usb_unlink_urb(u); - } - } - } -} - -/* - Wait until unlinking of all currently active capture URBs has been - finished. -*/ -void line6_wait_clear_audio_in_urbs(struct snd_line6_pcm *line6pcm) -{ - int timeout = HZ; - unsigned int i; - int alive; - - do { - alive = 0; - for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->in.active_urbs)) - alive++; - } - if (!alive) - break; - set_current_state(TASK_UNINTERRUPTIBLE); - schedule_timeout(1); - } while (--timeout > 0); - if (alive) - snd_printk(KERN_ERR "timeout: still %d active urbs..\n", alive); -} - -/* - Unlink all currently active capture URBs, and wait for finishing. -*/ -void line6_unlink_wait_clear_audio_in_urbs(struct snd_line6_pcm *line6pcm) -{ - line6_unlink_audio_in_urbs(line6pcm); - line6_wait_clear_audio_in_urbs(line6pcm); -} - -/* Copy data into ALSA capture buffer. */ void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, int fsize) diff --git a/sound/usb/line6/capture.h b/sound/usb/line6/capture.h index 0939f400a405..3cc71bc70b21 100644 --- a/sound/usb/line6/capture.h +++ b/sound/usb/line6/capture.h @@ -26,10 +26,6 @@ extern void line6_capture_check_period(struct snd_line6_pcm *line6pcm, extern int line6_create_audio_in_urbs(struct snd_line6_pcm *line6pcm); extern void line6_free_capture_buffer(struct snd_line6_pcm *line6pcm); extern int line6_submit_audio_in_all_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_unlink_audio_in_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_unlink_wait_clear_audio_in_urbs(struct snd_line6_pcm - *line6pcm); -extern void line6_wait_clear_audio_in_urbs(struct snd_line6_pcm *line6pcm); extern int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd);
#endif diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 738bfd82cecd..677419dcacf9 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -90,6 +90,47 @@ static int snd_line6_impulse_period_put(struct snd_kcontrol *kcontrol, return 1; }
+/* + Unlink all currently active URBs. +*/ +static void line6_unlink_audio_urbs(struct snd_line6_pcm *line6pcm, + struct line6_pcm_stream *pcms) +{ + int i; + + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { + if (test_bit(i, &pcms->active_urbs)) { + if (!test_and_set_bit(i, &pcms->unlink_urbs)) + usb_unlink_urb(pcms->urbs[i]); + } + } +} + +/* + Wait until unlinking of all currently active URBs has been finished. +*/ +static void line6_wait_clear_audio_urbs(struct snd_line6_pcm *line6pcm, + struct line6_pcm_stream *pcms) +{ + int timeout = HZ; + int i; + int alive; + + do { + alive = 0; + for (i = 0; i < LINE6_ISO_BUFFERS; i++) { + if (test_bit(i, &pcms->active_urbs)) + alive++; + } + if (!alive) + break; + set_current_state(TASK_UNINTERRUPTIBLE); + schedule_timeout(1); + } while (--timeout > 0); + if (alive) + snd_printk(KERN_ERR "timeout: still %d active urbs..\n", alive); +} + static bool test_flags(unsigned long flags0, unsigned long flags1, unsigned long mask) { @@ -202,18 +243,18 @@ int line6_pcm_release(struct snd_line6_pcm *line6pcm, int channels) } while (cmpxchg(&line6pcm->flags, flags_old, flags_new) != flags_old);
if (test_flags(flags_new, flags_old, LINE6_BITS_CAPTURE_STREAM)) - line6_unlink_audio_in_urbs(line6pcm); + line6_unlink_audio_urbs(line6pcm, &line6pcm->in);
if (test_flags(flags_new, flags_old, LINE6_BITS_CAPTURE_BUFFER)) { - line6_wait_clear_audio_in_urbs(line6pcm); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in); line6_free_capture_buffer(line6pcm); }
if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_STREAM)) - line6_unlink_audio_out_urbs(line6pcm); + line6_unlink_audio_urbs(line6pcm, &line6pcm->out);
if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_BUFFER)) { - line6_wait_clear_audio_out_urbs(line6pcm); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out); line6_free_playback_buffer(line6pcm); }
@@ -325,21 +366,24 @@ static struct snd_kcontrol_new line6_controls[] = { /* Cleanup the PCM device. */ -static void line6_cleanup_pcm(struct snd_pcm *pcm) +static void cleanup_urbs(struct line6_pcm_stream *pcms) { int i; - struct snd_line6_pcm *line6pcm = snd_pcm_chip(pcm);
for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (line6pcm->out.urbs[i]) { - usb_kill_urb(line6pcm->out.urbs[i]); - usb_free_urb(line6pcm->out.urbs[i]); - } - if (line6pcm->in.urbs[i]) { - usb_kill_urb(line6pcm->in.urbs[i]); - usb_free_urb(line6pcm->in.urbs[i]); + if (pcms->urbs[i]) { + usb_kill_urb(pcms->urbs[i]); + usb_free_urb(pcms->urbs[i]); } } +} + +static void line6_cleanup_pcm(struct snd_pcm *pcm) +{ + struct snd_line6_pcm *line6pcm = snd_pcm_chip(pcm); + + cleanup_urbs(&line6pcm->out); + cleanup_urbs(&line6pcm->in); kfree(line6pcm); }
@@ -374,8 +418,10 @@ static int snd_line6_new_pcm(struct usb_line6 *line6, struct snd_pcm **pcm_ret) */ void line6_pcm_disconnect(struct snd_line6_pcm *line6pcm) { - line6_unlink_wait_clear_audio_out_urbs(line6pcm); - line6_unlink_wait_clear_audio_in_urbs(line6pcm); + line6_unlink_audio_urbs(line6pcm, &line6pcm->out); + line6_unlink_audio_urbs(line6pcm, &line6pcm->in); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in); }
/* @@ -451,15 +497,17 @@ int snd_line6_prepare(struct snd_pcm_substream *substream)
switch (substream->stream) { case SNDRV_PCM_STREAM_PLAYBACK: - if ((line6pcm->flags & LINE6_BITS_PLAYBACK_STREAM) == 0) - line6_unlink_wait_clear_audio_out_urbs(line6pcm); - + if ((line6pcm->flags & LINE6_BITS_PLAYBACK_STREAM) == 0) { + line6_unlink_audio_urbs(line6pcm, &line6pcm->out); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out); + } break;
case SNDRV_PCM_STREAM_CAPTURE: - if ((line6pcm->flags & LINE6_BITS_CAPTURE_STREAM) == 0) - line6_unlink_wait_clear_audio_in_urbs(line6pcm); - + if ((line6pcm->flags & LINE6_BITS_CAPTURE_STREAM) == 0) { + line6_unlink_audio_urbs(line6pcm, &line6pcm->in); + line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in); + } break; }
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index d619c1718306..3820ed08b342 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -290,58 +290,6 @@ int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm) return 0; }
-/* - Unlink all currently active playback URBs. -*/ -void line6_unlink_audio_out_urbs(struct snd_line6_pcm *line6pcm) -{ - unsigned int i; - - for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->out.active_urbs)) { - if (!test_and_set_bit(i, &line6pcm->out.unlink_urbs)) { - struct urb *u = line6pcm->out.urbs[i]; - - usb_unlink_urb(u); - } - } - } -} - -/* - Wait until unlinking of all currently active playback URBs has been - finished. -*/ -void line6_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm) -{ - int timeout = HZ; - unsigned int i; - int alive; - - do { - alive = 0; - for (i = 0; i < LINE6_ISO_BUFFERS; i++) { - if (test_bit(i, &line6pcm->out.active_urbs)) - alive++; - } - if (!alive) - break; - set_current_state(TASK_UNINTERRUPTIBLE); - schedule_timeout(1); - } while (--timeout > 0); - if (alive) - snd_printk(KERN_ERR "timeout: still %d active urbs..\n", alive); -} - -/* - Unlink all currently active playback URBs, and wait for finishing. -*/ -void line6_unlink_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm) -{ - line6_unlink_audio_out_urbs(line6pcm); - line6_wait_clear_audio_out_urbs(line6pcm); -} - void line6_free_playback_buffer(struct snd_line6_pcm *line6pcm) { kfree(line6pcm->out.buffer); diff --git a/sound/usb/line6/playback.h b/sound/usb/line6/playback.h index 78a885113221..52a278353d3b 100644 --- a/sound/usb/line6/playback.h +++ b/sound/usb/line6/playback.h @@ -32,10 +32,6 @@ extern struct snd_pcm_ops snd_line6_playback_ops; extern int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm); extern void line6_free_playback_buffer(struct snd_line6_pcm *line6pcm); extern int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_unlink_audio_out_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_unlink_wait_clear_audio_out_urbs(struct snd_line6_pcm - *line6pcm); -extern void line6_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm); extern int snd_line6_playback_trigger(struct snd_line6_pcm *line6pcm, int cmd);
#endif
This is the last remaining snd_printk() usage in this driver.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 677419dcacf9..4152d92105b1 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -128,7 +128,8 @@ static void line6_wait_clear_audio_urbs(struct snd_line6_pcm *line6pcm, schedule_timeout(1); } while (--timeout > 0); if (alive) - snd_printk(KERN_ERR "timeout: still %d active urbs..\n", alive); + dev_err(line6pcm->line6->ifcdev, + "timeout: still %d active urbs..\n", alive); }
static bool test_flags(unsigned long flags0, unsigned long flags1,
The PCM stream buffer allocation and free are identical for both playback and capture streams. Provide single helper functions. These are used only in pcm.c, thus they can be even static.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/capture.c | 6 ------ sound/usb/line6/capture.h | 1 - sound/usb/line6/pcm.c | 53 +++++++++++++++++++++++++--------------------- sound/usb/line6/playback.c | 6 ------ sound/usb/line6/playback.h | 1 - 5 files changed, 29 insertions(+), 38 deletions(-)
diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c index 1d477d7a42fb..47cfcc2ab387 100644 --- a/sound/usb/line6/capture.c +++ b/sound/usb/line6/capture.c @@ -141,12 +141,6 @@ void line6_capture_check_period(struct snd_line6_pcm *line6pcm, int length) } }
-void line6_free_capture_buffer(struct snd_line6_pcm *line6pcm) -{ - kfree(line6pcm->in.buffer); - line6pcm->in.buffer = NULL; -} - /* * Callback for completed capture URB. */ diff --git a/sound/usb/line6/capture.h b/sound/usb/line6/capture.h index 3cc71bc70b21..db062e7200a6 100644 --- a/sound/usb/line6/capture.h +++ b/sound/usb/line6/capture.h @@ -24,7 +24,6 @@ extern void line6_capture_copy(struct snd_line6_pcm *line6pcm, char *fbuf, extern void line6_capture_check_period(struct snd_line6_pcm *line6pcm, int length); extern int line6_create_audio_in_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_free_capture_buffer(struct snd_line6_pcm *line6pcm); extern int line6_submit_audio_in_all_urbs(struct snd_line6_pcm *line6pcm); extern int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd);
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 4152d92105b1..f75825995e24 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -132,6 +132,27 @@ static void line6_wait_clear_audio_urbs(struct snd_line6_pcm *line6pcm, "timeout: still %d active urbs..\n", alive); }
+static int line6_alloc_stream_buffer(struct snd_line6_pcm *line6pcm, + struct line6_pcm_stream *pcms) +{ + /* Invoked multiple times in a row so allocate once only */ + if (pcms->buffer) + return 0; + + pcms->buffer = kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS * + line6pcm->max_packet_size, GFP_KERNEL); + if (!pcms->buffer) + return -ENOMEM; + return 0; +} + +static void line6_free_stream_buffer(struct snd_line6_pcm *line6pcm, + struct line6_pcm_stream *pcms) +{ + kfree(pcms->buffer); + pcms->buffer = NULL; +} + static bool test_flags(unsigned long flags0, unsigned long flags1, unsigned long mask) { @@ -153,17 +174,9 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) line6pcm->prev_fbuf = NULL;
if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_BUFFER)) { - /* Invoked multiple times in a row so allocate once only */ - if (!line6pcm->in.buffer) { - line6pcm->in.buffer = - kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS * - line6pcm->max_packet_size, GFP_KERNEL); - if (!line6pcm->in.buffer) { - err = -ENOMEM; - goto pcm_acquire_error; - } - } - + err = line6_alloc_stream_buffer(line6pcm, &line6pcm->in); + if (err < 0) + goto pcm_acquire_error; flags_final |= channels & LINE6_BITS_CAPTURE_BUFFER; }
@@ -190,17 +203,9 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels) }
if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_BUFFER)) { - /* Invoked multiple times in a row so allocate once only */ - if (!line6pcm->out.buffer) { - line6pcm->out.buffer = - kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS * - line6pcm->max_packet_size, GFP_KERNEL); - if (!line6pcm->out.buffer) { - err = -ENOMEM; - goto pcm_acquire_error; - } - } - + err = line6_alloc_stream_buffer(line6pcm, &line6pcm->out); + if (err < 0) + goto pcm_acquire_error; flags_final |= channels & LINE6_BITS_PLAYBACK_BUFFER; }
@@ -248,7 +253,7 @@ int line6_pcm_release(struct snd_line6_pcm *line6pcm, int channels)
if (test_flags(flags_new, flags_old, LINE6_BITS_CAPTURE_BUFFER)) { line6_wait_clear_audio_urbs(line6pcm, &line6pcm->in); - line6_free_capture_buffer(line6pcm); + line6_free_stream_buffer(line6pcm, &line6pcm->in); }
if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_STREAM)) @@ -256,7 +261,7 @@ int line6_pcm_release(struct snd_line6_pcm *line6pcm, int channels)
if (test_flags(flags_new, flags_old, LINE6_BITS_PLAYBACK_BUFFER)) { line6_wait_clear_audio_urbs(line6pcm, &line6pcm->out); - line6_free_playback_buffer(line6pcm); + line6_free_stream_buffer(line6pcm, &line6pcm->out); }
return 0; diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index 3820ed08b342..750d91dced57 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -290,12 +290,6 @@ int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm) return 0; }
-void line6_free_playback_buffer(struct snd_line6_pcm *line6pcm) -{ - kfree(line6pcm->out.buffer); - line6pcm->out.buffer = NULL; -} - /* Callback for completed playback URB. */ diff --git a/sound/usb/line6/playback.h b/sound/usb/line6/playback.h index 52a278353d3b..f6a9e18f87b6 100644 --- a/sound/usb/line6/playback.h +++ b/sound/usb/line6/playback.h @@ -30,7 +30,6 @@ extern struct snd_pcm_ops snd_line6_playback_ops;
extern int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm); -extern void line6_free_playback_buffer(struct snd_line6_pcm *line6pcm); extern int line6_submit_audio_out_all_urbs(struct snd_line6_pcm *line6pcm); extern int snd_line6_playback_trigger(struct snd_line6_pcm *line6pcm, int cmd);
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/playback.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index 750d91dced57..cbcd97f5d629 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -37,7 +37,8 @@ static void change_volume(struct urb *urb_out, int volume[], buf_end = p + urb_out->transfer_buffer_length / sizeof(*p);
for (; p < buf_end; ++p) { - *p = (*p * volume[chn & 1]) >> 8; + int val = (*p * volume[chn & 1]) >> 8; + *p = clamp(val, 0x7fff, -0x8000); ++chn; } } else if (bytes_per_frame == 6) { @@ -51,6 +52,7 @@ static void change_volume(struct urb *urb_out, int volume[],
val = p[0] + (p[1] << 8) + ((signed char)p[2] << 16); val = (val * volume[chn & 1]) >> 8; + val = clamp(val, 0x7fffff, -0x800000); p[0] = val; p[1] = val >> 8; p[2] = val >> 16; @@ -118,8 +120,10 @@ static void add_monitor_signal(struct urb *urb_out, unsigned char *signal, po = (short *)urb_out->transfer_buffer; buf_end = po + urb_out->transfer_buffer_length / sizeof(*po);
- for (; po < buf_end; ++pi, ++po) - *po += (*pi * volume) >> 8; + for (; po < buf_end; ++pi, ++po) { + int val = *po + ((*pi * volume) >> 8); + *po = clamp(val, 0x7fff, -0x8000); + } }
/*
A minor optimization; while pausing, the driver just copies the zero that doesn't need any volume changes.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/playback.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c index cbcd97f5d629..3762a98026aa 100644 --- a/sound/usb/line6/playback.c +++ b/sound/usb/line6/playback.c @@ -234,13 +234,14 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm) line6pcm->out.pos += urb_frames; if (line6pcm->out.pos >= runtime->buffer_size) line6pcm->out.pos -= runtime->buffer_size; + + change_volume(urb_out, line6pcm->volume_playback, + bytes_per_frame); } else { memset(urb_out->transfer_buffer, 0, urb_out->transfer_buffer_length); }
- change_volume(urb_out, line6pcm->volume_playback, bytes_per_frame); - if (line6pcm->prev_fbuf != NULL) { if (line6pcm->flags & LINE6_BITS_PCM_IMPULSE) { create_impulse_test_signal(line6pcm, urb_out,
participants (2)
-
Chris Rorvick -
Takashi Iwai