Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

On Thu, 02 Sep 2004, Maximilian scribeth:
> it shouldn't hinder 2.6 in it's progression.
I consider this a regression.
As schedule_timeout is used elesewhere in the prism54 code,
we are using a consistent and documented method.

Margit

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[maximilian attems]

On Thu, 02 Sep 2004, Margit Schubert-While wrote:

> On Thu, 02 Sep 2004, Maximilian scribeth:
> > it shouldn't hinder 2.6 in it's progression.
> I consider this a regression.
> As schedule_timeout is used elesewhere in the prism54 code,
> we are using a consistent and documented method.

you didn't answer to the unit argument in favour of msleep.

shure msleep is also consistent and documented.

--
maks
kernel janitor  	http://janitor.kernelnewbies.org/

Re: [Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Thu, Sep 02, 2004 at 12:03:22PM +0200, maximilian attems wrote:
> On Thu, 02 Sep 2004, Margit Schubert-While wrote:
> 
> > On Thu, 02 Sep 2004, Maximilian scribeth:
> > > it shouldn't hinder 2.6 in it's progression.
> > I consider this a regression.
> > As schedule_timeout is used elesewhere in the prism54 code,
> > we are using a consistent and documented method.

A grep of drivers/net/wireless/prism54 for schedule_timeout showed three
occurrences (in 2.6.9-rc1-bk7):

islpci_dev.c:   schedule_timeout(50*HZ/1000);
islpci_dev.c:           remaining = schedule_timeout(HZ);
islpci_mgt.c:           timeleft = schedule_timeout(wait_cycle_jiffies);

The first is removed by my patch.

The second & third are potentially bugs as there is no
set_current_state() preceding the call to schedule_timeout(). As per the
source:

/**
 * schedule_timeout - sleep until timeout
 * @timeout: timeout value in jiffies
 *
 * Make the current task sleep until @timeout jiffies have
 * elapsed. The routine will return immediately unless
 * the current task state has been set (see set_current_state()).

Therefore, in the current code, the schedule_timeout() call does not
have the desired effect (the same information is available in
kernel-hacking.ps).

Both of these calls should probably be fixed, but I'm not sure if you
wish to sleep in TASK_INTERUPTIBLE or TASK_UNINTERRUPTIBLE. Keep in mind
that msleep_interruptible() is also (hopefully) being pushed to the
kernel soon.

As to consistency or documentation . . . I have no evidence to suggest
that msleep() is inconsistent. And I don't think there is any need for
more documentation than the source in this case:

/**
 * msleep - sleep safely even with waitqueue interruptions
 * @msecs: Time in milliseconds to sleep for
 */

Hope this helps clear things up.

-Nish

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

On Thu, 02 Sep 2004, Nishanth scribeth:
> Keep in mind that msleep_interruptible() is also
> (hopefully) being pushed to the kernel soon

I think you need this for your current patch set ;-)
eg. In e100, where you replace an interruptible timeout:
> @@ -2020,8 +2016,7 @@

I don't think that's correct.

Margit

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Thu, Sep 02, 2004 at 07:52:18PM +0200, Margit Schubert-While wrote:
> On Thu, 02 Sep 2004, Nishanth scribeth:
> > Keep in mind that msleep_interruptible() is also
> > (hopefully) being pushed to the kernel soon
> 
> I think you need this for your current patch set ;-)
> eg. In e100, where you replace an interruptible timeout:
> > @@ -2020,8 +2016,7 @@
> 
> I don't think that's correct.

The reasoning for me behind changing some of the TASK_INTERRUPTIBLE'd
schedule_timeout()s to msleep()s was that LDD somewhat incorrectly
advised device driver authors to use an INTERRUPTIBLE timeout for longer
delays, when, in fact, they should probably use an UNINTERRUPTIBLE one.
Only if signals are explicitly expected to occur is INTERRUPTIBLE
necessary (in general). [By long delays, I mean those measurable in
msecs]

I am not an expert on the E100, so perhaps this was an error on my part.
But this is also why I have a header on my patch submission regarding
exactly this issue.

If someone could verify (none of the maintainers I sent the original
patch to did not reply with any problems for this patch) that there is
or is not an issue, I'd appreciate it.

-Nish

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

