2008-01-21 22:09:07

by John W. Linville

[permalink] [raw]
Subject: [PATCH] ath5k: use AR5K_KEYTABLE_SIZE when initializing key table

...instead of using AR5K_KEYCACHE_SIZE, which would seem to be a
typo/thinko...

Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 742616a..75e5970 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -663,7 +663,7 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
* Reset the key cache since some parts do not
* reset the contents on initial power up.
*/
- for (i = 0; i < AR5K_KEYCACHE_SIZE; i++)
+ for (i = 0; i < AR5K_KEYTABLE_SIZE; i++)
ath5k_hw_reset_key(ah, i);

/*
--
1.5.3.3



2008-01-21 22:09:17

by John W. Linville

[permalink] [raw]
Subject: [PATCH] ath5k: reset key cache after resume

Otherwise it may be impossible to connected to an open network after a
resume.

This is a modified version of an original patch by
Alex Eskin <[email protected]>:

https://bugzilla.redhat.com/show_bug.cgi?id=425950#c8

Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath5k/base.c | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 75e5970..6ab4746 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -604,7 +604,7 @@ ath5k_pci_resume(struct pci_dev *pdev)
{
struct ieee80211_hw *hw = pci_get_drvdata(pdev);
struct ath5k_softc *sc = hw->priv;
- int err;
+ int i, err;

err = pci_set_power_state(pdev, PCI_D0);
if (err)
@@ -628,6 +628,16 @@ ath5k_pci_resume(struct pci_dev *pdev)
ath5k_hw_set_gpio(sc->ah, sc->led_pin, 0);
}

+ /*
+ * Reset the key cache since some parts do not
+ * reset the contents on initial power up or resume.
+ *
+ * FIXME: This may need to be revisited when mac80211 becomes
+ * aware of suspend/resume.
+ */
+ for (i = 0; i < AR5K_KEYTABLE_SIZE; i++)
+ ath5k_hw_reset_key(sc->ah, i);
+
return 0;
}
#endif /* CONFIG_PM */
--
1.5.3.3


2008-01-22 00:20:53

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: reset key cache after resume

2008/1/21, John W. Linville <[email protected]>:
> Otherwise it may be impossible to connected to an open network after a
> resume.
>
> This is a modified version of an original patch by
> Alex Eskin <[email protected]>:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=425950#c8
>
> Signed-off-by: John W. Linville <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 12 +++++++++++-
> 1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 75e5970..6ab4746 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -604,7 +604,7 @@ ath5k_pci_resume(struct pci_dev *pdev)
> {
> struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> struct ath5k_softc *sc = hw->priv;
> - int err;
> + int i, err;
>
> err = pci_set_power_state(pdev, PCI_D0);
> if (err)
> @@ -628,6 +628,16 @@ ath5k_pci_resume(struct pci_dev *pdev)
> ath5k_hw_set_gpio(sc->ah, sc->led_pin, 0);
> }
>
> + /*
> + * Reset the key cache since some parts do not
> + * reset the contents on initial power up or resume.
> + *
> + * FIXME: This may need to be revisited when mac80211 becomes
> + * aware of suspend/resume.
> + */
> + for (i = 0; i < AR5K_KEYTABLE_SIZE; i++)
> + ath5k_hw_reset_key(sc->ah, i);
> +
> return 0;
> }
> #endif /* CONFIG_PM */
> --
> 1.5.3.3
>

Thanx John ;-)

Acked-by: Nick Kossifidis <[email protected]>


--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2008-01-22 01:20:54

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: use AR5K_KEYTABLE_SIZE when initializing key table

