Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1296873 > unrolled thread

[PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function "Felipe F. Tonello" <eu@felipetonello.com> - 2015-12-22 17:10 +0100
    Re: [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit  function Peter Chen <hzpeterchen@gmail.com> - 2015-12-25 09:10 +0100

#1296873 — [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function

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
allocated a usb_request for every new transmit made.

Signed-off-by: Felipe F. Tonello <eu@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

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

[toc] | [next] | [standalone]

#1298098 — Re: [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function

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 <eu@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 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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web