[PATCH] ath9k: introduce endian_check module parameter

[PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

A random (little endian eeprom'd) ar9278 card didn't work on my
PowerMac G5 without allowing the driver to byte-swap the eeprom.

Introduce a module parameter endian_check to allow this to happen,
and the PCIe card to function correctly on BE powerpc.

Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
---
 drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index fa58a32227f5..421039dc060a 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -67,6 +67,9 @@ static int ath9k_ps_enable;
 module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
 MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
 
+static int ath9k_endian_check;
+module_param_named(endian_check, ath9k_endian_check, int, 0444);
+MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
 #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
 
 int ath9k_use_chanctx;
@@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
 		ether_addr_copy(common->macaddr, mac);
 
 	ah->ah_flags &= ~AH_USE_EEPROM;
-	ah->ah_flags |= AH_NO_EEP_SWAP;
+	if (!ath9k_endian_check)
+		ah->ah_flags |= AH_NO_EEP_SWAP;
 
 	return 0;
 }
-- 
2.16.2


-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Kalle Valo]

Bas Vermeulen <bvermeul@blackstar.nl> writes:

> A random (little endian eeprom'd) ar9278 card didn't work on my
> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>
> Introduce a module parameter endian_check to allow this to happen,
> and the PCIe card to function correctly on BE powerpc.
>
> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
> ---
>  drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index fa58a32227f5..421039dc060a 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>  module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>  MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>  
> +static int ath9k_endian_check;
> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>  #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>  
>  int ath9k_use_chanctx;
> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>  		ether_addr_copy(common->macaddr, mac);
>  
>  	ah->ah_flags &= ~AH_USE_EEPROM;
> -	ah->ah_flags |= AH_NO_EEP_SWAP;
> +	if (!ath9k_endian_check)
> +		ah->ah_flags |= AH_NO_EEP_SWAP;

A bit annoying to have a module parameter, isn't there any automatic way
to detect/try this? But on the other hand I guess this isn't a common
problem as nobody has reported this before?

-- 
Kalle Valo

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-02-18 10:54, Kalle Valo wrote:
> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>
>> A random (little endian eeprom'd) ar9278 card didn't work on my
>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>
>> Introduce a module parameter endian_check to allow this to happen,
>> and the PCIe card to function correctly on BE powerpc.
>>
>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>> ---
>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>> index fa58a32227f5..421039dc060a 100644
>> --- a/drivers/net/wireless/ath/ath9k/init.c
>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>   
>> +static int ath9k_endian_check;
>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>   
>>   int ath9k_use_chanctx;
>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>   		ether_addr_copy(common->macaddr, mac);
>>   
>>   	ah->ah_flags &= ~AH_USE_EEPROM;
>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>> +	if (!ath9k_endian_check)
>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
> A bit annoying to have a module parameter, isn't there any automatic way
> to detect/try this? But on the other hand I guess this isn't a common
> problem as nobody has reported this before?
There is an automatic way to detect this, but that is disabled by the 
AH_NO_EEP_SWAP flag.
The platform initialisation does not set this flag if the endian_check 
member of pdata is set
to true, but there is no way to not set this when using a device tree. I 
used a module
parameter instead of a device tree variable because I don't know of a 
way to modify the
device tree my PowerMac boots with.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Sebastian Gottschall]

Am 26.02.2018 um 11:07 schrieb Bas Vermeulen:
> On 26-02-18 10:54, Kalle Valo wrote:
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>
>>> Introduce a module parameter endian_check to allow this to happen,
>>> and the PCIe card to function correctly on BE powerpc.
>>>
>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c 
>>> b/drivers/net/wireless/ath/ath9k/init.c
>>> index fa58a32227f5..421039dc060a 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>   +static int ath9k_endian_check;
>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness 
>>> compatibility");
>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>     int ath9k_use_chanctx;
>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>           ether_addr_copy(common->macaddr, mac);
>>>         ah->ah_flags &= ~AH_USE_EEPROM;
>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>> +    if (!ath9k_endian_check)
>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>> A bit annoying to have a module parameter, isn't there any automatic way
>> to detect/try this? But on the other hand I guess this isn't a common
>> problem as nobody has reported this before?
> There is an automatic way to detect this, but that is disabled by the 
> AH_NO_EEP_SWAP flag.
> The platform initialisation does not set this flag if the endian_check 
> member of pdata is set
> to true, but there is no way to not set this when using a device tree. 
> I used a module
> parameter instead of a device tree variable because I don't know of a 
> way to modify the
> device tree my PowerMac boots with.
have you tried to compile it without device tree support? since its just 
a pcie card, i dont think that devicetree is required here
it should run fine without it.
>
> Bas Vermeulen
>

-- 
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-02-18 12:30, Sebastian Gottschall wrote:
> Am 26.02.2018 um 11:07 schrieb Bas Vermeulen:
>> On 26-02-18 10:54, Kalle Valo wrote:
>>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>>
>>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>>
>>>> Introduce a module parameter endian_check to allow this to happen,
>>>> and the PCIe card to function correctly on BE powerpc.
>>>>
>>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>>> ---
>>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c 
>>>> b/drivers/net/wireless/ath/ath9k/init.c
>>>> index fa58a32227f5..421039dc060a 100644
>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>   +static int ath9k_endian_check;
>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness 
>>>> compatibility");
>>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>     int ath9k_use_chanctx;
>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>           ether_addr_copy(common->macaddr, mac);
>>>>         ah->ah_flags &= ~AH_USE_EEPROM;
>>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>>> +    if (!ath9k_endian_check)
>>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>>> A bit annoying to have a module parameter, isn't there any automatic 
>>> way
>>> to detect/try this? But on the other hand I guess this isn't a common
>>> problem as nobody has reported this before?
>> There is an automatic way to detect this, but that is disabled by the 
>> AH_NO_EEP_SWAP flag.
>> The platform initialisation does not set this flag if the 
>> endian_check member of pdata is set
>> to true, but there is no way to not set this when using a device 
>> tree. I used a module
>> parameter instead of a device tree variable because I don't know of a 
>> way to modify the
>> device tree my PowerMac boots with.
> have you tried to compile it without device tree support? since its 
> just a pcie card, i dont think that devicetree is required here
> it should run fine without it.

The driver will still set AH_NO_EEP_SWAP regardless, and will still not 
swap the eeprom from
little endian to big endian on big endian machines. See 
drivers/net/wireless/ath/ath9k/eeprom.c:188
and drivers/net/wireless/ath/ath9k/init.c (lines 593 and 645).

The reason I'm talking about device trees here is because I could have 
used a device tree parameter
instead of a module parameter.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Kalle Valo]

