On Tuesday 27 June 2006 21:33, John W. Linville wrote:

On Tue, Jun 27, 2006 at 06:31:01PM +0200, Michael Buesch wrote:

quoted

On Tuesday 27 June 2006 18:12, Jeff Garzik wrote:

quoted

Michael Buesch wrote:

quoted

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

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.