2008/1/21, John W. Linville <[email protected]>:
> ...instead of using AR5K_KEYCACHE_SIZE, which would seem to be a
> typo/thinko...
>
> Signed-off-by: John W. Linville <[email protected]>
> ---
> drivers/net/wireless/ath5k/base.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index 742616a..75e5970 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -663,7 +663,7 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
> * Reset the key cache since some parts do not
> * reset the contents on initial power up.
> */
> - for (i = 0; i < AR5K_KEYCACHE_SIZE; i++)
> + for (i = 0; i < AR5K_KEYTABLE_SIZE; i++)
> ath5k_hw_reset_key(ah, i);
>
> /*
> --
> 1.5.3.3
>

This seems ok since we check entry against KEYTABLE_SIZE (it was
indeed a typo)...

int ath5k_hw_reset_key(struct ath5k_hw *ah, u16 entry)
{
unsigned int i;

ATH5K_TRACE(ah->ah_sc);
AR5K_ASSERT_ENTRY(entry, AR5K_KEYTABLE_SIZE);
...

So it's O.K. by me ;-)

But now that you 've mentioned it, i have found the following...

reset_key seems ok for 5211 since the whole cache is zeroed, that's
what we also get on regdumps during atach...

W: 0x8800 = 0x00000000 - AR5K_KEYTABLE_0_5211(0)
................................ (ath_hal_keyreset)
W: 0x8804 = 0x00000000 - AR5K_KEYTABLE_0_5211(1)
................................ (ath_hal_keyreset)
W: 0x8808 = 0x00000000 - AR5K_KEYTABLE_0_5211(2)
................................ (ath_hal_keyreset)

...

W: 0x97f4 = 0x00000000 - AR5K_KEYTABLE_0_5211(1021)
................................ (ath_hal_keyreset)
W: 0x97f8 = 0x00000000 - AR5K_KEYTABLE_0_5211(1022)
................................ (ath_hal_keyreset)
W: 0x97fc = 0x00000000 - AR5K_KEYTABLE_0_5211(1023)
................................ (ath_hal_keyreset)

(We have 128 * 8 = 1024 entries as expected)

BUT on newer chips, during reset the cache is not entirely zero, check
this out...

This is repeating for every key cache entry (every 8 steps on the table)...

a) We read the register entry + 5 (in our code just before the for
loop -we need nested loops for this)...
R: 0x8814 = 0x00003058 - AR5K_KEYTABLE_0_5211(5)
..................11.....1.11... (ath_hal_keyreset)

b) Inside the for loop we just replace it with 0x00000007 (no matter
what we read)...
W: 0x8814 = 0x00000007 - AR5K_KEYTABLE_0_5211(5)
.............................111 (unknown)

This is how it looks...

1st entry...
R: 0x8814 = 0x00003058 - AR5K_KEYTABLE_0_5211(5)
..................11.....1.11... (ath_hal_keyreset)
W: 0x8800 = 0x00000000 - AR5K_KEYTABLE_0_5211(0)
................................ (ath_hal_keyreset)
W: 0x8804 = 0x00000000 - AR5K_KEYTABLE_0_5211(1)
................................ (ath_hal_keyreset)
W: 0x8808 = 0x00000000 - AR5K_KEYTABLE_0_5211(2)
................................ (ath_hal_keyreset)
W: 0x880c = 0x00000000 - AR5K_KEYTABLE_0_5211(3)
................................ (ath_hal_keyreset)
W: 0x8810 = 0x00000000 - AR5K_KEYTABLE_0_5211(4)
................................ (ath_hal_keyreset)
W: 0x8814 = 0x00000007 - AR5K_KEYTABLE_0_5211(5)
.............................111 (ath_hal_keyreset)
W: 0x8818 = 0x00000000 - AR5K_KEYTABLE_0_5211(6)
................................ (ath_hal_keyreset)
W: 0x881c = 0x00000000 - AR5K_KEYTABLE_0_5211(7)
................................ (ath_hal_keyreset)

2nd entry...
R: 0x8834 = 0x0000be95 - AR5K_KEYTABLE_0_5211(13)
................1.11111.1..1.1.1 (ath_hal_keyreset)
W: 0x8820 = 0x00000000 - AR5K_KEYTABLE_0_5211(8)
................................ (ath_hal_keyreset)
W: 0x8824 = 0x00000000 - AR5K_KEYTABLE_0_5211(9)
................................ (ath_hal_keyreset)
W: 0x8828 = 0x00000000 - AR5K_KEYTABLE_0_5211(10)
................................ (ath_hal_keyreset)
W: 0x882c = 0x00000000 - AR5K_KEYTABLE_0_5211(11)
................................ (ath_hal_keyreset)
W: 0x8830 = 0x00000000 - AR5K_KEYTABLE_0_5211(12)
................................ (ath_hal_keyreset)
W: 0x8834 = 0x00000007 - AR5K_KEYTABLE_0_5211(13)
.............................111 (ath_hal_keyreset)
W: 0x8838 = 0x00000000 - AR5K_KEYTABLE_0_5211(14)
................................ (ath_hal_keyreset)
W: 0x883c = 0x00000000 - AR5K_KEYTABLE_0_5211(15)
................................ (ath_hal_keyreset)

etc.

Same happens on 5418 also (just there registers always read
0x00000007, at least in my case).

So for chips newer than 5211 we have to tweak reset_key a little. I
have a patch ready but i'll post them all together on my next series
that 'll addd 2413 support + rf code cleanups/refactoring (currently
we 're having exams and i don't have any time to propertly make a
patch series the way i want to).


Anyway this patch is correct so thanx again ;-)

Acked-by: Nick Kossifidis <[email protected]>

--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick