Re: [patch] usb: gadget: f_midi: unlock on error

[Michal Nazarewicz <mina86@xxxxxxxxxx>]

On Sat, Apr 02 2016, Dan Carpenter wrote:
> We added some new locking here, but missed an error path where we need
> to unlock.
>
> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit function')
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>

Acked-by: Michal Nazarewicz <mina86@xxxxxxxxxx>

But perhaps this would be cleaner:

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 56e2dde..c04436f 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -606,19 +606,14 @@ static void f_midi_transmit(struct f_midi *midi)
                goto drop_out;
 
        spin_lock_irqsave(&midi->transmit_lock, flags);
-
-       do {
-               ret = f_midi_do_transmit(midi, ep);
-               if (ret < 0)
-                       goto drop_out;
-       } while (ret);
-
+       while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
+               /* nop */
+       }
        spin_unlock_irqrestore(&midi->transmit_lock, flags);
 
-       return;
-
+       if (ret < 0)
 drop_out:
-       f_midi_drop_out_substreams(midi);
+               f_midi_drop_out_substreams(midi);
 }
 
 static void f_midi_in_tasklet(unsigned long data)

Or maybe even this which gets away with gotos all together:

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 56e2dde..91cae60 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -598,27 +598,20 @@ done:
 static void f_midi_transmit(struct f_midi *midi)
 {
        struct usb_ep *ep = midi->in_ep;
-       int ret;
        unsigned long flags;
+       int ret = -1;
 
        /* 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 {
-               ret = f_midi_do_transmit(midi, ep);
-               if (ret < 0)
-                       goto drop_out;
-       } while (ret);
-
-       spin_unlock_irqrestore(&midi->transmit_lock, flags);
-
-       return;
+       if (ep && ep->enabled) {
+               spin_lock_irqsave(&midi->transmit_lock, flags);
+               while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
+                       /* nop */
+               }
+               spin_unlock_irqrestore(&midi->transmit_lock, flags);
+       }
 
-drop_out:
-       f_midi_drop_out_substreams(midi);
+       if (ret < 0)
+               f_midi_drop_out_substreams(midi);
 }
 
 static void f_midi_in_tasklet(unsigned long data)



> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..2c0616c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>  
>  	do {
>  		ret = f_midi_do_transmit(midi, ep);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			spin_unlock_irqrestore(&midi->transmit_lock, flags);
>  			goto drop_out;
> +		}
>  	} while (ret);
>  
>  	spin_unlock_irqrestore(&midi->transmit_lock, flags);

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html