On Fri, Aug 12, 2016 at 10:44 AM, Takashi Iwai tiwai@suse.de wrote:
On Thu, 11 Aug 2016 21:02:21 +0200, Andrej Krutak wrote:
We must do it this way, because e.g. POD X3 won't play any sound unless the host listens on the bulk EP, so we cannot export it only via libusb.
The driver currently doesn't use the bulk EP messages in other way, in future it could e.g. sense/modify volume(s).
Signed-off-by: Andrej Krutak dev@andree.sk
include/uapi/sound/asound.h | 3 +- sound/usb/line6/driver.c | 167 ++++++++++++++++++++++++++++++++++++++++++++ sound/usb/line6/driver.h | 12 ++++ 3 files changed, 181 insertions(+), 1 deletion(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 609cadb..be353a7 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -106,9 +106,10 @@ enum { SNDRV_HWDEP_IFACE_FW_OXFW, /* Oxford OXFW970/971 based device */ SNDRV_HWDEP_IFACE_FW_DIGI00X, /* Digidesign Digi 002/003 family */ SNDRV_HWDEP_IFACE_FW_TASCAM, /* TASCAM FireWire series */
SNDRV_HWDEP_IFACE_LINE6, /* Line6 USB processors */ /* Don't forget to change the following: */
SNDRV_HWDEP_IFACE_LAST = SNDRV_HWDEP_IFACE_FW_TASCAM
SNDRV_HWDEP_IFACE_LAST = SNDRV_HWDEP_IFACE_LINE6};
struct snd_hwdep_info { diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index 8a71d45..0a0e324 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -14,9 +14,11 @@ #include <linux/export.h> #include <linux/slab.h> #include <linux/usb.h> +#include <linux/circ_buf.h>
#include <sound/core.h> #include <sound/initval.h> +#include <sound/hwdep.h>
#include "capture.h" #include "driver.h" @@ -315,8 +317,11 @@ static void line6_data_received(struct urb *urb) line6->process_message(line6); } } else {
line6->buffer_message = urb->transfer_buffer;line6->message_length = urb->actual_length; if (line6->process_message) line6->process_message(line6);line6->buffer_message = NULL; } line6_start_listen(line6);@@ -522,6 +527,163 @@ static void line6_get_interval(struct usb_line6 *line6) } }
+/* Enable buffering of incoming messages, flush the buffer */ +static int line6_hwdep_open(struct snd_hwdep *hw, struct file *file) +{
struct usb_line6 *line6 = hw->private_data;/* NOTE: hwdep already provides atomicity (exclusive == true), but for* sure... */if (test_and_set_bit(0, &line6->buffer_circular.active))return -EBUSY;line6->buffer_circular.head = 0;line6->buffer_circular.tail = 0;sema_init(&line6->buffer_circular.sem, 0);Why do you use semaphore, not mutex? And initializing at this point...? It looks racy.
I can move it to hwdep_init, but here it should be fine too, since hwdep_open() is serialized by hwdep layer.
Semaphore is used because it also uses as counter for hwdep_read, which otherwise I'd have to do manually. I could rewrite this to use waitqueues, but when the counter is added to that, I end up basically with semaphore reimplementation... Do you see this as a big issue?
line6->buffer_circular.active = 1;Looks superfluous, done in the above?
This is here to just drop the USB packets if there's no active reader. To save some CPU time basically...
line6->buffer_circular.data =kmalloc(LINE6_MESSAGE_MAXLEN * LINE6_MESSAGE_MAXCOUNT, GFP_KERNEL);if (!line6->buffer_circular.data) {return -ENOMEM;}line6->buffer_circular.data_len =kmalloc(sizeof(*line6->buffer_circular.data_len) *LINE6_MESSAGE_MAXCOUNT, GFP_KERNEL);if (!line6->buffer_circular.data_len) {kfree(line6->buffer_circular.data);return -ENOMEM;}return 0;+}
+/* Stop buffering */ +static int line6_hwdep_release(struct snd_hwdep *hw, struct file *file) +{
struct usb_line6 *line6 = hw->private_data;/* By this time, no readers are waiting, we can safely recreate the* semaphore at next open. */line6->buffer_circular.active = 0;kfree(line6->buffer_circular.data);kfree(line6->buffer_circular.data_len);return 0;+}
+/* Read from circular buffer, return to user */ +static long +line6_hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count,
loff_t *offset)+{
struct usb_line6 *line6 = hwdep->private_data;unsigned long tail;No need to use long for FIFO index.
See below.
if (down_interruptible(&line6->buffer_circular.sem)) {return -ERESTARTSYS;}/* There must an item now in the buffer... */tail = line6->buffer_circular.tail;if (line6->buffer_circular.data_len[tail] > count) {/* Buffer too small; allow re-read of the current item... */up(&line6->buffer_circular.sem);return -EINVAL;}if (copy_to_user(buf,&line6->buffer_circular.data[tail * LINE6_MESSAGE_MAXLEN],line6->buffer_circular.data_len[tail])) {rv = -EFAULT;goto end;} else {rv = line6->buffer_circular.data_len[tail];}smp_store_release(&line6->buffer_circular.tail,(tail + 1) & (LINE6_MESSAGE_MAXCOUNT - 1));return 0;+}
+/* Write directly (no buffering) to device by user*/ +static long +line6_hwdep_write(struct snd_hwdep *hwdep, const char __user *data, long count,
loff_t *offset)+{
struct usb_line6 *line6 = hwdep->private_data;int rv;char *data_copy;data_copy = kmalloc(count, GFP_ATOMIC);No need to use GFP_ATOMIC in this context. Also, you should add the sanity check of the given size. User may pass any size of the write.
if (!data_copy)return -ENOMEM;if (copy_from_user(data_copy, data, count))rv = -EFAULT;Maybe easier to use memdup_user() helper.
elserv = line6_send_raw_message(line6, data_copy, count);kfree(data_copy);return rv;+}
+static const struct snd_hwdep_ops hwdep_ops = {
.open = line6_hwdep_open,.release = line6_hwdep_release,.read = line6_hwdep_read,.write = line6_hwdep_write,+};
+/* Insert into circular buffer */ +static void line6_hwdep_push_message(struct usb_line6 *line6) +{
unsigned long head = line6->buffer_circular.head;/* The spin_unlock() and next spin_lock() provide needed ordering. */unsigned long tail = ACCESS_ONCE(line6->buffer_circular.tail);if (!line6->buffer_circular.active)return;if (CIRC_SPACE(head, tail, LINE6_MESSAGE_MAXCOUNT) >= 1) {unsigned char *item = &line6->buffer_circular.data[head * LINE6_MESSAGE_MAXLEN];memcpy(item, line6->buffer_message, line6->message_length);line6->buffer_circular.data_len[head] = line6->message_length;smp_store_release(&line6->buffer_circular.head,(head + 1) & (LINE6_MESSAGE_MAXCOUNT - 1));up(&line6->buffer_circular.sem);}Hmm... this kind of a simple FIFO can be seen in anywhere in the kernel code, and I'm sure that you can find an easier way to implement it. The whole code looks a bit scary as it being home-brewed.
This code is based on Documentation/circular-buffers.txt, except for the semaphore magic.
Also, the blocking read/write control isn't usually done by a semaphore. Then you can handle the interrupt there.
I actually wonder why, semaphores seemed perfect for this... Do you have some hints?