On Tue, Mar 08, 2005 at 12:05:45PM -0500, Alan Stern wrote:
> 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.
I was trying to keep to the current code as closely as possible, sorry.
It loops at 10 jiffy intervals up to 50 times, which is a total of 500
jiffies. If the intent was to use milliseconds, though, then
interruptible_sleep_on_timeout() was a much more broken usage than I
thought. Trust me, if I thought milliseconds should be used, I would
have used them :)
> > 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?
Yes, I will fix that.
> 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.
I was banking on schedule_timeout() returning early. Because it now uses
TASK_UNINTERRUPTIBLE, only wait-queue events should cause the timeout to
return a non-zero value. I just changed the wake_up_interruptible() call
in the callback to wake_up() so it will catch the UNINTERRUPTIBLE tasks
as well. Unfortunately other paths also wake-up the same wq, so the fix
is, as you said, incomplete.
I have no means of testing this device or my patches, though. And, to be
frank, don't care too much about the device beyond a general feeling of
making Linux better :) I have to get some other work done in the mean
time, but I will try to get a revised patch out.
> 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.
Ok, I will verify this is the case in the new patch.
Thanks,
Nish
-------------------------------------------------------
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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
