On Sun, Mar 06, 2005 at 06:14:00PM -0500, Alan Stern wrote:
> On Sun, 6 Mar 2005 [EMAIL PROTECTED] wrote:
>
> > Replace direct wait-queue usage with wait_event_timeout(). Removed
> > some local variables which help determine loop time, but which are now
> > compressed into the wait_event_timeout() macro. Patch is compile-tested.
> >
> > Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
> > Signed-off-by: Domen Puncer <[EMAIL PROTECTED]>
> > ---
> >
> >
> > kj-domen/drivers/usb/class/usb-midi.c | 18 ++++--------------
> > 1 files changed, 4 insertions(+), 14 deletions(-)
> >
> > diff -puN
> > drivers/usb/class/usb-midi.c~int_sleep_on-drivers_usb_class_usb-midi
> > drivers/usb/class/usb-midi.c
> > --- kj/drivers/usb/class/usb-midi.c~int_sleep_on-drivers_usb_class_usb-midi
> > 2005-03-05 16:12:10.000000000 +0100
> > +++ kj-domen/drivers/usb/class/usb-midi.c 2005-03-05 16:12:10.000000000
> > +0100
>
> > @@ -335,19 +334,10 @@ static int usb_write( struct midi_out_en
> > goto error;
> > }
> >
> > - add_wait_queue( &ep->wait, &wait );
> > - set_current_state( TASK_INTERRUPTIBLE );
> > -
> > - while( ep->urb->status == -EINPROGRESS ) {
> > - if ( maxretry-- < 0 ) {
> > - printk(KERN_ERR "usbmidi: usb_bulk_msg timed out\n");
> > - ret = -ETIME;
> > - break;
> > - }
> > - interruptible_sleep_on_timeout( &ep->wait, 10 );
> > + if (!wait_event_timeout(ep->wait, (ep->urb->status != -EINPROGRESS),
> > 500)) {
> > + printk(KERN_ERR "usbmidi: usb_bulk_msg timed out\n");
> > + ret = -ETIME;
>
> While there's nothing objectionable about the proposed patch, the code's
> strategy isn't so hot. A driver should never test whether urb->status
> is equal to -EINPROGRESS; instead it should rely on a completion handler
> to set a flag it can test. (Testing for urb->status == -EINPROGRESS is
> unreliable because the USB stack will set it to other values even while
> the URB is still in progress.)
Ah, this is the same problem Vojtech had with a different patch I had
sent. Does the attached fix seem better / more appropriate?
> Is there currently a maintainer for the usb-midi driver?
Not that I am aware of.
Thanks,
Nish
Description: The wait-queue code in usb-midi is excessive, as the
complete() call-back will wake-up the waitqueue on success. We can
simply sleep for about half a minute, waiting for this wakeup, instead
of checking status at regular intervals, which is not allowed outside
the complete() call-back anyways. interruptible_sleep_on() is also
deprecated, so this removes a further usage.
Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
--- 2.6.11-v/drivers/usb/class/usb-midi.c 2005-03-01 23:37:45.000000000
-0800
+++ 2.6.11/drivers/usb/class/usb-midi.c 2005-03-07 15:25:07.000000000 -0800
@@ -317,7 +317,7 @@ static int usb_write( struct midi_out_en
int pipe;
int ret = 0;
int status;
- int maxretry = 50;
+ unsigned long timeout = 500;
DECLARE_WAITQUEUE(wait,current);
init_waitqueue_head(&ep->wait);
@@ -335,20 +335,14 @@ static int usb_write( struct midi_out_en
goto error;
}
- add_wait_queue( &ep->wait, &wait );
- set_current_state( TASK_INTERRUPTIBLE );
-
- while( ep->urb->status == -EINPROGRESS ) {
- if ( maxretry-- < 0 ) {
- printk(KERN_ERR "usbmidi: usb_bulk_msg timed out\n");
- ret = -ETIME;
- break;
- }
- interruptible_sleep_on_timeout( &ep->wait, 10 );
- }
- set_current_state( TASK_RUNNING );
- remove_wait_queue( &ep->wait, &wait );
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ add_wait_queue(&ep->wait, &wait);
+ /* if no wq events in 500 jiffies */
+ if (!schedule_timeout(timeout))
+ ret = -ETIME;
+
+ remove_wait_queue(&ep->wait, &wait);
error:
return ret;
}
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
