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

[Michal Nazarewicz <mina86@mina86.com>]

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