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

[Felipe Ferreri Tonello <eu@felipetonello.com>]

[-- 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 --]