Bas Vermeulen <bvermeul@blackstar.nl> writes:

> On 26-02-18 10:54, Kalle Valo wrote:
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>
>>> Introduce a module parameter endian_check to allow this to happen,
>>> and the PCIe card to function correctly on BE powerpc.
>>>
>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>>> index fa58a32227f5..421039dc060a 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>   +static int ath9k_endian_check;
>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>     int ath9k_use_chanctx;
>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>   		ether_addr_copy(common->macaddr, mac);
>>>     	ah->ah_flags &= ~AH_USE_EEPROM;
>>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>>> +	if (!ath9k_endian_check)
>>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
>> A bit annoying to have a module parameter, isn't there any automatic way
>> to detect/try this? But on the other hand I guess this isn't a common
>> problem as nobody has reported this before?
>
> There is an automatic way to detect this, but that is disabled by the
> AH_NO_EEP_SWAP flag.

Ah, I didn't check the code at all.

> The platform initialisation does not set this flag if the endian_check
> member of pdata is set to true, but there is no way to not set this
> when using a device tree. I used a module parameter instead of a
> device tree variable because I don't know of a way to modify the
> device tree my PowerMac boots with.

Ok, makes sense. A module parameter is not an ideal solution I guess
it's ok in this case.

-- 
Kalle Valo

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-02-18 15:52, Kalle Valo wrote:
> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>
>> On 26-02-18 10:54, Kalle Valo wrote:
>>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>>
>>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>>
>>>> Introduce a module parameter endian_check to allow this to happen,
>>>> and the PCIe card to function correctly on BE powerpc.
>>>>
>>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>>> ---
>>>>    drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>>>> index fa58a32227f5..421039dc060a 100644
>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>    module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>    MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>    +static int ath9k_endian_check;
>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>>    #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>      int ath9k_use_chanctx;
>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>    		ether_addr_copy(common->macaddr, mac);
>>>>      	ah->ah_flags &= ~AH_USE_EEPROM;
>>>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>>>> +	if (!ath9k_endian_check)
>>>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
>>> A bit annoying to have a module parameter, isn't there any automatic way
>>> to detect/try this? But on the other hand I guess this isn't a common
>>> problem as nobody has reported this before?
>> There is an automatic way to detect this, but that is disabled by the
>> AH_NO_EEP_SWAP flag.
> Ah, I didn't check the code at all.
>
>> The platform initialisation does not set this flag if the endian_check
>> member of pdata is set to true, but there is no way to not set this
>> when using a device tree. I used a module parameter instead of a
>> device tree variable because I don't know of a way to modify the
>> device tree my PowerMac boots with.
> Ok, makes sense. A module parameter is not an ideal solution I guess
> it's ok in this case.
Kalle: Are there any changes you want me to make in order to get this 
accepted?
I didn't see anything for me to resolve, but I may have missed something.

I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if 
you prefer, as that
would fix my problem as well. I am just not sure that doesn't break 
things for some
platform/device I don't have.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Kalle Valo]

Bas Vermeulen <bvermeul@blackstar.nl> writes:

>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>    module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>    MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>    +static int ath9k_endian_check;
>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>>>    #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>      int ath9k_use_chanctx;
>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>    		ether_addr_copy(common->macaddr, mac);
>>>>>      	ah->ah_flags &= ~AH_USE_EEPROM;
>>>>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>> +	if (!ath9k_endian_check)
>>>>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
>>>> A bit annoying to have a module parameter, isn't there any automatic way
>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>> problem as nobody has reported this before?
>>>
>>> There is an automatic way to detect this, but that is disabled by the
>>> AH_NO_EEP_SWAP flag.
>>
>> Ah, I didn't check the code at all.
>>
>>> The platform initialisation does not set this flag if the endian_check
>>> member of pdata is set to true, but there is no way to not set this
>>> when using a device tree. I used a module parameter instead of a
>>> device tree variable because I don't know of a way to modify the
>>> device tree my PowerMac boots with.
>>
>> Ok, makes sense. A module parameter is not an ideal solution I guess
>> it's ok in this case.
>>
> Kalle: Are there any changes you want me to make in order to get this
> accepted? I didn't see anything for me to resolve, but I may have
> missed something.
>
> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
> you prefer, as that would fix my problem as well. I am just not sure
> that doesn't break things for some platform/device I don't have.

I'm not really sure what to do. Basically this is a choise between bad
for user experience (the module parameter) or risk of regressions
(disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
hardware I'm starting to lean towards the module parameter approach
(your patch) but would like to know what others think, especially Felix
(CCed).

Full patch here:

https://patchwork.kernel.org/patch/10241731/

-- 
Kalle Valo

Re: [PATCH] ath9k: introduce endian_check module parameter

[Arend van Spriel]

+ Martin

On 3/14/2018 3:34 PM, Kalle Valo wrote:
> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>
>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>     module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>     MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>>     +static int ath9k_endian_check;
>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>>>>     #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>       int ath9k_use_chanctx;
>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>>     		ether_addr_copy(common->macaddr, mac);
>>>>>>       	ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>> +	if (!ath9k_endian_check)
>>>>>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>> A bit annoying to have a module parameter, isn't there any automatic way
>>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>>> problem as nobody has reported this before?
>>>>
>>>> There is an automatic way to detect this, but that is disabled by the
>>>> AH_NO_EEP_SWAP flag.
>>>
>>> Ah, I didn't check the code at all.
>>>
>>>> The platform initialisation does not set this flag if the endian_check
>>>> member of pdata is set to true, but there is no way to not set this
>>>> when using a device tree. I used a module parameter instead of a
>>>> device tree variable because I don't know of a way to modify the
>>>> device tree my PowerMac boots with.
>>>
>>> Ok, makes sense. A module parameter is not an ideal solution I guess
>>> it's ok in this case.
>>>
>> Kalle: Are there any changes you want me to make in order to get this
>> accepted? I didn't see anything for me to resolve, but I may have
>> missed something.
>>
>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
>> you prefer, as that would fix my problem as well. I am just not sure
>> that doesn't break things for some platform/device I don't have.
>
> I'm not really sure what to do. Basically this is a choise between bad
> for user experience (the module parameter) or risk of regressions
> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
> hardware I'm starting to lean towards the module parameter approach
> (your patch) but would like to know what others think, especially Felix
> (CCed).

Hi Kalle,

Sorry for barging in, but I figured git history might tell us something. 
The flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add 
support for endian swap of eeprom from platform data")) and the function 
ath9k_of_init() was added by Martin (CCed):

commit 138b41253d9c9f9a06c8b086880cd3e839a23d69
Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Sun Oct 16 22:59:07 2016 +0200

     ath9k: parse the device configuration from an OF node

Maybe he recalls what device(s) needed it.

Regards,
Arend

Re: [PATCH] ath9k: introduce endian_check module parameter

[Martin Blumenstingl]

Hello everyone,

On Wed, Mar 14, 2018 at 10:34 PM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
> + Martin
>
> On 3/14/2018 3:34 PM, Kalle Valo wrote:
>>
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>>     module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>>     MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>>>     +static int ath9k_endian_check;
>>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness
>>>>>>> compatibility");
>>>>>>>     #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>>       int ath9k_use_chanctx;
>>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>>>                 ether_addr_copy(common->macaddr, mac);
>>>>>>>         ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>>> -       ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>> +       if (!ath9k_endian_check)
>>>>>>> +               ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>
>>>>>> A bit annoying to have a module parameter, isn't there any automatic
>>>>>> way
>>>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>>>> problem as nobody has reported this before?
>>>>>
>>>>>
>>>>> There is an automatic way to detect this, but that is disabled by the
>>>>> AH_NO_EEP_SWAP flag.
>>>>
>>>>
>>>> Ah, I didn't check the code at all.
>>>>
>>>>> The platform initialisation does not set this flag if the endian_check
>>>>> member of pdata is set to true, but there is no way to not set this
>>>>> when using a device tree. I used a module parameter instead of a
>>>>> device tree variable because I don't know of a way to modify the
>>>>> device tree my PowerMac boots with.
>>>>
>>>>
>>>> Ok, makes sense. A module parameter is not an ideal solution I guess
>>>> it's ok in this case.
>>>>
>>> Kalle: Are there any changes you want me to make in order to get this
>>> accepted? I didn't see anything for me to resolve, but I may have
>>> missed something.
>>>
>>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
>>> you prefer, as that would fix my problem as well. I am just not sure
>>> that doesn't break things for some platform/device I don't have.
>>
>>
>> I'm not really sure what to do. Basically this is a choise between bad
>> for user experience (the module parameter) or risk of regressions
>> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
>> hardware I'm starting to lean towards the module parameter approach
>> (your patch) but would like to know what others think, especially Felix
>> (CCed).
>
>
> Hi Kalle,
>
> Sorry for barging in, but I figured git history might tell us something. The
> flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add support
> for endian swap of eeprom from platform data")) and the function
> ath9k_of_init() was added by Martin (CCed):
>
> commit 138b41253d9c9f9a06c8b086880cd3e839a23d69
> Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Date:   Sun Oct 16 22:59:07 2016 +0200
>
>     ath9k: parse the device configuration from an OF node
>
> Maybe he recalls what device(s) needed it.
lots embedded devices (supported by OpenWrt) use ath9k chips

my primary target was the BT HomeHub 5A and some AVM Fritz Box (all
using a lantiq SoC, but there are many more). these typically ship
with:
- a generic ath9k EEPROM which is sometimes even stored on NAND (= not
directly connected to the ath9k chipset), which is why we need the
"qca,no-eeprom"
- some ship with a broken EEPROM that enables the 5GHz band on a
2.4GHz-only card, which is why we need "qca,disable-5ghz"
- some ship with an EEPROM that doesn't have a unique mac address (the
mac address is sometimes also stored on NAND), which is why we support
the "mac-address" property
- some ship an EEPROM where only the magic bytes are swapped (but the
content is not), while others have both (magic bytes and content)
byte-swapped

