[Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace

[Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace

[Nishanth Aravamudan]

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

Any comments would be appreciated.

Description: Use ssleep() instead of schedule_timeout()
to guarantee the task delays as expected.

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


--- 2.6.9-rc1-mm4-vanilla/drivers/net/pcnet32.c	2004-09-09 23:05:32.000000000 -0700
+++ 2.6.9-rc1-mm4/drivers/net/pcnet32.c	2004-09-13 14:38:47.000000000 -0700
@@ -847,7 +847,7 @@ static int pcnet32_phys_id(struct net_de
     if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ)))
     data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
 
-    schedule_timeout(data * HZ);
+    msleep_interruptible(data * 1000);
     del_timer_sync(&lp->blink_timer);
 
     /* Restore the original value of the bcrs */

[-- 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 2.6.9-rc2 28/38] net/pcnet32: replace

[Matthew Wilcox]

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

On Fri, Sep 24, 2004 at 11:08:25AM -0700, Nishanth Aravamudan wrote:
> Any comments would be appreciated.
> +++ 2.6.9-rc1-mm4/drivers/net/pcnet32.c	2004-09-13 14:38:47.000000000 -0700
> @@ -847,7 +847,7 @@ static int pcnet32_phys_id(struct net_de
>      if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ)))
>      data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
>  
> -    schedule_timeout(data * HZ);
> +    msleep_interruptible(data * 1000);
>      del_timer_sync(&lp->blink_timer);

Surely that needs to be:

	if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / 1000))
		data = (u32)(MAX_SCHEDULE_TIMEOUT / 1000)
	msleep_interruptible(data * 1000);

(and actually, this should be pulled into net/core/ethtool.c out of each
driver).

-- 
"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 2.6.9-rc2 28/38] net/pcnet32: replace

[Matthew Wilcox]

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

On Fri, Sep 24, 2004 at 07:21:13PM +0100, Matthew Wilcox wrote:
> Surely that needs to be:
> 
> 	if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / 1000))
> 		data = (u32)(MAX_SCHEDULE_TIMEOUT / 1000)
> 	msleep_interruptible(data * 1000);
> 
> (and actually, this should be pulled into net/core/ethtool.c out of each
> driver).

A little further thought ...

unsigned long msleep_interruptible(unsigned int msecs)
{
       unsigned long timeout = msecs_to_jiffies(msecs);

       while (timeout && !signal_pending(current)) {
               set_current_state(TASK_INTERRUPTIBLE);
               timeout = schedule_timeout(timeout);
       }
       return jiffies_to_msecs(timeout);
}

There's no guarantee that there is a value in ms that converts to
MAX_SCHEDULE_TIMEOUT.  Do we need an MSEC_SCHEDULE_TIMEOUT so we can
do something like:

unsigned long msleep_interruptible(unsigned int msecs)
{
       unsigned long timeout = (msecs == MSEC_SCHEDULE_TIMEOUT) ?
		MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(msecs);
	...
}

I'd be loathe to put this in the msec_to_jiffies macro itself since it
isn't always used in an appropriate way...

-- 
"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 2.6.9-rc2 28/38] net/pcnet32: replace

[Nishanth Aravamudan]

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

On Fri, Sep 24, 2004 at 07:21:13PM +0100, Matthew Wilcox wrote:
> On Fri, Sep 24, 2004 at 11:08:25AM -0700, Nishanth Aravamudan wrote:
> > Any comments would be appreciated.
> > +++ 2.6.9-rc1-mm4/drivers/net/pcnet32.c	2004-09-13 14:38:47.000000000 -0700
> > @@ -847,7 +847,7 @@ static int pcnet32_phys_id(struct net_de
> >      if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ)))
> >      data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
> >  
> > -    schedule_timeout(data * HZ);
> > +    msleep_interruptible(data * 1000);
> >      del_timer_sync(&lp->blink_timer);
> 
> Surely that needs to be:
> 
> 	if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / 1000))
> 		data = (u32)(MAX_SCHEDULE_TIMEOUT / 1000)
> 	msleep_interruptible(data * 1000);
> 
> (and actually, this should be pulled into net/core/ethtool.c out of each
> driver).

I agree - the tabbing in that whole driver is non-standard (looks to be
4 spaces). And some places simply aren't tabbed like this one.

[-- 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 2.6.9-rc2 28/38] net/pcnet32: replace

[Matthew Wilcox]

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

On Fri, Sep 24, 2004 at 11:40:42AM -0700, Nishanth Aravamudan wrote:
> > >      if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ)))
> > >      data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
> > 	if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / 1000))
> > 		data = (u32)(MAX_SCHEDULE_TIMEOUT / 1000)
> I agree - the tabbing in that whole driver is non-standard (looks to be
> 4 spaces). And some places simply aren't tabbed like this one.

It wasn't the tabbing, but the divisor that I was complaining about.

-- 
"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 2.6.9-rc2 28/38] net/pcnet32: replace

[Nishanth Aravamudan]

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

On Fri, Sep 24, 2004 at 07:21:13PM +0100, Matthew Wilcox wrote:
> On Fri, Sep 24, 2004 at 11:08:25AM -0700, Nishanth Aravamudan wrote:
> > Any comments would be appreciated.
> > +++ 2.6.9-rc1-mm4/drivers/net/pcnet32.c	2004-09-13 14:38:47.000000000 -0700
> > @@ -847,7 +847,7 @@ static int pcnet32_phys_id(struct net_de
> >      if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ)))
> >      data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
> >  
> > -    schedule_timeout(data * HZ);
> > +    msleep_interruptible(data * 1000);
> >      del_timer_sync(&lp->blink_timer);
> 
> Surely that needs to be:
> 
> 	if ((!data) || (data > (u32)(MAX_SCHEDULE_TIMEOUT / 1000))
> 		data = (u32)(MAX_SCHEDULE_TIMEOUT / 1000)
> 	msleep_interruptible(data * 1000);

Beyond the tabbing issue I already replied to (which I now realize
wasn't really what you were referring to :) ), there is this problem:

I seems data is in terms of seconds. MAX_SCHEDULE_TIMEOUT is in terms
of jiffies, so dividing it by 1000 to determine if the requested timeout
is longer than the longest possible timeout would not work, I don't
think.

The code should work as is, because the if checks to see if
either data is 0 or greater than the maximum allowed number of seconds
to sleep. In either case, data gets set to the maximum number of seconds
to sleep. Then msleep_interruptible will sleep for the corresponding
number of msecs :)

I think it all works itself out...

-Nish

[-- 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