Re: [alsa-devel] [PATCH 09/15] ALSA: line6: Add hwdep interface to access the POD control messages

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