All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] usb: gadget: f_midi: unlock on error
@ 2016-04-02  4:51 Dan Carpenter
  2016-04-04  9:43 ` Felipe Ferreri Tonello
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Dan Carpenter @ 2016-04-02  4:51 UTC (permalink / raw)
  To: kernel-janitors

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@oracle.com>

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);

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [patch] usb: gadget: f_midi: unlock on error
  2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
@ 2016-04-04  9:43 ` Felipe Ferreri Tonello
  2016-04-04 12:11 ` Michal Nazarewicz
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Felipe Ferreri Tonello @ 2016-04-04  9:43 UTC (permalink / raw)
  To: kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Hi Dan,

On 02/04/16 05:51, 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@oracle.com>

Acked-by: Felipe F. Tonello <eu@felipetonello.com>

> 
> 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);
> 

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [patch] usb: gadget: f_midi: unlock on error
  2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
  2016-04-04  9:43 ` Felipe Ferreri Tonello
@ 2016-04-04 12:11 ` Michal Nazarewicz
  2016-04-04 12:18 ` Felipe Balbi
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Michal Nazarewicz @ 2016-04-04 12:11 UTC (permalink / raw)
  To: kernel-janitors

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 3481 bytes --]

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@oracle.com>
>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

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 kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [patch] usb: gadget: f_midi: unlock on error
  2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
  2016-04-04  9:43 ` Felipe Ferreri Tonello
  2016-04-04 12:11 ` Michal Nazarewicz
@ 2016-04-04 12:18 ` Felipe Balbi
  2016-04-04 13:22 ` Felipe Ferreri Tonello
  2016-04-04 19:25 ` Michal Nazarewicz
  4 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2016-04-04 12:18 UTC (permalink / raw)
  To: kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Michal Nazarewicz <mina86@mina86.com> writes:

> 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@oracle.com>
>>
>
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
>
> But perhaps this would be cleaner:

both options below are not real fixes and, as such, as not subject to
the -rc cycle.

thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [patch] usb: gadget: f_midi: unlock on error
  2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
                   ` (2 preceding siblings ...)
  2016-04-04 12:18 ` Felipe Balbi
@ 2016-04-04 13:22 ` Felipe Ferreri Tonello
  2016-04-04 19:25 ` Michal Nazarewicz
  4 siblings, 0 replies; 6+ messages in thread
From: Felipe Ferreri Tonello @ 2016-04-04 13:22 UTC (permalink / raw)
  To: kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]

Hi Michal,

On 04/04/16 13:11, Michal Nazarewicz wrote:
> 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@oracle.com>
>>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
> 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)

I am fine with these options, probably the second, but I don't think
they are any clearer than before, so I don't see any real benefits with
the changes.


In fact, I think f_midi_do_transmit should be documented, since there
are 3 possible return condition:
<0 breaks the loop and drop out substreams
0 breaks the loop
>0 continues the loop

> 
> 
> 
>> 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);
> 

-- 
Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [patch] usb: gadget: f_midi: unlock on error
  2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
                   ` (3 preceding siblings ...)
  2016-04-04 13:22 ` Felipe Ferreri Tonello
@ 2016-04-04 19:25 ` Michal Nazarewicz
  4 siblings, 0 replies; 6+ messages in thread
From: Michal Nazarewicz @ 2016-04-04 19:25 UTC (permalink / raw)
  To: kernel-janitors

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 2469 bytes --]

> On 04/04/16 13:11, Michal Nazarewicz wrote:
>> 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)

On Mon, Apr 04 2016, Felipe Ferreri Tonello wrote:
> I am fine with these options, probably the second, but I don't think
> they are any clearer than before, so I don't see any real benefits with
> the changes.

The spin_lock_irqsave is paired with exactly one spin_unlock_irqrestore
which is cleaner in my book.  I don’t have strong feelings about this
though.

> In fact, I think f_midi_do_transmit should be documented, since there
> are 3 possible return condition:
> <0 breaks the loop and drop out substreams
> 0 breaks the loop
> >0 continues the loop

That’s of course separate issue.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-04 19:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-02  4:51 [patch] usb: gadget: f_midi: unlock on error Dan Carpenter
2016-04-04  9:43 ` Felipe Ferreri Tonello
2016-04-04 12:11 ` Michal Nazarewicz
2016-04-04 12:18 ` Felipe Balbi
2016-04-04 13:22 ` Felipe Ferreri Tonello
2016-04-04 19:25 ` Michal Nazarewicz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.