On Thu, 02 Sep 2004, Nishanth scribeth:
> A grep of drivers/net/wireless/prism54 for schedule_timeout showed three
> occurrences (in 2.6.9-rc1-bk7):
> islpci_dev.c:   schedule_timeout(50*HZ/1000);
> islpci_dev.c:           remaining = schedule_timeout(HZ);
> islpci_mgt.c:           timeleft = schedule_timeout(wait_cycle_jiffies);
> The first is removed by my patch.
> The second & third are potentially bugs as there is no
> set_current_state() preceding the call to schedule_timeout(). As per the
> source:

Nope, look a few lines above:
DEFINE_WAIT() and prepare_to_wait().

Margit

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

On Thu, 02 Sep 2004, Maximilian scribeth:
> you didn't answer to the unit argument in favour of msleep.

Don't need to, here's msleep -
void msleep(unsigned int msecs)
{
        unsigned long timeout = msecs_to_jiffies(msecs);

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

In other words, with the subtle exception of the while loop,
it reconstitutes the original code.
(Although m_to_j doesn't even exactly do that)
(And note because of the while loop, this may not be 
 what the author intended)

> shure msleep is also consistent and documented.
grep -r Documentation = Nix

Margit

Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Margit Schubert-While]

I agree with Jean and add the following :
You are assuming HZ = 1000.
In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).
The prism54 code base is identical between 2.4/2.6 and is
maintained as such in the project.
Therefore, I look forward to your implementation of msleep()
in 2.4  ;-)

Margit

[Kernel-janitors] Re: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[maximilian attems]

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

On Thu, 02 Sep 2004, Margit Schubert-While wrote:

> I agree with Jean and add the following :
> You are assuming HZ = 1000.
> In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).

shure, 
but this is an argument for msleep as it uses msecs as unit,
which don't depend on your arch. 
(well physics says different for arch/relativistic :)

> The prism54 code base is identical between 2.4/2.6 and is
> maintained as such in the project.
> Therefore, I look forward to your implementation of msleep()
> in 2.4  ;-)

2.4 is closed for such stuff, you'll know that better than me,
it shouldn't hinder 2.6 in it's progression.
one day you will need to branch.

--
maks
kernel janitor  	http://janitor.kernelnewbies.org/


[-- 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: [patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Thu, Sep 02, 2004 at 10:24:33AM +0200, maximilian attems wrote:
> On Thu, 02 Sep 2004, Margit Schubert-While wrote:
> 
> > I agree with Jean and add the following :
> > You are assuming HZ = 1000.
> > In 2.4, HZ = 100 (And in 2.6, HZ is not necessarily = 1000).

As the original author of the patches, I feel I should interject . . .
Why/Where do you see an assumption about the value of HZ? The conversion
of the parameter to schedule_timeout() from jiffies to msecs is the only
place I can see where that might appear to be the case. But, upon closer
examination, there is no such assumption:

1000 = the number of milliseconds in a second.
HZ = the number of jiffies in a second (regardless of architecture)

In the original code for prism54/islpci_dev.c:

set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(50*HZ/1000);

Thus, to convert (50*HZ/1000) from jiffies to msecs, multiply by 1000
and divide by HZ, or:

50*HZ/1000 jiffies * 1000/HZ msecs/jiffie = 50 msecs.

And thus, in the patched code, the above becomes:

msleep(50);

Does that clear things up?

-Nish

[patch 8/8] prism54/islpci_dev: replace schedule_timeout() with msleep()

[janitor]

I would appreciate any comments from the janitor@sternweltens list. 



Description: Use msleep() instead of schedule_timeout() to guarantee
the task delays for the desired time.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Signed-off-by: Maximilian Attems <janitor@sternwelten.at>



---

 linux-2.6.9-rc1-bk7-max/drivers/net/wireless/prism54/islpci_dev.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

diff -puN drivers/net/wireless/prism54/islpci_dev.c~msleep-drivers_net_wireless_prism54_islpci_dev drivers/net/wireless/prism54/islpci_dev.c
--- linux-2.6.9-rc1-bk7/drivers/net/wireless/prism54/islpci_dev.c~msleep-drivers_net_wireless_prism54_islpci_dev	2004-09-01 19:35:41.000000000 +0200
+++ linux-2.6.9-rc1-bk7-max/drivers/net/wireless/prism54/islpci_dev.c	2004-09-01 19:35:41.000000000 +0200
@@ -436,8 +436,7 @@ prism54_bring_down(islpci_private *priv)
 	wmb();
 
 	/* wait a while for the device to reset */
-	set_current_state(TASK_UNINTERRUPTIBLE);
-	schedule_timeout(50*HZ/1000);
+	msleep(50);
 
 	return 0;
 }

_