On Tuesday 27 June 2006 21:33, John W. Linville wrote:
> On Tue, Jun 27, 2006 at 06:31:01PM +0200, Michael Buesch wrote:
> > On Tuesday 27 June 2006 18:12, Jeff Garzik wrote:
> > > Michael Buesch wrote:
> > > > So, I will submit a patch to lower the udelay(10) to udelay(1)
> > > > and we can close the discussion? ;)
> > >
> > > No, that totally avoids my point. Your "otherwise idle machine" test is
> > > probably nowhere near worst case in the field, for loops that can
> > > potentially lock the CPU for a long time upon hardware fault. And then
> > > there are the huge delays in specific functions that I pointed out...
> >
> > wtf are you requesting from me?
> > 1) I proved you that the loop does only spin _once_ or even _less_.
> > 2) If the hardware is faulty, the user must replace it.
> > Because, if the hardware is faulty, it can crash the whole
> > machine anyway, obviously.
> >
> > 3) There is no "huge delay". I proved it with my logs.
> > -> No CPU hog => Nothing to fix.
>
> Michael,
>
> I think Jeff's concern is that by using udelay you are busy-waiting.
> And, the for loop limit of 100000 means you could freeze the kernel
> for up to a whole second. Granted that this won't happen very often
s/very often/ever/
It won't happen, as long as the driver is not buggy, or the device
is hardware broken. So, if it happens, something has to be fixed.
In fact, it did happen _never_ for me.
If it triggers, the device does not work _at all_ anyway.
> and in the grand scheme of things a second isn't all _that_ long,
> but still it would be better to avoid a delay like that -- a second
> could be the time it takes to avoid a meltdown at the nuclear power
> plant. :-)
>
> Could you not use msleep instead of udelay (and scale the for loop
> appropriately)? What would be the problem with that? It would get
> rid of the busy waiting.
Becauses it horribly _increases_ the delay.
We "spin" for _at most_ 10 usecs here. Please always remember that.
We are talking about a 10 usec delay here. And I already sent a
patch to even reduce this to under 10 usec.
> To be fair, this code was already in the driver and was only being
> moved by this patch. Still, what better time to fix it than now? :-)
If it ain't broken, don't fix it.
> I'll go ahead and reshuffle wireless-2.6 to drop this patch. A new
> patch that passes muster w/ Jeff will be most welcome! :-)
A new patch won't appear, as there is no problem with this
delay.
Please don't drop anything and apply the following patch on top
of it:
--
Microoptimization:
This reduces the udelay in bcm43xx_mac_suspend.
Signed-off-by: Michael Buesch <[EMAIL PROTECTED]>
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c
2006-06-27 17:47:24.000000000 +0200
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c 2006-06-27
17:53:29.000000000 +0200
@@ -2328,7 +2328,7 @@
tmp = bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON);
if (tmp & BCM43xx_IRQ_READY)
goto out;
- udelay(10);
+ udelay(1);
}
printkl(KERN_ERR PFX "MAC suspend failed\n");
}
--
Greetings Michael.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
