On Mon, 7 Mar 2005, Nishanth Aravamudan wrote:
> Ah, this is the same problem Vojtech had with a different patch I had
> sent. Does the attached fix seem better / more appropriate?
There are still some things to consider. See below.
> --- 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;
_Please_ don't set timeout to 500 jiffies! Use msecs_to_jiffies(500)
instead.
> 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;
> }
You removed the error message. Should it remain present?
Note that schedule_timeout can return before the timeout has elapsed, even
though the condition you're waiting for (URB completed) hasn't happened
yet. People use a loop to catch these things, and you definitely need a
way to test for URB completed. But seeing if ep->urb->status is equal to
-EINPROGRESS is not the correct way to do it. I see that the completion
handler, usb_write_callback, doesn't do anything but wake up the
waitqueue. That's not good enough; it needs to set a flag in the ep
structure for you to test.
Note also that the test must be made _before_ calling schedule_timeout.
Otherwise you have a race; the URB may complete before you have managed to
call add_wait_queue.
Alan Stern
-------------------------------------------------------
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
