Re: patch usb-jiffies-long.patch added to gregkh-2.6 tree

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 foo@baz 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 <[email protected]>
> > 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
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel