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

[Takashi Iwai]

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 at 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