On Tuesday 27 June 2006 04:27, Larry Finger wrote:
> Jeff Garzik wrote:
> > John W. Linville wrote:
> >> + assert(bcm->mac_suspended >= 0);
> >> + if (bcm->mac_suspended == 0) {
> >> + bcm43xx_power_saving_ctl_bits(bcm, -1, 1);
> >> + bcm43xx_write32(bcm, BCM43xx_MMIO_STATUS_BITFIELD,
> >> + bcm43xx_read32(bcm,
> >> BCM43xx_MMIO_STATUS_BITFIELD)
> >> + & ~BCM43xx_SBF_MAC_ENABLED);
> >> + bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON); /* dummy
> >> read */
> >> + for (i = 100000; i; i--) {
> >> + tmp = bcm43xx_read32(bcm, BCM43xx_MMIO_GEN_IRQ_REASON);
> >> + if (tmp & BCM43xx_IRQ_READY)
> >> + goto out;
> >> + udelay(10);
> >> + }
> >> + printkl(KERN_ERR PFX "MAC suspend failed\n");
> >> }
> >
> >
> > NAK this super-long delay... should be done in a workqueue, looks like?
> >
> > ACK everything else.
> >
>
> That delay was set to try to accommodate my interface when it refused to
> suspend the MAC, which
> resulted in transmit errors. That problem has since been cured by reworking
> the periodic work
> handlers - thus such a long delay should not be needed. The original spec
> from the clean-room group
> was a delay loop of 1000. I'm currently testing that value now. If it passes
> the test, would a for
> (i=1000; i; i--) be acceptable?
Short: Don't touch it. Fullstop.
Long: The delay will _never_ be exhausted. Actually the for-counter
is only there to not lock up the machine, if there is a Bug in the
driver. (__much__ easier debugging).
The loop will only iterate a few times, typically.
Actually, _if_ we want to change something, we should do this:
for (i = 1000000; i; i--) {
...
udelay(1);
}
(max loop multiplied by 10, delay value divided by 10).
This will shorten the whole delay by a few usecs (up to 10).
I will send a patch for this, if it is desired.
But lowering the loop counter value is NACKed by me,
because it simply does not make sense.
--
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
