[Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout() with

[Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout() with

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

I would appreciate any comments from the janitors list. This is one (of
many) cases where I made a decision about replacing

set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);

with

msleep(jiffies_to_msecs(some_time));

msleep() is not exactly the same as the previous code, but I only did
this replacement where I thought long delays were *desired*. If this is
not the case here, then just disregard this patch.

Thanks,
Nish



Applys-to: 2.6.7

Description: Use msleep() instead of schedule_timeout() to guarantee
desired timeout.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- linux-vanilla/drivers/ieee1394/sbp2.c	2004-06-16 05:18:58.000000000 +0000
+++ linux-dev/drivers/ieee1394/sbp2.c	2004-07-12 18:07:08.000000000 +0000
@@ -903,8 +903,7 @@ alloc_fail:
 	 * connected to the sbp2 device being removed. That host would
 	 * have a certain amount of time to relogin before the sbp2 device
 	 * allows someone else to login instead. One second makes sense. */
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout(HZ);
+	msleep(1000);
 
 	/*
 	 * Login to the sbp-2 device

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

[Ben Collins]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On Thu, Jul 22, 2004 at 10:35:28PM +0000, Nishanth Aravamudan wrote:
> I would appreciate any comments from the janitors list. This is one (of
> many) cases where I made a decision about replacing
> 
> set_current_state(TASK_INTERRUPTIBLE);
> schedule_timeout(some_time);
> 
> with
> 
> msleep(jiffies_to_msecs(some_time));
> 
> msleep() is not exactly the same as the previous code, but I only did
> this replacement where I thought long delays were *desired*. If this is
> not the case here, then just disregard this patch.

Doesn't msleep() block? If so, that's not what we want at all. We want
other things to happen during that time (like maybe catch a bus reset).

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Thu, Jul 22, 2004 at 07:26:53PM -0400, Ben Collins wrote:
> Doesn't msleep() block? If so, that's not what we want at all. We want
> other things to happen during that time (like maybe catch a bus reset).

No, msleep sleeps (as its name suggests).  The code that implements it is:

void msleep(unsigned int msecs)
{
        unsigned long timeout = msecs_to_jiffies(msecs);

        while (timeout) {
                set_current_state(TASK_UNINTERRUPTIBLE);
                timeout = schedule_timeout(timeout);
        }
}

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

[Ben Collins]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Fri, Jul 23, 2004 at 02:04:03AM +0100, Matthew Wilcox wrote:
> On Thu, Jul 22, 2004 at 07:26:53PM -0400, Ben Collins wrote:
> > Doesn't msleep() block? If so, that's not what we want at all. We want
> > other things to happen during that time (like maybe catch a bus reset).
> 
> No, msleep sleeps (as its name suggests).  The code that implements it is:

In that case, I've got a few places that could use msleep().

Thanks.

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace

[Nish Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Thu, 22 Jul 2004 21:53:05 -0400, Ben Collins <bcollins@debian.org> wrote:
> On Fri, Jul 23, 2004 at 02:04:03AM +0100, Matthew Wilcox wrote:
> > On Thu, Jul 22, 2004 at 07:26:53PM -0400, Ben Collins wrote:
> > > Doesn't msleep() block? If so, that's not what we want at all. We want
> > > other things to happen during that time (like maybe catch a bus reset).
> >
> > No, msleep sleeps (as its name suggests).  The code that implements it is:
> 
> In that case, I've got a few places that could use msleep().

The one thing is, it depends on what you mean by "like maybe catch a
bus reset" (I think), because msleep will sleep through all signals,
i.e. it provides a guaranteed minimum delay. In the case of my patch,
I didn't see you checking for any signals afterwards (or any comments
indicating you were expecting some) so I figured it would be safe to
use msleep(). Also, I think it's clearer what's going on if the time
units for delays are msecs.

-Nish

PS. Would these other places be in drivers as well?

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors