* Re: [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace
2004-09-24 18:08 [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace Nishanth Aravamudan
@ 2004-09-24 18:21 ` Matthew Wilcox
2004-09-24 18:33 ` Matthew Wilcox
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2004-09-24 18:21 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace
2004-09-24 18:08 [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace Nishanth Aravamudan
2004-09-24 18:21 ` Matthew Wilcox
@ 2004-09-24 18:33 ` Matthew Wilcox
2004-09-24 18:40 ` Nishanth Aravamudan
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2004-09-24 18:33 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace
2004-09-24 18:08 [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace Nishanth Aravamudan
2004-09-24 18:21 ` Matthew Wilcox
2004-09-24 18:33 ` Matthew Wilcox
@ 2004-09-24 18:40 ` Nishanth Aravamudan
2004-09-24 18:44 ` Matthew Wilcox
2004-09-24 18:46 ` Nishanth Aravamudan
4 siblings, 0 replies; 6+ messages in thread
From: Nishanth Aravamudan @ 2004-09-24 18:40 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace
2004-09-24 18:08 [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace Nishanth Aravamudan
` (2 preceding siblings ...)
2004-09-24 18:40 ` Nishanth Aravamudan
@ 2004-09-24 18:44 ` Matthew Wilcox
2004-09-24 18:46 ` Nishanth Aravamudan
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2004-09-24 18:44 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace
2004-09-24 18:08 [Kernel-janitors] [PATCH 2.6.9-rc2 28/38] net/pcnet32: replace Nishanth Aravamudan
` (3 preceding siblings ...)
2004-09-24 18:44 ` Matthew Wilcox
@ 2004-09-24 18:46 ` Nishanth Aravamudan
4 siblings, 0 replies; 6+ messages in thread
From: Nishanth Aravamudan @ 2004-09-24 18:46 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread