ath9k: Avoid OF no-EEPROM quirks without qca,no-eeprom

Commit Message

Daniel F. Dickinson Dec. 22, 2018, 6:09 a.m. UTC

ath9k_of_init() function[0] was initially written on the assumption that
if someone had an explicit ath9k OF node that "there must be something
wrong, why would someone add an OF node if everything is fine"[1]
(Quoting Martin Blumenstingl <[e-mail]>)

"it turns out it's not that simple. with your requirements I'm now aware
of two use-cases where the current code in ath9k_of_init() doesn't work
without modifications"[1]

The "your requirements" Martin speaks of is the result of the fact that I
have a device (PowerCloud Systems CR5000) has some kind of default - not
unique mac address - set and requires to set the correct MAC address via
mac-address devicetree property, however:

"some cards come with a physical EEPROM chip [or OTP] so "qca,no-eeprom"
should not be set (your use-case). in this case AH_USE_EEPROM should be
set (which is the default when there is no OF node)"[1]

The other use case is:

the firmware on some PowerMac G5 seems to add a OF node for the ath9k
card automatically. depending on the EEPROM on the card AH_NO_EEP_SWAP
should be unset (which is the default when there is no OF node). see [3]

After this patch to ath9k_of_init() the new behavior will be:

    if there's no OF node then everything is the same as before
    if there's an empty OF node then ath9k will use the hardware EEPROM
      (before ath9k would fail to initialize because no EEPROM data was
      provided by userspace)
    if there's an OF node with only a MAC address then ath9k will use
      the MAC address and the hardware EEPROM (see the case above)
    with "qca,no-eeprom" EEPROM data from userspace will be requested.
      the behavior here will not change
[1]

Martin provides additional background on EEPROM swapping[1].

Thanks to Christian Lamparter <[e-mail]> for all his help on
troubleshooting this issue and the basis for this patch.

Fixes: 138b41253d9c ("ath9k: parse the device configuration from an OF node")

[0]https://elixir.bootlin.com/linux/v4.20-rc7/source/drivers/net/wireless/ath/ath9k/init.c#L615
[1]https://github.com/openwrt/openwrt/pull/1645#issuecomment-448027058
[2]https://github.com/openwrt/openwrt/pull/1613
[3]https://patchwork.kernel.org/patch/10241731/

Signed-off-by: Daniel F. Dickinson <[e-mail]>
---
 drivers/net/wireless/ath/ath9k/init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Additional Background on EEPROM swapping[0] (quoting Martin again):

some additional information on the EEPROM swapping (for the curious who
want some additional context why I'm suggesting to move both flags
together): we need AH_NO_EEP_SWAP on a few devices. one of them is the
BT Home Hub 5A (lantiq target). this flag decides whether ath9k will
look at the first two "magic" bytes (0x5aa5 or 0xa55a). if this
doesn't match ath9k's expectations (defined at compile-time: big or
little endian) then ath9k calls swab16 on the whole EEPROM data.
however, this assumes that whoever defined the EEPROM data knew how to
set the magic bytes correctly (which is not the case - at least on
BT Home Hub 5A).  The EEPROM data itself (assuming all bytes are in the
correct place - which may require calling swab16 on the whole EEPROM
data) has an endianness bit which decided whether all u16/u32 values
are to be interpreted as little (default) or big endian.
Some EEPROM authors got the magic bytes wrong, but as far as I know
everyone got the EEPMISC endianness bit right.

[0]https://github.com/openwrt/openwrt/pull/1645#issuecomment-448027058

Comments

Daniel F. Dickinson Dec. 23, 2018, 12:42 a.m. UTC | #1

Greetings Patchwork admin/operator:

Patchwork didn't recognizing that the Re: went with the original 
[PATCH], even though In-Reply-To is correct.

This is because "[PATCH] ath9k: Avoid OF no-EEPROM quirks without 
qca,no-eeprom" <[e-mail]>, there was 
a reply ("Re: [PATCH] ath9k: Avoid OF no-EEPROM quirks without 
qca,no-eeprom" 
<CAFBinCA3MTO=[e-mail]>) 
which was accepted by the list before the patch.

It seems the list was refusing connections for some hours so my Bcc: to 
Martin Blumenstingl reached him long before the PATCH hit the list, and 
when it was back he's probably already on the whitelist for greylisting 
whereas I was greylisted.

I did look on the wireless wiki, patchwork site, and vger.kernel.org 
website to if there were any hints on how I could fix this, or who I 
could ping about it, but since I couldn't find such a link, I am posting 
to the applicable (this) list.

Regards,

Daniel

Kalle Valo Jan. 7, 2019, 1:23 p.m. UTC | #2

Martin Blumenstingl <[e-mail]> writes:

