On Mon, 17 May 2004, Jean Tourrilhes wrote: > On Sun, May 16, 2004 at 04:44:51PM -0700, Greg KH wrote: > > On Sun, May 16, 2004 at 12:01:03PM -0400, Alan Stern wrote: > > > While looking through the irda-usb driver, I found that it does several > > > questionable things. > > Just because you don't understand the code doesn't make it > automatically questionable. Check comments in write_bulk_callback() to > understand what's happening.
I have read the comments. They didn't explain very much, not enough for me to fully understand what's happening. For instance, modifying urb->status in irda_urb_net_timeout() would be perfectly okay, provided you can guarantee that this routine is never called while the URBs are active. The comment in write_bulk_callback() indicates that this _might_ be the case, but the comments in irda_usb_net_timeout() indicate that it might not be. > > > irda_urb_net_timeout() looks at urb->status while the URB is > > > still in progress. > > If you check the code, you will see that it does that with irq > disabled. Therefore, the status is not goind to change under us. That's not true. Just because interrupts are disabled on one processor doesn't mean another processor won't handle them. The status might very well change under you. Furthermore, it doesn't matter here whether the URB status changes under you or not. The problem is that _you_ might change the URB status from under the USB core and host drivers! > > > Even worse, it changes urb->status! > > Yes, but only if the URB is idle. Check what the status in > question mean. I did check. You change urb->status when it is non-zero and not equal to -EINPROGRESS. So what? The status can be set to many possible values while the URB is still active and in use by the USB core. That's why you're not supposed to look at it unless the URB is idle. > > Ick ick ick. That's wrong and needs to be fixed. > > I spent enough time testing the driver and working around the > "features" of the USB API that I can say : be my guest. The basic idea is simple enough. When an URB has been successfully submitted it is owned by the USB core, not your driver. You may not modify it and probably shouldn't even read it until the completion handler is called. > > > The driver uses the urb->timeout field. This field is supported > > > only by the UHCI driver, and I would like to remove that support. > > > > You mean the ohci and ehci drivers do not support that? Since when? We > > need to update the documentation if that is true... > > Yep, I tested the timeout support with uhci, usb-uhci and ohci > driver in 2.4.X and uhic-hcd and ohci-hcd in 2.6.X. What is there to test in ohci-hcd? The only occurrences of "timeout" in the OHCI sources are in a comment and in "schedule_timeout". The driver completely ignores urb->timeout. Alan Stern ------------------------------------------------------- This SF.Net email is sponsored by: SourceForge.net Broadband Sign-up now for SourceForge Broadband and get the fastest 6.0/768 connection for only $19.95/mo for the first 3 months! http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
