Bug 13288

Created attachment 21318 [details]
Debugging output from madwifi driver

Created attachment 21319 [details]
Output of ath_info for the card

Thanks can you also post lspci -vnn?  LED handling is implemented in led.c as of 2.6.30.

> Thanks can you also post lspci -vnn?

02:00.0 Ethernet controller [0200]: Atheros Communications Inc. AR5212/AR5213 Multiprotocol MAC/baseband processor [168c:0013] (rev 01)
        Subsystem: Global Sun Technology Inc Device [16ab:7103]
        Flags: bus master, medium devsel, latency 168, IRQ 18
        Memory at 30000000 (32-bit, non-prefetchable) [size=64K]
        Capabilities: [44] Power Management version 2
        Kernel driver in use: ath5k
        Kernel modules: ath_pci, ath5k

> LED handling is implemented in led.c as of 2.6.30.

Yes, I saw that file as well but wondered how relevant that was for my PCMCIA card. I noticed it registers "rx" and "tx" leds for sysfs, which does not match the naming of the leds I actually have on the device: "power" and "link".

Also, that ath5k_hw_set_ledstate() function has never been used AFAICT. I checked back to 2.6.25.

I also wonder if it really should be needed to add each and every PCI ID in led.c before leds can be supported. The madwifi driver seems to be able to do so correctly with generic functions based on device versions.
Possibly some exceptions are needed for systems (laptops) with the device built in, but otherwise (from an outsider PoV and without any knowledge of the hardware) listing all PCI IDs seems like an expensive option and also means it will take ages to get all hardware covered.

Thanks,
FJP

P.S. The wireless connection is more stable with ath5k than with madwifi. In the exact same conditions I see occasional lost packets with the latter. So kudos for that.