looking at ath9k_of_init it seems that "ah->ah_flags &=
~AH_USE_EEPROM" and "ah->ah_flags |= AH_NO_EEP_SWAP" are called
unconditionally.
maybe these should be part of the "qca,no-eeprom" if-block a few lines above

I also added Mathias to the CC list
@Mathias: I believe all our .dts files in OpenWrt which specify an
ath9k chipset also set the "qca,no-eeprom" property, right (a quick
check suggests "yes")?


Regards
Martin

Re: [PATCH] ath9k: introduce endian_check module parameter

[Rafał Miłecki]

On 19 March 2018 at 09:11, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> - some ship with a broken EEPROM that enables the 5GHz band on a
> 2.4GHz-only card, which is why we need "qca,disable-5ghz"

No. You just need to use ieee80211-freq-limit.

--=20
Rafa=C5=82

Re: [PATCH] ath9k: introduce endian_check module parameter

[Martin Blumenstingl]

Hi Rafal,

On Tue, Mar 20, 2018 at 10:07 PM, Rafa=C5=82 Mi=C5=82ecki <zajec5@gmail.com=
> wrote:
> On 19 March 2018 at 09:11, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> - some ship with a broken EEPROM that enables the 5GHz band on a
>> 2.4GHz-only card, which is why we need "qca,disable-5ghz"
>
> No. You just need to use ieee80211-freq-limit.
you are right with this one, "qca,disable-5ghz" is an out-of-tree
patch and ieee80211-freq-limit support should be added upstream
instead
however, the cause is still the same (devices are shipped with an
EEPROM which has the 5GHz band enabled while it shouldn't have)


Regards
Martin

Re: [PATCH] ath9k: introduce endian_check module parameter

[Mathias Kresin]

19.03.2018 09:11, Martin Blumenstingl:
> Hello everyone,
> 
> On Wed, Mar 14, 2018 at 10:34 PM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> + Martin
>>
>> On 3/14/2018 3:34 PM, Kalle Valo wrote:
>>>
>>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>>
>>>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>>>      module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>>>      MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>>>>      +static int ath9k_endian_check;
>>>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness
>>>>>>>> compatibility");
>>>>>>>>      #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>>>        int ath9k_use_chanctx;
>>>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>>>>                  ether_addr_copy(common->macaddr, mac);
>>>>>>>>          ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>>>> -       ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>>> +       if (!ath9k_endian_check)
>>>>>>>> +               ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>>
>>>>>>> A bit annoying to have a module parameter, isn't there any automatic
>>>>>>> way
>>>>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>>>>> problem as nobody has reported this before?
>>>>>>
>>>>>>
>>>>>> There is an automatic way to detect this, but that is disabled by the
>>>>>> AH_NO_EEP_SWAP flag.
>>>>>
>>>>>
>>>>> Ah, I didn't check the code at all.
>>>>>
>>>>>> The platform initialisation does not set this flag if the endian_check
>>>>>> member of pdata is set to true, but there is no way to not set this
>>>>>> when using a device tree. I used a module parameter instead of a
>>>>>> device tree variable because I don't know of a way to modify the
>>>>>> device tree my PowerMac boots with.
>>>>>
>>>>>
>>>>> Ok, makes sense. A module parameter is not an ideal solution I guess
>>>>> it's ok in this case.
>>>>>
>>>> Kalle: Are there any changes you want me to make in order to get this
>>>> accepted? I didn't see anything for me to resolve, but I may have
>>>> missed something.
>>>>
>>>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
>>>> you prefer, as that would fix my problem as well. I am just not sure
>>>> that doesn't break things for some platform/device I don't have.
>>>
>>>
>>> I'm not really sure what to do. Basically this is a choise between bad
>>> for user experience (the module parameter) or risk of regressions
>>> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
>>> hardware I'm starting to lean towards the module parameter approach
>>> (your patch) but would like to know what others think, especially Felix
>>> (CCed).
>>
>>
>> Hi Kalle,
>>
>> Sorry for barging in, but I figured git history might tell us something. The
>> flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add support
>> for endian swap of eeprom from platform data")) and the function
>> ath9k_of_init() was added by Martin (CCed):
>>
>> commit 138b41253d9c9f9a06c8b086880cd3e839a23d69
>> Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Date:   Sun Oct 16 22:59:07 2016 +0200
>>
>>      ath9k: parse the device configuration from an OF node
>>
>> Maybe he recalls what device(s) needed it.
> lots embedded devices (supported by OpenWrt) use ath9k chips
> 
> my primary target was the BT HomeHub 5A and some AVM Fritz Box (all
> using a lantiq SoC, but there are many more). these typically ship
> with:
> - a generic ath9k EEPROM which is sometimes even stored on NAND (= not
> directly connected to the ath9k chipset), which is why we need the
> "qca,no-eeprom"
> - some ship with a broken EEPROM that enables the 5GHz band on a
> 2.4GHz-only card, which is why we need "qca,disable-5ghz"
> - some ship with an EEPROM that doesn't have a unique mac address (the
> mac address is sometimes also stored on NAND), which is why we support
> the "mac-address" property
> - some ship an EEPROM where only the magic bytes are swapped (but the
> content is not), while others have both (magic bytes and content)
> byte-swapped
> 
> looking at ath9k_of_init it seems that "ah->ah_flags &=
> ~AH_USE_EEPROM" and "ah->ah_flags |= AH_NO_EEP_SWAP" are called
> unconditionally.
> maybe these should be part of the "qca,no-eeprom" if-block a few lines above
> 
> I also added Mathias to the CC list
> @Mathias: I believe all our .dts files in OpenWrt which specify an
> ath9k chipset also set the "qca,no-eeprom" property, right (a quick
> check suggests "yes")?

Yes, they all should have the qca,no-eeprom property. If not, it is a 
bug in the OpenWrt dts files.

Mathias

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 14-03-18 15:34, Kalle Valo wrote:
> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>
>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>     module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>     MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>>     +static int ath9k_endian_check;
>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>>>>     #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>       int ath9k_use_chanctx;
>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>>     		ether_addr_copy(common->macaddr, mac);
>>>>>>       	ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>> +	if (!ath9k_endian_check)
>>>>>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>> A bit annoying to have a module parameter, isn't there any automatic way
>>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>>> problem as nobody has reported this before?
>>>> There is an automatic way to detect this, but that is disabled by the
>>>> AH_NO_EEP_SWAP flag.
>>> Ah, I didn't check the code at all.
>>>
>>>> The platform initialisation does not set this flag if the endian_check
>>>> member of pdata is set to true, but there is no way to not set this
>>>> when using a device tree. I used a module parameter instead of a
>>>> device tree variable because I don't know of a way to modify the
>>>> device tree my PowerMac boots with.
>>> Ok, makes sense. A module parameter is not an ideal solution I guess
>>> it's ok in this case.
>>>
>> Kalle: Are there any changes you want me to make in order to get this
>> accepted? I didn't see anything for me to resolve, but I may have
>> missed something.
>>
>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
>> you prefer, as that would fix my problem as well. I am just not sure
>> that doesn't break things for some platform/device I don't have.
> I'm not really sure what to do. Basically this is a choise between bad
> for user experience (the module parameter) or risk of regressions
> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
> hardware I'm starting to lean towards the module parameter approach
> (your patch) but would like to know what others think, especially Felix
> (CCed).
>
> Full patch here:
>
> https://patchwork.kernel.org/patch/10241731/

Just another ping. As I understood things, all OpenWRT dts' use 
qca,no-eeprom, and perhaps we could
move ~AH_USE_EEPROM and |= AH_NO_EEP_SWAP in that if block.

This would solve my problem, as I need to have AH_NO_EEP_SWAP removed 
from flags for my card (which is a
little endian eeprom/card used on a big endian machine).

Would you like me to prepare a patch for this? Is there anyone who can 
test this on the various OpenWRT
boards/soc and or other configurations? I don't want to break things for 
other people.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Martin Blumenstingl]

Hello Bas,

