usb: gadget: f_midi: unlock on error
Subsystems (2)
PATCH 1/1 usb: gadget: f_midi: unlock on error
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 <[email protected]>
drivers/usb/gadget/function/f_midi.c +3 -1
@@ -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);
Hi Dan,
Quoted 5 lines · Dan Carpenter
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 <[email protected]>
Quoted 18 lines
drivers/usb/gadget/function/f_midi.c +3
-1@@ -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);
Signature · 1 line
Felipe
Quoted 6 lines · Dan Carpenter
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 <[email protected]>
But perhaps this would be cleaner:
drivers/usb/gadget/function/f_midi.c +5 -10
@@ -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:
drivers/usb/gadget/function/f_midi.c +10 -17
@@ -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)
Quoted 16 lines
drivers/usb/gadget/function/f_midi.c +3
-1@@ -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);
Signature · 7 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Michal Nazarewicz
Quoted 6 lines · Dan Carpenter
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 <[email protected]>But perhaps this would be cleaner:
both options below are not real fixes and, as such, as not subject to the -rc cycle.
thanks
Signature · 1 line
balbi
Hi Michal,
Quoted 76 lines · Michal Nazarewicz
Quoted 6 lines · Dan Carpenter
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 <[email protected]>But perhaps this would be cleaner:
drivers/usb/gadget/function/f_midi.c +5
-10@@ -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:
drivers/usb/gadget/function/f_midi.c +6
-15@@ -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 */ + }
drivers/usb/gadget/function/f_midi.c done: · line 617
Quoted 10 lines
drivers/usb/gadget/function/f_midi.c +4
-2+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
Quoted 22 lines
0 continues the loop
Quoted 16 lines
drivers/usb/gadget/function/f_midi.c +3
-1@@ -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);
Signature · 1 line
Felipe
Quoted 45 lines · Michal Nazarewicz
Quoted 44 lines
Or maybe even this which gets away with gotos all together:
drivers/usb/gadget/function/f_midi.c +10
-17@@ -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)
Quoted 3 lines · Felipe Ferreri Tonello
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.
Quoted 5 lines
In fact, I think
f_midi_do_transmitshould be documented, since there are 3 possible return condition:
<0 breaks the loop and drop out substreams
0 breaks the loopQuoted 1 line
0 continues the loop
That’s of course separate issue.
Signature · 7 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html