On Tue, Dec 22, 2015 at 04:08:06PM +0000, Felipe F. Tonello wrote:
> Since f_midi_transmit is called by both ALSA and USB frameworks, it can
> potentially cause a race condition between both calls. This is bad because the
> way f_midi_transmit is implemented can't handle concurrent calls. This is due
> to the fact that the usb request fifo looks for the next element and only if
> it has data to process it enqueues the request, otherwise re-uses it. If both
> (ALSA and USB) frameworks calls this function at the same time, the
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
>
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by
> usb_request->complete
> callback in interrupt context.
>
> On benchmarks realized by me, spinlocks were more efficient then scheduling
> the f_midi_transmit tasklet in process context and using a mutex to
> synchronize. Also it performs better then previous implementation that
%s/then/than/
> allocated a usb_request for every new transmit made.
>
> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
> ---
> drivers/usb/gadget/function/f_midi.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index b70a830..00a15e9 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -24,6 +24,7 @@
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/kfifo.h>
> +#include <linux/spinlock.h>
>
> #include <sound/core.h>
> #include <sound/initval.h>
> @@ -97,6 +98,7 @@ struct f_midi {
> /* This fifo is used as a buffer ring for pre-allocated IN usb_requests
> */
> DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> unsigned int in_last_port;
> + spinlock_t transmit_lock;
> };
>
> static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -574,12 +576,15 @@ static void f_midi_drop_out_substreams(struct f_midi
> *midi)
> static void f_midi_transmit(struct f_midi *midi)
> {
> struct usb_ep *ep = midi->in_ep;
> + unsigned long flags;
> bool active;
>
> /* We only care about USB requests if IN endpoint is enabled */
> if (!ep || !ep->enabled)
> goto drop_out;
>
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> +
> do {
> struct usb_request *req = NULL;
> unsigned int len, i;
> @@ -591,14 +596,17 @@ static void f_midi_transmit(struct f_midi *midi)
> len = kfifo_peek(&midi->in_req_fifo, &req);
> if (len != 1) {
> ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> goto drop_out;
> }
>
> /* If buffer overrun, then we ignore this transmission.
> * IMPORTANT: This will cause the user-space rawmidi device to
> block until a) usb
> * requests have been completed or b) snd_rawmidi_write() times
> out. */
> - if (req->length > 0)
> + if (req->length > 0) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> return;
> + }
>
> for (i = midi->in_last_port; i < MAX_PORTS; i++) {
> struct gmidi_in_port *port = midi->in_port[i];
> @@ -650,6 +658,8 @@ static void f_midi_transmit(struct f_midi *midi)
> }
> } while (active);
>
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> +
> return;
>
> drop_out:
> @@ -1255,6 +1265,8 @@ static struct usb_function *f_midi_alloc(struct
> usb_function_instance *fi)
> if (status)
> goto setup_fail;
>
> + spin_lock_init(&midi->transmit_lock);
> +
> ++opts->refcnt;
> mutex_unlock(&opts->lock);
>
> --
> 2.5.0
>
> --
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