(In reply to comment #4)
> > Thanks can you also post lspci -vnn?
> > LED handling is implemented in led.c as of 2.6.30.
> 
> Yes, I saw that file as well but wondered how relevant that was for my PCMCIA
> card. I noticed it registers "rx" and "tx" leds for sysfs, which does not
> match
> the naming of the leds I actually have on the device: "power" and "link".

True, this is rather arbitrary since you can repurpose them with the
sysfs trigger files.  But anyway, it sounds like your device is using
hardware LEDs which are indeed handled separately from the led.c code
(or rather, aren't handled at all at the moment).  I'll see if I can
cook up a patch today for it.

> in, but otherwise (from an outsider PoV and without any knowledge of the
> hardware) listing all PCI IDs seems like an expensive option and also means
> it
> will take ages to get all hardware covered.
> 

Agreed -- but these are all devices with sw LEDs, and as far as Atheros has
told us, the relevant GPIOs aren't stored in the EEPROM anywhere.  Madwifi
just used a modparam for this.

Created attachment 21668 [details]
use hardware LEDs

Can you try this patch?  Specifically I'd like to know (since I don't have the HW) whether power LED comes on appropriately, whether it works in general, and whether it does the right thing for suspend/resume and shutdown.

Thanks a lot Bob, works great!

Associating (alternate blink) and associated (both blink) states, as well as net down (power only) and driver removed (power blink) states show correctly.

Boot and shutdown are good. Suspend to RAM also looks good (I also checked wireless disappearing while suspended).

Please add my:
Tested-by: Frans Pop <elendil@planet.nl>

There are a few minor differences with the madwifi driver, but AFAICT those are mostly due to differences in how connections are done (for example: the madwifi driver will associate with my AP after 'ifconfig wlan0 up', while the ath5k driver does not; both allow 'iwlist wlan0 scan' at that point).

One inconsistency, but not sure if that's worth a lot of effort. Take the following sequence, starting with interface up and associated (both leds blinking):
$ ifdown wlan0
  power led fixed on, link led off; 'iwlist scan' does not work (net down)
$ ifconfig wlan0 up
  No change; 'iwlist scan' works (power led shows activity during scan)
$ ifconfig wlan0 down
  power led blinks, link led off; 'iwlist scan' does not work (net down)

Question is why there's a difference in the power led (on versus blinking) after the first and third commands? In both cases the network is down.
With the madwifi driver the blinking state only shows when the driver is removed; after 'ifconfig wlan0 down' the power led stays on.

Thanks again,
FJP

P.S. My userland is Debian stable (Lenny).

(In reply to comment #7)
> $ ifdown wlan0
>   power led fixed on, link led off; 'iwlist scan' does not work (net down)
> $ ifconfig wlan0 up
>   No change; 'iwlist scan' works (power led shows activity during scan)
> $ ifconfig wlan0 down
>   power led blinks, link led off; 'iwlist scan' does not work (net down)

Hmm, perhaps we don't get a notification that the scan is complete
when the interface is brought down?  You can try adding:

	ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);

to base.c right after ath5k_led_off(sc) in ath5k_stop_locked (line 2364
in my copy).

(This turns off blinking, we could force it off but it sounds like
the hw already does the right thing when the card is powered down.)

>> $ ifconfig wlan0 down
>>   power led blinks, link led off; 'iwlist scan' does not work (net down)
>
> Hmm, perhaps we don't get a notification that the scan is complete
> when the interface is brought down?

I think you misunderstood. The "blinking" is not the "quick blinking to show there is activity", but the slow blink that seems to correspond to something like "card has power, but no driver is loaded".

So, the fact that a scan is complete when the interface is up but not associated shows correctly (the power led blinks while the scan is in progress and then returns to fixed on).

It is rather that after 'ifconfig wlan0 down' the led switches to slow blink. The question is whether _that_ is correct or not, especially given that after the similar 'ifdown wlan0' it stays on.
I'm not sure though. Maybe 'ifconfig wlan0 down' really does result in a different ("lower"?) state than 'ifdown wlan0'.

Hmm. That's strange. The final change of the led state does not seem to originate in the driver, or at least not in ath5k_hw_set_ledstate(). I added a simple printk there, and this is the result (interface is renamed to ath0; values in hex).

# ifup ath0
kernel: ADDRCONF(NETDEV_UP): ath0: link is not ready
kernel: FJP: changing ath5k leds: state 1, value 20
kernel: FJP: changing ath5k leds: state 0, value 0
kernel: ath0: authenticate with AP 00:14:c1:38:e5:15
kernel: ath0: authenticated
kernel: ath0: associate with AP 00:14:c1:38:e5:15
kernel: ath0: RX AssocResp from 00:14:c1:38:e5:15 (capab=0x411 status=0 aid=2)
kernel: ath0: associated
kernel: FJP: changing ath5k leds: state 3, value 40
kernel: ADDRCONF(NETDEV_CHANGE): ath0: link becomes ready
kernel: ath0: no IPv6 routers present

# ifdown ath0
kernel: ath0: deauthenticating by local choice (reason=3)
kernel: FJP: changing ath5k leds: state 0, value 0

# ifconfig ath0 up
kernel: FJP: changing ath5k leds: state 1, value 20
kernel: ADDRCONF(NETDEV_UP): ath0: link is not ready
kernel: FJP: changing ath5k leds: state 0, value 0
kernel: FJP: changing ath5k leds: state 1, value 20
kernel: FJP: changing ath5k leds: state 0, value 0

# ifconfig ath0 down

So, after the last command there is no output from my printk, but still the power led changes from "on" to "slow blink".


From this debugging it also looks as if ath5k sets slightly different values from the madwifi driver when I compare with the madwifi debug log (see first attachment in this BR): 0x00 versus 0x10; 0x20 vs. 0x30; 0x40 vs. 0x50.
I haven't checked yet what the difference means.

During the 'ifup' it also resets the state to 0x00 after a scan instead of going directly to "associated" if the association is complete, which makes for a slightly messy transfer. The madwifi driver seems to leave the leds in the "associating" state until it is known if it was successful or not.

> From this debugging it also looks as if ath5k sets slightly different values
> from the madwifi driver when I compare with the madwifi debug log (see first
> attachment in this BR): 0x00 versus 0x10; 0x20 vs. 0x30; 0x40 vs. 0x50.
> I haven't checked yet what the difference means.

If I read things correctly, the madwifi driver always sets bit 4 which, according to ar5212_reg.h, is "s/w control + both leds off". Apparently that can safely be combined with also setting bits 5 and 6:
#define AR_PCICFG_LEDMODE       0x000E0000 /* LED mode */
#define AR_PCICFG_LEDMODE_PROP  0          /* Blink prop to filtered tx/rx */
#define AR_PCICFG_LEDMODE_RPROP 1          /* Blink prop to unfiltered tx/rx */
#define AR_PCICFG_LEDMODE_SPLIT 2          /* Blink power for tx/net for rx */
#define AR_PCICFG_LEDMODE_RAND  3          /* Blink randomly */
/* NB: s/w led control present in Hainan 1.1 and above */
#define AR_PCICFG_LEDMODE_OFF   4          /* s/w control + both led's off */
#define AR_PCICFG_LEDMODE_POWON 5          /* s/w control + power led on */
#define AR_PCICFG_LEDMODE_NETON 6          /* s/w control + network led on */


Could that also explain the difference after 'ifconfig down'? That the led changes because the leds are no longer under software control as it was last set to 0x00?

Yeah the way it works is there are 3 bits that set the LED mode, the bottom two set the blink mode and the top bit allows the driver to bitbang the power and network LEDs (when it is set, the PROP/RPROP/SPLIT etc have no effect, instead the bottom two bits set the state of the power/net LEDs -- this is what madwifi means by s/w control).  I just enabled what was already there but unused, ath5k doesn't currently use the s/w control.

Then there are another pair of bits to set the connection state (and therefore blink pattern) which are LED_NONE, LED_PEND, LED_ASSOC.  I currently set it up to be in PEND when we are scanning, ASSOC when associated, and NONE otherwise.  The problem is I don't think we know from a driver's standpoint when we are trying to associate since all of that is handled by the upper layers; that is why it reverts to NONE between scanning and associating.

Also, taking down the last interface powers down the chip which is probably why you get slow blink on the last ifdown.

Re comment #12:
Thanks for the explanation. As I mentioned before, it works well
enough in practice to not worry too much about this. Just thought
I'd point out the differences.

Re comment #13:
> Also, taking down the last interface powers down the chip
> which is probably why you get slow blink on the last ifdown.

I'm still confused how that is different from the initial ifdown,
which also takes down the interface...

Yeah, I don't know what the userspace does in ifdown, it(In reply to comment #14)
> Re comment #12:
> Thanks for the explanation. As I mentioned before, it works well
> enough in practice to not worry too much about this. Just thought
> I'd point out the differences.

Ok, I'll go ahead and submit the patch (with a slight change, to not
blink when scanning while associated, as suggested on the mailing list).

