Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754725AbbLYIIX (ORCPT ); Fri, 25 Dec 2015 03:08:23 -0500 Received: from mail-pa0-f43.google.com ([209.85.220.43]:33945 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753836AbbLYIIV (ORCPT ); Fri, 25 Dec 2015 03:08:21 -0500 Date: Fri, 25 Dec 2015 16:03:34 +0800 From: Peter Chen To: "Felipe F. Tonello" Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Felipe Balbi , Greg Kroah-Hartman , Robert Baldyga , Clemens Ladisch Subject: Re: [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function Message-ID: <20151225080334.GC9700@shlinux2> References: <1450800486-18150-1-git-send-email-eu@felipetonello.com> <1450800486-18150-2-git-send-email-eu@felipetonello.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450800486-18150-2-git-send-email-eu@felipetonello.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3991 Lines: 114 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 > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/