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

["Andrej Kruták" <dev@andree.sk>]

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?

-- 
Andrej