> I'm still confused how that is different from the initial ifdown,
> which also takes down the interface...

Yeah it would be interesting to know what the userspace is doing
differently in that case.

Hi,

What's the status of Bob's patch re. inclusion in 2.6.31? I've been following the merge requests for wireless, but so far I've not seen it.
Anything I can do?

TIA,
FJP

Bob, have you sent that upstream?  In any case, I'm not sure it merits "bug/regression fix" status needed for 2.6.31...

Yeah, it's 44553a1f1966fdd77dab68f67a46926283dd357c in wireless-testing - I think it was too late to make the merge window.

On Saturday 20 June 2009, you wrote:
> Yeah, it's 44553a1f1966fdd77dab68f67a46926283dd357c in
> wireless-testing - I think it was too late to make the merge window.

Maybe you have your own merge window for wireless, but the patch was ready 
on June 1. Linus' only opened the merge window for .31 on June 10, so 
from that perspective there's still 4 days left.

As the change is relatively simple and mostly cosmetic (low risk) it would 
IMHO be a pity to delay it for another 2/3 months, but it is your call of 
course.

Thanks for the replies.

I believe this bug can be closed now, it should be in 2.6.32.

Agreed and done.
For reference: commit ID is f0f3d388baabdbc613548d6ad8e5da7616b1cbd1.