> On Sat, Dec 22, 2018 at 7:09 AM Daniel F. Dickinson
> <[e-mail]> wrote:
>>
>> ath9k_of_init() function[0] was initially written on the assumption that
>> if someone had an explicit ath9k OF node that "there must be something
>> wrong, why would someone add an OF node if everything is fine"[1]
>> (Quoting Martin Blumenstingl <[e-mail]>)
>>
>> "it turns out it's not that simple. with your requirements I'm now aware
>> of two use-cases where the current code in ath9k_of_init() doesn't work
>> without modifications"[1]
>>
>> The "your requirements" Martin speaks of is the result of the fact that I
>> have a device (PowerCloud Systems CR5000) has some kind of default - not
>> unique mac address - set and requires to set the correct MAC address via
>> mac-address devicetree property, however:
>>
>> "some cards come with a physical EEPROM chip [or OTP] so "qca,no-eeprom"
>> should not be set (your use-case). in this case AH_USE_EEPROM should be
>> set (which is the default when there is no OF node)"[1]
>>
>> The other use case is:
>>
>> the firmware on some PowerMac G5 seems to add a OF node for the ath9k
>> card automatically. depending on the EEPROM on the card AH_NO_EEP_SWAP
>> should be unset (which is the default when there is no OF node). see [3]
>>
>> After this patch to ath9k_of_init() the new behavior will be:
>>
>>     if there's no OF node then everything is the same as before
>>     if there's an empty OF node then ath9k will use the hardware EEPROM
>>       (before ath9k would fail to initialize because no EEPROM data was
>>       provided by userspace)
>>     if there's an OF node with only a MAC address then ath9k will use
>>       the MAC address and the hardware EEPROM (see the case above)
>>     with "qca,no-eeprom" EEPROM data from userspace will be requested.
>>       the behavior here will not change
>> [1]
>>
>> Martin provides additional background on EEPROM swapping[1].
>>
>> Thanks to Christian Lamparter <[e-mail]> for all his help on
>> troubleshooting this issue and the basis for this patch.
>>
>> Fixes: 138b41253d9c ("ath9k: parse the device configuration from an OF node")
>>
>> [0]https://elixir.bootlin.com/linux/v4.20-rc7/source/drivers/net/wireless/ath/ath9k/init.c#L615
>> [1]https://github.com/openwrt/openwrt/pull/1645#issuecomment-448027058
>> [2]https://github.com/openwrt/openwrt/pull/1613
>> [3]https://patchwork.kernel.org/patch/10241731/
>
> +Cc Bas Vermeulen: can you please try Daniel's patch on your PowerMac
> G5? I hope that this replaces your "endian_check" module parameter!
>
>> Signed-off-by: Daniel F. Dickinson <[e-mail]>
>
> Reviewed-by: Martin Blumenstingl <[e-mail]>
> (to me the description makes sense, but that said: I contributed some
> of the description)
>
> as well as:
> Tested-by: Martin Blumenstingl <[e-mail]>
> (on my BT HomeHub 5A - which sets qca,no-eeprom - using OpenWrt, which
> is currently running backports v4.19.7)

Daniel noticed that patchwork dropped Martin's reply, adding it to
patchwork now.

Kalle Valo Jan. 10, 2019, 1:29 p.m. UTC | #3

"Daniel F. Dickinson" <[e-mail]> wrote:

> ath9k_of_init() function[0] was initially written on the assumption that
> if someone had an explicit ath9k OF node that "there must be something
> wrong, why would someone add an OF node if everything is fine"[1]
> (Quoting Martin Blumenstingl <[e-mail]>)
> 
> "it turns out it's not that simple. with your requirements I'm now aware
> of two use-cases where the current code in ath9k_of_init() doesn't work
> without modifications"[1]
> 
> The "your requirements" Martin speaks of is the result of the fact that I
> have a device (PowerCloud Systems CR5000) has some kind of default - not
> unique mac address - set and requires to set the correct MAC address via
> mac-address devicetree property, however:
> 
> "some cards come with a physical EEPROM chip [or OTP] so "qca,no-eeprom"
> should not be set (your use-case). in this case AH_USE_EEPROM should be
> set (which is the default when there is no OF node)"[1]
> 
> The other use case is:
> 
> the firmware on some PowerMac G5 seems to add a OF node for the ath9k
> card automatically. depending on the EEPROM on the card AH_NO_EEP_SWAP
> should be unset (which is the default when there is no OF node). see [3]
> 
> After this patch to ath9k_of_init() the new behavior will be:
> 
>     if there's no OF node then everything is the same as before
>     if there's an empty OF node then ath9k will use the hardware EEPROM
>       (before ath9k would fail to initialize because no EEPROM data was
>       provided by userspace)
>     if there's an OF node with only a MAC address then ath9k will use
>       the MAC address and the hardware EEPROM (see the case above)
>     with "qca,no-eeprom" EEPROM data from userspace will be requested.
>       the behavior here will not change
> [1]
> 
> Martin provides additional background on EEPROM swapping[1].
> 
> Thanks to Christian Lamparter <[e-mail]> for all his help on
> troubleshooting this issue and the basis for this patch.
> 
> [0]https://elixir.bootlin.com/linux/v4.20-rc7/source/drivers/net/wireless/ath/ath9k/init.c#L615
> [1]https://github.com/openwrt/openwrt/pull/1645#issuecomment-448027058
> [2]https://github.com/openwrt/openwrt/pull/1613
> [3]https://patchwork.kernel.org/patch/10241731/
> 
> Fixes: 138b41253d9c ("ath9k: parse the device configuration from an OF node")
> Reviewed-by: Martin Blumenstingl <[e-mail]>
> Tested-by: Martin Blumenstingl <[e-mail]>
> Signed-off-by: Daniel F. Dickinson <[e-mail]>
> Signed-off-by: Kalle Valo <[e-mail]>

Patch applied to ath-next branch of ath.git, thanks.

ce938231bd3b ath9k: Avoid OF no-EEPROM quirks without qca,no-eeprom

Patch

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index c070a9e51ebf..fae572b38416 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -636,15 +636,15 @@  static int ath9k_of_init(struct ath_softc *sc)
 		ret = ath9k_eeprom_request(sc, eeprom_name);
 		if (ret)
 			return ret;
+
+		ah->ah_flags &= ~AH_USE_EEPROM;
+		ah->ah_flags |= AH_NO_EEP_SWAP;
 	}
 
 	mac = of_get_mac_address(np);
 	if (mac)
 		ether_addr_copy(common->macaddr, mac);
 
-	ah->ah_flags &= ~AH_USE_EEPROM;
-	ah->ah_flags |= AH_NO_EEP_SWAP;
-
 	return 0;
 }