| Message ID | 1470942147-19848-10-git-send-email-dev@andree.sk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + line6->buffer_circular.active = 1; Looks superfluous, done in the above? > + 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. > + 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. > + else > + rv = 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. Also, the blocking read/write control isn't usually done by a semaphore. Then you can handle the interrupt there. Takashi
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. > > > + else > > + rv = 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?
On Fri, 12 Aug 2016 13:10:37 +0200, Andrej Kruták wrote: > > > +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. The example there is basically a semi lock-free implementation. For your purpose it's an overkill. This is no severely hot path, thus a simpler version would make life easier. > > 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? Assume you want to interrupt the user-space app while it's being blocked by the semaphore. With your code, you can't. Takashi
On Fri, Aug 12, 2016 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Fri, 12 Aug 2016 13:10:37 +0200, > Andrej Kruták wrote: >> > > +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. > > The example there is basically a semi lock-free implementation. For > your purpose it's an overkill. This is no severely hot path, thus a > simpler version would make life easier. > Fair enough, I'll spinlock-ize it then. >> > 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? > > Assume you want to interrupt the user-space app while it's being > blocked by the semaphore. With your code, you can't. > > You can - down_interruptible() is there for this exact reason. As said, semaphore is basically wait-queue + counter, so there's no real reason not to do this IMO. But in the end - if you have some nice example of FIFO buffer in some simple driver, I have no problem using a more already-proven/standard way :-)
On Fri, 12 Aug 2016 14:15:16 +0200, Andrej Kruták wrote: > > On Fri, Aug 12, 2016 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Fri, 12 Aug 2016 13:10:37 +0200, > > Andrej Kruták wrote: > >> > > +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. > > > > The example there is basically a semi lock-free implementation. For > > your purpose it's an overkill. This is no severely hot path, thus a > > simpler version would make life easier. > > > > Fair enough, I'll spinlock-ize it then. > > > >> > 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? > > > > Assume you want to interrupt the user-space app while it's being > > blocked by the semaphore. With your code, you can't. > > > > > > You can - down_interruptible() is there for this exact reason. There is another blocking path: you keep semaphore down after line6_hwdep_read() until line6_hwdep_push_message(). What happens if user-space interrupts during that, and line6_hwdep_push_message() is delayed or stall by some reason? > As > said, semaphore is basically wait-queue + counter, so there's no real > reason not to do this IMO. > > But in the end - if you have some nice example of FIFO buffer in some > simple driver, I have no problem using a more already-proven/standard > way :-) Just use the normal waitqueue and schedule or wait_event() or its variant. Takashi
On Fri, Aug 12, 2016 at 2:30 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Fri, 12 Aug 2016 14:15:16 +0200, >> >> >> > 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? >> > >> > Assume you want to interrupt the user-space app while it's being >> > blocked by the semaphore. With your code, you can't. >> > >> > >> >> You can - down_interruptible() is there for this exact reason. > > There is another blocking path: you keep semaphore down after > line6_hwdep_read() until line6_hwdep_push_message(). What happens if > user-space interrupts during that, and line6_hwdep_push_message() is > delayed or stall by some reason? > Actually, I think I don't see what's the "another path" here, could you please elaborate one more bit? I just want to make sure to not reimplement the same race using waitqueue... What's the point then? line6_hwdep_push_message() could get not scheduled for some while. So until it calls up(), line6_hwdep_read() will block on down_interruptible(), or until signal (in which case user gets -ERESTARTSYS). After up() is called, there are data in buffer... If line6_hwdep_read() returns after interrupt, no problem - the buffer will just continue to be filled and semaphore will be up()'d while there's free buffer space. Or until the device is closed... If I do the same via waitqueue, I will have the same problems, no? Maybe if you could post the steps where you see the race... At the same time, looking at __down_common(), it just does the standard waitqueue stuff (TASK_INTERRUPTIBLE + schedule()) (+counter in down())... So do you have some other race in mind? I'm running in circles, so surely you must :-) Sorry if I sound like a moron... and thanks for you time!
On Fri, 12 Aug 2016 18:43:30 +0200, Andrej Kruták wrote: > > On Fri, Aug 12, 2016 at 2:30 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Fri, 12 Aug 2016 14:15:16 +0200, > >> > >> >> > 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? > >> > > >> > Assume you want to interrupt the user-space app while it's being > >> > blocked by the semaphore. With your code, you can't. > >> > > >> > > >> > >> You can - down_interruptible() is there for this exact reason. > > > > There is another blocking path: you keep semaphore down after > > line6_hwdep_read() until line6_hwdep_push_message(). What happens if > > user-space interrupts during that, and line6_hwdep_push_message() is > > delayed or stall by some reason? > > > > Actually, I think I don't see what's the "another path" here, could > you please elaborate one more bit? I just want to make sure to not > reimplement the same race using waitqueue... > > What's the point then? line6_hwdep_push_message() could get not > scheduled for some while. So until it calls up(), line6_hwdep_read() > will block on down_interruptible(), or until signal (in which case > user gets -ERESTARTSYS). After up() is called, there are data in > buffer... Well, what happens if user aborts before up() is called in line6_hwdep_push_message()? Now the driver calls close, and it frees the memory. What if, at the very same time, line6_hwdep_push_message() is triggered? > If line6_hwdep_read() returns after interrupt, no problem - > the buffer will just continue to be filled and semaphore will be > up()'d while there's free buffer space. Or until the device is > closed... Or, what if line6_hwdep_push_message() is triggered twice or more before line6_hwdep_read() is called? It will call up() twice or more. Then at this point, you call line6_hwdep_read() concurrently from two threads... How do they protect against each other? > If I do the same via waitqueue, I will have the same problems, no? > Maybe if you could post the steps where you see the race... In your code, it's not clear that you're protecting from what. A simple lock+wait loop shows it more easily, at least. Takashi > At the same time, looking at __down_common(), it just does the > standard waitqueue stuff (TASK_INTERRUPTIBLE + schedule()) (+counter > in down())... So do you have some other race in mind? I'm running in > circles, so surely you must :-) > > > Sorry if I sound like a moron... and thanks for you time! > > -- > Andrej >
On Fri, Aug 12, 2016 at 10:01 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Fri, 12 Aug 2016 18:43:30 +0200, > Andrej Kruták wrote: >> >> On Fri, Aug 12, 2016 at 2:30 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > On Fri, 12 Aug 2016 14:15:16 +0200, >> >> >> >> >> > 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? >> >> > >> >> > Assume you want to interrupt the user-space app while it's being >> >> > blocked by the semaphore. With your code, you can't. >> >> > >> >> > >> >> >> >> You can - down_interruptible() is there for this exact reason. >> > >> > There is another blocking path: you keep semaphore down after >> > line6_hwdep_read() until line6_hwdep_push_message(). What happens if >> > user-space interrupts during that, and line6_hwdep_push_message() is >> > delayed or stall by some reason? >> > >> >> Actually, I think I don't see what's the "another path" here, could >> you please elaborate one more bit? I just want to make sure to not >> reimplement the same race using waitqueue... >> >> What's the point then? line6_hwdep_push_message() could get not >> scheduled for some while. So until it calls up(), line6_hwdep_read() >> will block on down_interruptible(), or until signal (in which case >> user gets -ERESTARTSYS). After up() is called, there are data in >> buffer... > > Well, what happens if user aborts before up() is called in > line6_hwdep_push_message()? Now the driver calls close, and it frees > the memory. What if, at the very same time, > line6_hwdep_push_message() is triggered? > Right, the 'active' flag is cleary not a good lock... >> If line6_hwdep_read() returns after interrupt, no problem - >> the buffer will just continue to be filled and semaphore will be >> up()'d while there's free buffer space. Or until the device is >> closed... > > Or, what if line6_hwdep_push_message() is triggered twice or more > before line6_hwdep_read() is called? It will call up() twice or > more. Then at this point, you call line6_hwdep_read() concurrently > from two threads... How do they protect against each other? > There's read_lock, unfortunately after "fixing" the sleep-in-atomic in line6_hwdep_read(), it's broken now... >> If I do the same via waitqueue, I will have the same problems, no? >> Maybe if you could post the steps where you see the race... > > In your code, it's not clear that you're protecting from what. > A simple lock+wait loop shows it more easily, at least. > Okay, I'll try to rethink/rework it, simplify the code, and get back. Thanks!
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); + + line6->buffer_circular.active = 1; + + 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; + + 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); + if (!data_copy) + return -ENOMEM; + + if (copy_from_user(data_copy, data, count)) + rv = -EFAULT; + else + rv = 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); + } +} + +static int line6_hwdep_init(struct usb_line6 *line6) +{ + int err; + struct snd_hwdep *hwdep; + + /* TODO: usb_driver_claim_interface(); */ + line6->process_message = line6_hwdep_push_message; + + line6->buffer_circular.active = 0; + err = snd_hwdep_new(line6->card, "config", 0, &hwdep); + if (err < 0) + goto end; + strcpy(hwdep->name, "config"); + hwdep->iface = SNDRV_HWDEP_IFACE_LINE6; + hwdep->ops = hwdep_ops; + hwdep->private_data = line6; + hwdep->exclusive = true; + +end: + return err; +} + static int line6_init_cap_control(struct usb_line6 *line6) { int ret; @@ -539,6 +701,10 @@ static int line6_init_cap_control(struct usb_line6 *line6) line6->buffer_message = kmalloc(LINE6_MESSAGE_MAXLEN, GFP_KERNEL); if (!line6->buffer_message) return -ENOMEM; + } else { + ret = line6_hwdep_init(line6); + if (ret < 0) + return ret; } ret = line6_start_listen(line6); @@ -716,3 +882,4 @@ EXPORT_SYMBOL_GPL(line6_resume); MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL"); + diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h index 71ad299..f4ab6cd 100644 --- a/sound/usb/line6/driver.h +++ b/sound/usb/line6/driver.h @@ -33,6 +33,8 @@ #define LINE6_TIMEOUT (1) #define LINE6_BUFSIZE_LISTEN (64) #define LINE6_MESSAGE_MAXLEN (256) +/* Must be 2^n; 4k packets are common, MAXLEN * MAXCOUNT should be bigger... */ +#define LINE6_MESSAGE_MAXCOUNT (1 << 5) /* Line 6 MIDI control commands @@ -155,6 +157,16 @@ struct usb_line6 { /* Length of message to be processed, generated from MIDI layer */ int message_length; + /* Circular buffer for non-MIDI control messages */ + struct { + int active; + char *data; + int *data_len; + unsigned long head, tail; + /* Actually is up'd # of items in the buffer - times */ + struct semaphore sem; + } buffer_circular; + /* If MIDI is supported, buffer_message contains the pre-processed data; * otherwise the data is only in urb_listen (buffer_incoming). */ void (*process_message)(struct usb_line6 *);
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(-)