On Tue, Apr 10, 2018 at 11:05 AM, Bas Vermeulen <bvermeul@blackstar.nl> wrote:
> On 14-03-18 15:34, Kalle Valo wrote:
>>
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>>     module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>>     MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>>>     +static int ath9k_endian_check;
>>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness
>>>>>>> compatibility");
>>>>>>>     #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>>       int ath9k_use_chanctx;
>>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>>>>                 ether_addr_copy(common->macaddr, mac);
>>>>>>>         ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>>> -       ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>> +       if (!ath9k_endian_check)
>>>>>>> +               ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>>
>>>>>> A bit annoying to have a module parameter, isn't there any automatic
>>>>>> way
>>>>>> to detect/try this? But on the other hand I guess this isn't a common
>>>>>> problem as nobody has reported this before?
>>>>>
>>>>> There is an automatic way to detect this, but that is disabled by the
>>>>> AH_NO_EEP_SWAP flag.
>>>>
>>>> Ah, I didn't check the code at all.
>>>>
>>>>> The platform initialisation does not set this flag if the endian_check
>>>>> member of pdata is set to true, but there is no way to not set this
>>>>> when using a device tree. I used a module parameter instead of a
>>>>> device tree variable because I don't know of a way to modify the
>>>>> device tree my PowerMac boots with.
>>>>
>>>> Ok, makes sense. A module parameter is not an ideal solution I guess
>>>> it's ok in this case.
>>>>
>>> Kalle: Are there any changes you want me to make in order to get this
>>> accepted? I didn't see anything for me to resolve, but I may have
>>> missed something.
>>>
>>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if
>>> you prefer, as that would fix my problem as well. I am just not sure
>>> that doesn't break things for some platform/device I don't have.
>>
>> I'm not really sure what to do. Basically this is a choise between bad
>> for user experience (the module parameter) or risk of regressions
>> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic
>> hardware I'm starting to lean towards the module parameter approach
>> (your patch) but would like to know what others think, especially Felix
>> (CCed).
>>
>> Full patch here:
>>
>> https://patchwork.kernel.org/patch/10241731/
>
>
> Just another ping. As I understood things, all OpenWRT dts' use
> qca,no-eeprom, and perhaps we could
> move ~AH_USE_EEPROM and |= AH_NO_EEP_SWAP in that if block.
>
> This would solve my problem, as I need to have AH_NO_EEP_SWAP removed from
> flags for my card (which is a
> little endian eeprom/card used on a big endian machine).
>
> Would you like me to prepare a patch for this? Is there anyone who can test
> this on the various OpenWRT
> boards/soc and or other configurations? I don't want to break things for
> other people.
can you please prepare a patch for this?
if you want I can test it on two OpenWrt devices and see if it breaks
anything (please give me a few days to test though)


Regards
Martin

Re: [PATCH] ath9k: introduce endian_check module parameter

[Larry Finger]

On 02/26/2018 04:07 AM, Bas Vermeulen wrote:
> On 26-02-18 10:54, Kalle Valo wrote:
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>
>>> Introduce a module parameter endian_check to allow this to happen,
>>> and the PCIe card to function correctly on BE powerpc.
>>>
>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c 
>>> b/drivers/net/wireless/ath/ath9k/init.c
>>> index fa58a32227f5..421039dc060a 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>> +static int ath9k_endian_check;
>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>   int ath9k_use_chanctx;
>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>           ether_addr_copy(common->macaddr, mac);
>>>       ah->ah_flags &= ~AH_USE_EEPROM;
>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>> +    if (!ath9k_endian_check)
>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>> A bit annoying to have a module parameter, isn't there any automatic way
>> to detect/try this? But on the other hand I guess this isn't a common
>> problem as nobody has reported this before?
> There is an automatic way to detect this, but that is disabled by the 
> AH_NO_EEP_SWAP flag.
> The platform initialisation does not set this flag if the endian_check member of 
> pdata is set
> to true, but there is no way to not set this when using a device tree. I used a 
> module
> parameter instead of a device tree variable because I don't know of a way to 
> modify the
> device tree my PowerMac boots with.

Shouldn't you be able to set ath9k_endian_check inside #ifdef __BIG_ENDIAN ... 
#endif in the initialization? I think that would achieve the same functionality 
without requiring the user to set a module parameter.

I agree that you want to stay away from the device tree in a PPC computer.

Larry

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-02-18 17:32, Larry Finger wrote:
> On 02/26/2018 04:07 AM, Bas Vermeulen wrote:
>> On 26-02-18 10:54, Kalle Valo wrote:
>>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>>
>>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>>
>>>> Introduce a module parameter endian_check to allow this to happen,
>>>> and the PCIe card to function correctly on BE powerpc.
>>>>
>>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>>> ---
>>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c 
>>>> b/drivers/net/wireless/ath/ath9k/init.c
>>>> index fa58a32227f5..421039dc060a 100644
>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>> +static int ath9k_endian_check;
>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness 
>>>> compatibility");
>>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>   int ath9k_use_chanctx;
>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>>           ether_addr_copy(common->macaddr, mac);
>>>>       ah->ah_flags &= ~AH_USE_EEPROM;
>>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>>> +    if (!ath9k_endian_check)
>>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>>> A bit annoying to have a module parameter, isn't there any automatic 
>>> way
>>> to detect/try this? But on the other hand I guess this isn't a common
>>> problem as nobody has reported this before?
>> There is an automatic way to detect this, but that is disabled by the 
>> AH_NO_EEP_SWAP flag.
>> The platform initialisation does not set this flag if the 
>> endian_check member of pdata is set
>> to true, but there is no way to not set this when using a device 
>> tree. I used a module
>> parameter instead of a device tree variable because I don't know of a 
>> way to modify the
>> device tree my PowerMac boots with.
>
> Shouldn't you be able to set ath9k_endian_check inside #ifdef 
> __BIG_ENDIAN ... #endif in the initialization? I think that would 
> achieve the same functionality without requiring the user to set a 
> module parameter.

I could have done that, but didn't want to change the default behaviour. 
This patch keeps the
current behaviour on all platforms if the module parameter is not set. I 
don't have the means
to test mips and other platforms this could be used on. I don't mind 
having to set a module
parameter, I mind not being able to change the behaviour.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Dan Williams]

On Mon, 2018-02-26 at 17:44 +0100, Bas Vermeulen wrote:
> On 26-02-18 17:32, Larry Finger wrote:
> > On 02/26/2018 04:07 AM, Bas Vermeulen wrote:
> > > On 26-02-18 10:54, Kalle Valo wrote:
> > > > Bas Vermeulen <bvermeul@blackstar.nl> writes:
> > > > 
> > > > > A random (little endian eeprom'd) ar9278 card didn't work on
> > > > > my
> > > > > PowerMac G5 without allowing the driver to byte-swap the
> > > > > eeprom.
> > > > > 
> > > > > Introduce a module parameter endian_check to allow this to
> > > > > happen,
> > > > > and the PCIe card to function correctly on BE powerpc.
> > > > > 
> > > > > Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
> > > > > ---
> > > > >   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
> > > > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/net/wireless/ath/ath9k/init.c 
> > > > > b/drivers/net/wireless/ath/ath9k/init.c
> > > > > index fa58a32227f5..421039dc060a 100644
> > > > > --- a/drivers/net/wireless/ath/ath9k/init.c
> > > > > +++ b/drivers/net/wireless/ath/ath9k/init.c
> > > > > @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
> > > > >   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
> > > > >   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
> > > > > +static int ath9k_endian_check;
> > > > > +module_param_named(endian_check, ath9k_endian_check, int,
> > > > > 0444);
> > > > > +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness 
> > > > > compatibility");
> > > > >   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
> > > > >   int ath9k_use_chanctx;
> > > > > @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc
> > > > > *sc)
> > > > >           ether_addr_copy(common->macaddr, mac);
> > > > >       ah->ah_flags &= ~AH_USE_EEPROM;
> > > > > -    ah->ah_flags |= AH_NO_EEP_SWAP;
> > > > > +    if (!ath9k_endian_check)
> > > > > +        ah->ah_flags |= AH_NO_EEP_SWAP;
> > > > 
> > > > A bit annoying to have a module parameter, isn't there any
> > > > automatic 
> > > > way
> > > > to detect/try this? But on the other hand I guess this isn't a
> > > > common
> > > > problem as nobody has reported this before?
> > > 
> > > There is an automatic way to detect this, but that is disabled by
> > > the 
> > > AH_NO_EEP_SWAP flag.
> > > The platform initialisation does not set this flag if the 
> > > endian_check member of pdata is set
> > > to true, but there is no way to not set this when using a device 
> > > tree. I used a module
> > > parameter instead of a device tree variable because I don't know
> > > of a 
> > > way to modify the
> > > device tree my PowerMac boots with.
> > 
> > Shouldn't you be able to set ath9k_endian_check inside #ifdef 
> > __BIG_ENDIAN ... #endif in the initialization? I think that would 
> > achieve the same functionality without requiring the user to set a 
> > module parameter.
> 
> I could have done that, but didn't want to change the default
> behaviour. 
> This patch keeps the
> current behaviour on all platforms if the module parameter is not
> set. I 
> don't have the means
> to test mips and other platforms this could be used on. I don't mind 
> having to set a module
> parameter, I mind not being able to change the behaviour

Still, module parameters are an awful user experience because you need
to know that they exist, what they do, and whether you need it or not. 
It doesn't just work.

Is there no way to autodetect the endian-ness of the firmware itself,
and if the known machine endian-ness isn't the same then swap?  This
seems like a solvable problem.

Dan

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-2-2018 18:42, Dan Williams wrote:
> On Mon, 2018-02-26 at 17:44 +0100, Bas Vermeulen wrote:
>> On 26-02-18 17:32, Larry Finger wrote:
>>> On 02/26/2018 04:07 AM, Bas Vermeulen wrote:
>>>> On 26-02-18 10:54, Kalle Valo wrote:
>>>>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>>>>
>>>>>> A random (little endian eeprom'd) ar9278 card didn't work on
>>>>>> my
>>>>>> PowerMac G5 without allowing the driver to byte-swap the
>>>>>> eeprom.
>>>>>>
>>>>>> Introduce a module parameter endian_check to allow this to
>>>>>> happen,
>>>>>> and the PCIe card to function correctly on BE powerpc.
>>>>>>
>>>>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>>>>> ---
>>>>>>    drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c
>>>>>> b/drivers/net/wireless/ath/ath9k/init.c
>>>>>> index fa58a32227f5..421039dc060a 100644
>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>>>>    module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>>>>    MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>>>> +static int ath9k_endian_check;
>>>>>> +module_param_named(endian_check, ath9k_endian_check, int,
>>>>>> 0444);
>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness
>>>>>> compatibility");
>>>>>>    #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>>>>    int ath9k_use_chanctx;
>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc
>>>>>> *sc)
>>>>>>            ether_addr_copy(common->macaddr, mac);
>>>>>>        ah->ah_flags &= ~AH_USE_EEPROM;
>>>>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>>> +    if (!ath9k_endian_check)
>>>>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>>>>> A bit annoying to have a module parameter, isn't there any
>>>>> automatic
>>>>> way
>>>>> to detect/try this? But on the other hand I guess this isn't a
>>>>> common
>>>>> problem as nobody has reported this before?
>>>> There is an automatic way to detect this, but that is disabled by
>>>> the
>>>> AH_NO_EEP_SWAP flag.
>>>> The platform initialisation does not set this flag if the
>>>> endian_check member of pdata is set
>>>> to true, but there is no way to not set this when using a device
>>>> tree. I used a module
>>>> parameter instead of a device tree variable because I don't know
>>>> of a
>>>> way to modify the
>>>> device tree my PowerMac boots with.
>>> Shouldn't you be able to set ath9k_endian_check inside #ifdef
>>> __BIG_ENDIAN ... #endif in the initialization? I think that would
>>> achieve the same functionality without requiring the user to set a
>>> module parameter.
>> I could have done that, but didn't want to change the default
>> behaviour.
>> This patch keeps the
>> current behaviour on all platforms if the module parameter is not
>> set. I
>> don't have the means
>> to test mips and other platforms this could be used on. I don't mind
>> having to set a module
>> parameter, I mind not being able to change the behaviour
> Still, module parameters are an awful user experience because you need
> to know that they exist, what they do, and whether you need it or not.
> It doesn't just work.
>
> Is there no way to autodetect the endian-ness of the firmware itself,
> and if the known machine endian-ness isn't the same then swap?  This
> seems like a solvable problem.

