net/pcnet32: replace

Any comments would be appreciated.

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

@@ -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 */

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors