ath5k: ath5k_pci_probe(): weirdo code

ath5k: ath5k_pci_probe(): weirdo code

[Andreas Mohr]

Hello all,

that 2GHz/5GHz radio information code in ath5k_pci_probe() in 2.6.31-rc6 source
seems VERY weird.
ah_radio_5ghz_revision/ah_radio_2ghz_revision conditional evaluation seems
completely bonkered in several places, unless something is escaping on
me.
Then also at this place, why is there no
else
{
  WARN_ONCE(...); // Huh. Unknown/unhandled/impossible revision combo
}
branch at the end? (or something less incinerating)
I've had my few lessons about defensive programming...
(and applying such stuff liberally in this entire driver wouldn't hurt either)


The

        if (!sc->ah->ah_single_chip) {
                /* Single chip radio (!RF5111) */

part seemed weird logic, too, but I then thought that this code section
is perhaps _intended_ to be skipped in this case, to simply not log anything
for single-chip radios
(but the comment above would be very insufficient/confusing if that is
the case, and if the entire weird section is _actually_ correct as it stands
then even lots more rectifying comments about this strange evaluation
being correct are missing).

Could anyone enlighten me?
So far it looks like a large part of this code is buggy, but I could be
wrong.



BTW, I'm currently on 2.6.31-rc6 on Aspire One, with even more ath5k problems
than before, connection is dying every couple minutes, extended to every
couple dozen minutes if I force rate 1M
(but it looks like "[Bug #13948] ath5k broken after suspend-to-ram"
http://lkml.org/lkml/2009/8/19/468 is hopefully covering these issues already).
And I do have CONFIG..._PS (powersave) enabled by default...
And I do make liberal use of suspend-to-ram, as should everyone.

Thanks,

Andreas Mohr

Re: ath5k: ath5k_pci_probe(): weirdo code

[Bob Copeland]

On Fri, Aug 21, 2009 at 11:47:32AM +0200, Andreas Mohr wrote:
> Hello all,
> 
> that 2GHz/5GHz radio information code in ath5k_pci_probe() in 2.6.31-rc6
> source seems VERY weird.

The code looks horribly confusing, it should be moved into attach.c
and reworked a lot probably.  It looks correct though - it is fixing up
settings for (really old) multiple radio chip devices, where we can't
tell by the radio revision alone if it is multiband or not.

ah_radio_{2,5}ghz_revision is what is in the appropriate registers in
the card, but e.g. the revision in the 5ghz radio register may actually
refer to the 2ghz radio on some 802.11B-only devices, depending on what
is in the eeprom.

> Then also at this place, why is there no
> else
> {
>   WARN_ONCE(...); // Huh. Unknown/unhandled/impossible revision combo

Should be caught already in attach.c, but yeah, another warning wouldn't
hurt.

>         if (!sc->ah->ah_single_chip) {
>                 /* Single chip radio (!RF5111) */
> 
> part seemed weird logic, too, but I then thought that this code section
> is perhaps _intended_ to be skipped in this case, to simply not log anything
> for single-chip radios

The comment belongs to the subsequent if() test, where it should probably
live to be clearer.  All recent devices (including yours) are now single
chip so this code wouldn't run on your device.

> BTW, I'm currently on 2.6.31-rc6 on Aspire One, with even more ath5k problems
> than before, connection is dying every couple minutes, extended to every
> couple dozen minutes if I force rate 1M

Can you supply /debug/ieee80211/phy0/stations/<...>/rc_stats?

> (but it looks like "[Bug #13948] ath5k broken after suspend-to-ram"
> http://lkml.org/lkml/2009/8/19/468 is hopefully covering these issues already).

Can you verify that the patch mentioned in the bug report works?
We need to queue that for 2.6.31.

> And I do have CONFIG..._PS (powersave) enabled by default...
> And I do make liberal use of suspend-to-ram, as should everyone.

As do I.  I don't have a 2425 though, and there are quite some variations
between each HW revision.

-- 
Bob Copeland %% www.bobcopeland.com

Re: ath5k: ath5k_pci_probe(): weirdo code

[Andreas Mohr]

Hi,

On Sat, Aug 22, 2009 at 09:35:57AM -0400, Bob Copeland wrote:
> On Fri, Aug 21, 2009 at 11:47:32AM +0200, Andreas Mohr wrote:
> > Hello all,
> > 
> > that 2GHz/5GHz radio information code in ath5k_pci_probe() in 2.6.31-rc6
> > source seems VERY weird.

> ah_radio_{2,5}ghz_revision is what is in the appropriate registers in
> the card, but e.g. the revision in the 5ghz radio register may actually
> refer to the 2ghz radio on some 802.11B-only devices, depending on what
> is in the eeprom.

Ah, that explains why the logic was rather counter-intuitive.
Still, in OSS it is very common to have _other_ people modify code,
and with that kind of clarity things can thus go haywire easily
(unless a core maintainer happens to catch it after submission).

> Can you verify that the patch mentioned in the bug report works?

YES, I can verify that replacing those 2 §"§$%>: Hermei capacitors
with good Rubycon ones in one of my three WRT54G(S) worked. :-P
(global series failure, "Defective By Design _without_ even needing
to make use of DRM"). aka "Planned failure one year post warranty" ;)

It seems MUCH more reliable now, after some short testing...

> Can you supply /debug/ieee80211/phy0/stations/<...>/rc_stats?

[NOTE: not sure whether these ath5k module stats are (partially) still pre-repair]

root@andinet:/sys/kernel/debug/ieee80211/phy19/stations/00:0f:66:4c:c4:44# cat rc_stats

                                         ^^^^^ *wink* ;-)

rate     throughput  ewma prob   this prob  this succ/attempt   success    attempts
     1         0.9       97.6      100.0          0(  0)         20          20
     2         0.4       25.0      100.0          0(  0)          1           1
     5.5       0.0        0.0        0.0          0(  0)          0           0
    11         9.1       95.5      100.0          0(  0)         78          82
     6         0.0        0.0        0.0          0(  0)          0           0
     9         0.0        0.0        0.0          0(  0)          0           0
    12         2.7       25.0      100.0          0(  0)          1           1
    18         4.0       25.0      100.0          0(  0)          1           1
    24         5.3       25.0      100.0          0(  0)          1           1
    36         7.6       25.0      100.0          0(  0)          1           1
 t  48         7.0       77.6       50.0          0(  0)       1543        1685
T P 54        20.0       99.9      100.0          1(  1)     357588      398612

Total packet count::    ideal 8186      lookaround 909

> We need to queue that for 2.6.31.

Cannot test the patch right now since I cannot do a kernel build at this place.

> As do I.  I don't have a 2425 though, and there are quite some variations
> between each HW revision.

Which is quite the norm with todays hardware :-P
(I even once bought an ACX100 hardware with the wrong revision
- read: COMPLETELY different hardware - where I then found that the seller
had _both_ revision variants in the _SAME_ shelf box at the same time
and they were friggin' out themselves when I enlightened them about this fact...)
Not to mention the even much more HORRIBLE CRIME of assigning the SAME PCI ID even,
to DIFFERENT hardware, that some vendors dare to commit.

Andreas Mohr