The problem is already solved, but the solution is disabled by the 
AH_NO_EEP_SWAP
flag set in both initialization functions. See ath9k/init.c and 
ath9k/eeprom.c.
My patch just disables the flag when a module parameter is set, which 
enables the solution.

I can disable the flag without a problem, but that might have unintended 
side effects
on some platforms. If the consensus is that's the better solution I can 
prepare a patch,
but that would have to come from someone more knowledgeable than me.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

Re: [PATCH] ath9k: introduce endian_check module parameter

[Sebastian Gottschall]

Am 26.02.2018 um 10:54 schrieb Kalle Valo:
> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>
>> A random (little endian eeprom'd) ar9278 card didn't work on my
>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>
>> Introduce a module parameter endian_check to allow this to happen,
>> and the PCIe card to function correctly on BE powerpc.
>>
>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>> ---
>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>> index fa58a32227f5..421039dc060a 100644
>> --- a/drivers/net/wireless/ath/ath9k/init.c
>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>   
>> +static int ath9k_endian_check;
>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility");
>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>   
>>   int ath9k_use_chanctx;
>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>   		ether_addr_copy(common->macaddr, mac);
>>   
>>   	ah->ah_flags &= ~AH_USE_EEPROM;
>> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>> +	if (!ath9k_endian_check)
>> +		ah->ah_flags |= AH_NO_EEP_SWAP;
> A bit annoying to have a module parameter, isn't there any automatic way
> to detect/try this? But on the other hand I guess this isn't a common
> problem as nobody has reported this before?
There is a way by simply checking the eeprom magic on this chipset
>

-- 
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH] ath9k: introduce endian_check module parameter

[Bas Vermeulen]

On 26-02-18 12:28, Sebastian Gottschall wrote:
> Am 26.02.2018 um 10:54 schrieb Kalle Valo:
>> Bas Vermeulen <bvermeul@blackstar.nl> writes:
>>
>>> A random (little endian eeprom'd) ar9278 card didn't work on my
>>> PowerMac G5 without allowing the driver to byte-swap the eeprom.
>>>
>>> Introduce a module parameter endian_check to allow this to happen,
>>> and the PCIe card to function correctly on BE powerpc.
>>>
>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/init.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c 
>>> b/drivers/net/wireless/ath/ath9k/init.c
>>> index fa58a32227f5..421039dc060a 100644
>>> --- a/drivers/net/wireless/ath/ath9k/init.c
>>> +++ b/drivers/net/wireless/ath/ath9k/init.c
>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable;
>>>   module_param_named(ps_enable, ath9k_ps_enable, int, 0444);
>>>   MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave");
>>>   +static int ath9k_endian_check;
>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444);
>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness 
>>> compatibility");
>>>   #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT
>>>     int ath9k_use_chanctx;
>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc)
>>>           ether_addr_copy(common->macaddr, mac);
>>>         ah->ah_flags &= ~AH_USE_EEPROM;
>>> -    ah->ah_flags |= AH_NO_EEP_SWAP;
>>> +    if (!ath9k_endian_check)
>>> +        ah->ah_flags |= AH_NO_EEP_SWAP;
>> A bit annoying to have a module parameter, isn't there any automatic way
>> to detect/try this? But on the other hand I guess this isn't a common
>> problem as nobody has reported this before?
> There is a way by simply checking the eeprom magic on this chipset

I am well aware.

The AH_NO_EEP_SWAP flag disables fixing the eeprom by swapping the data 
read from eeprom.
AH_NO_EEP_SWAP is enabled by default in ath9k_of_init() without this patch.
I am happy if the AH_NO_EEP_SWAP flag is not set, that would fix my 
problem, but changes the
current behaviour. I wanted to keep the current behaviour by default, 
and give me and others a
way to make it work on big endian machines with cards with little endian 
eeproms.

Bas Vermeulen

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.