On Tue, Apr 10, 2007 at 11:12:45AM -0400, Alan Stern wrote: > On Mon, 9 Apr 2007 [EMAIL PROTECTED] wrote: > > > > > This is a note to let you know that I've just added the patch titled > > > > Subject: USB: jiffies is an unsigned long so treat it that way > > > > to my gregkh-2.6 tree. Its filename is > > > > usb-jiffies-long.patch > > > > This tree can be found at > > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/ > > > > > > >From [EMAIL PROTECTED] Tue Apr 9 12:12:43 2002 > > Date: Mon, 9 Apr 2007 11:52:31 -0400 (EDT) > > To: Greg KH <[EMAIL PROTECTED]> > > From: Greg Kroah-Hartman <[EMAIL PROTECTED]> > > Subject: USB: jiffies is an unsigned long so treat it that way > > > > In the suspend logic, we are using jiffies as a signed long, which can > > cause problems. This patch fixes this, and uses the proper function to > > determine if something has expired or not, instead of attempting to roll > > it ourselves. > > > > Thanks to Oliver for helping with the unsigned/signed issues in thinking > > this patch through. > > > > Cc: Alan Stern <[EMAIL PROTECTED]> > > Cc: Oliver Neukum <[EMAIL PROTECTED]> > > Cc: David Brownell <[EMAIL PROTECTED]> > > Cc: linux-usb-devel <linux-usb-devel@lists.sourceforge.net> > > Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]> > > > > --- > > drivers/usb/core/driver.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > --- gregkh-2.6.orig/drivers/usb/core/driver.c > > +++ gregkh-2.6/drivers/usb/core/driver.c > > @@ -932,7 +932,7 @@ static int autosuspend_check(struct usb_ > > { > > int i; > > struct usb_interface *intf; > > - long suspend_time; > > + unsigned long suspend_time; > > > > /* For autosuspend, fail fast if anything is in use or autosuspend > > * is disabled. Also fail if any interfaces require remote wakeup > > @@ -944,7 +944,6 @@ static int autosuspend_check(struct usb_ > > if (udev->autosuspend_delay < 0 || udev->autosuspend_disabled) > > return -EPERM; > > > > - suspend_time = udev->last_busy + udev->autosuspend_delay; > > if (udev->actconfig) { > > for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) { > > intf = udev->actconfig->interface[i]; > > @@ -964,8 +963,8 @@ static int autosuspend_check(struct usb_ > > /* If everything is okay but the device hasn't been idle for long > > * enough, queue a delayed autosuspend request. > > */ > > - suspend_time -= jiffies; > > - if (suspend_time > 0) { > > + suspend_time = udev->last_busy + udev->autosuspend_delay; > > + if (time_after(suspend_time, jiffies)) { > > if (!timer_pending(&udev->autosuspend.timer)) > > queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend, > > suspend_time); > > This change is wrong. The last argument to queue_delayed_work() is > supposed to be a delay value, not an absolute time. > > I suppose you could simply replace "suspend_time" with "suspend_time - > jiffies" in the last line above. It's a little suspect because the value > of jiffies might change between the the timer_after() comparison and the > function call. (That's why I wrote the code in a roundabout way -- so it > wouldn't read jiffies more than once.) I suppose that wouldn't hurt; > setting a timer to go off at the present moment or in the recent past does > behave sanely. However it would be a good idea to add a comment > explaining why it's okay.
Ok, care to make a patch that clarifies this? Also I really think the signed usage is wrong and will not properly handle the jiffies wrap around issue as the code is today. > Note: If at some future point we decide to add last_busy and > autosuspend_delay values for individual interfaces, then the calculation > will have to change anyway. At the very least, the initial assignment to > suspend_time will have to move back to where I had it originally. That's fine, the line can move then :) thanks, greg k-h ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
