On Tuesday 27 June 2006 16:11, Jeff Garzik wrote:
> Michael Buesch wrote:
> > 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.
>
>
> My overriding concern was that this type of loop spins the CPU at 100%
> until the hardware condition is satisfied, which starves all other
> kernel work on that CPU, and is very unfriendly to power consumption
> (though I believe monitor/mwait/cpu_relax helps on x86).
Ok, I did a testrun. Here's the result:
[ 68.711243] bcm43xx_d80211: Chip ID 0x4306, rev 0x3
[ 68.712662] bcm43xx_d80211: Number of cores: 5
[ 68.714023] bcm43xx_d80211: Core 0: ID 0x800, rev 0x4, vendor 0x4243, enabled
[ 68.715536] bcm43xx_d80211: Core 1: ID 0x812, rev 0x5, vendor 0x4243,
disabled
[ 68.717062] bcm43xx_d80211: Core 2: ID 0x80d, rev 0x2, vendor 0x4243, enabled
[ 68.718575] bcm43xx_d80211: Core 3: ID 0x807, rev 0x2, vendor 0x4243,
disabled
[ 68.720089] bcm43xx_d80211: Core 4: ID 0x804, rev 0x9, vendor 0x4243, enabled
[ 68.724897] bcm43xx_d80211: PHY connected
[ 68.726154] bcm43xx_d80211: Detected PHY: Version: 2, Type 2, Revision 2
[ 68.727638] bcm43xx_d80211: Detected Radio: ID: 2205017f (Manuf: 17f Ver:
2050 Rev: 2)
[ 68.729232] bcm43xx_d80211: Radio turned off
[ 68.730512] bcm43xx_d80211: Radio turned off
[ 68.745153] wmaster0: Selected rate control algorithm 'simple'
[ 69.853876] bcm43xx_d80211: Virtual interface added (type: 0x00000002, ID:
7, MAC: 00:11:24:a0:de:8b)
[ 69.861872] bcm43xx_d80211: PHY connected
[ 70.000713] bcm43xx_d80211: Radio turned on
[ 70.190153] bcm43xx_d80211: Chip initialized
[ 70.192051] bcm43xx_d80211: DMA initialized
[ 70.193987] bcm43xx_d80211: 80211 cores initialized
[ 70.195550] bcm43xx_d80211: Keys cleared
[ 70.212565] wmaster0: Does not support passive scan, disabled
[ 70.252321] bcm43xx_d80211: mac_suspend() took 1 loops == 10 usec
[ 70.542160] NET: Registered protocol family 17
[ 70.692256] sta0: starting scan
[ 70.702234] HW CONFIG: channel=1 freq=2412 phymode=3
[ 70.762225] HW CONFIG: channel=2 freq=2417 phymode=3
[ 70.822227] HW CONFIG: channel=3 freq=2422 phymode=3
[ 70.882225] HW CONFIG: channel=4 freq=2427 phymode=3
[ 70.942225] HW CONFIG: channel=5 freq=2432 phymode=3
[ 71.002285] HW CONFIG: channel=6 freq=2437 phymode=3
[ 71.062229] HW CONFIG: channel=7 freq=2442 phymode=3
[ 71.122230] HW CONFIG: channel=8 freq=2447 phymode=3
[ 71.182239] HW CONFIG: channel=9 freq=2452 phymode=3
[ 71.242226] HW CONFIG: channel=10 freq=2457 phymode=3
[ 71.302226] HW CONFIG: channel=11 freq=2462 phymode=3
[ 71.512225] HW CONFIG: channel=1 freq=2412 phymode=3
[ 71.520066] sta0: scan completed
[ 71.523437] HW CONFIG: channel=6 freq=2437 phymode=3
[ 71.531600] sta0: Initial auth_alg=0
[ 71.531806] sta0: authenticate with AP 00:90:4c:60:04:00
[ 71.533279] sta0: RX authentication from 00:90:4c:60:04:00 (alg=0
transaction=2 status=0)
[ 71.533292] sta0: authenticated
[ 71.533301] sta0: associate with AP 00:90:4c:60:04:00
[ 71.536264] sta0: RX AssocResp from 00:90:4c:60:04:00 (capab=0x411 status=0
aid=1)
[ 71.536276] sta0: associated
[ 71.536352] wmaster0: Added STA 00:90:4c:60:04:00
[ 130.292251] bcm43xx_d80211: mac_suspend() took 1 loops == 10 usec
[ 190.322251] bcm43xx_d80211: mac_suspend() took 1 loops == 10 usec
[ 250.362252] bcm43xx_d80211: mac_suspend() took 0 loops == 0 usec
[ 310.392278] bcm43xx_d80211: mac_suspend() took 1 loops == 10 usec
So, I will submit a patch to lower the udelay(10) to udelay(1)
and we can close the discussion? ;)
--
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
