[PATCH v2 0/2] ath9k: of_init: add endian check

[PATCH v2 0/2] ath9k: of_init: add endian check

[Álvaro Fernández Rojas]

Add new endian check flag to allow checking the endianness of EEPROM and swap
its values if needed.
This flag was already present on ath9k_platform_data as "endian_check".

For compatibility with current devices the AH_NO_EEP_SWAP flag will be
activated only when qca,endian-check isn't present in the device tree.
This is because some devices have the magic values swapped but not the actual
EEPROM data, so activating the flag for those devices will break them.

Álvaro Fernández Rojas (2):
  dt-bindings: net: wireless: ath9k: document endian check
  ath9k: fix calibration data endianness

 .../devicetree/bindings/net/wireless/qca,ath9k.yaml          | 5 +++++
 drivers/net/wireless/ath/ath9k/init.c                        | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] dt-bindings: net: wireless: ath9k: document endian check

[Álvaro Fernández Rojas]

Document new endian check flag to allow checking the endianness of EEPROM and
swap its values if needed.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 .../devicetree/bindings/net/wireless/qca,ath9k.yaml          | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
index 0e5412cff2bc..ff9ca5e3674b 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
@@ -44,6 +44,11 @@ properties:
 
   ieee80211-freq-limit: true
 
+  qca,endian-check:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that the EEPROM endianness should be checked
+
   qca,no-eeprom:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.30.2

Re: [PATCH v2 1/2] dt-bindings: net: wireless: ath9k: document endian check

[Krzysztof Kozlowski]

On 17/04/2023 07:35, Álvaro Fernández Rojas wrote:
> Document new endian check flag to allow checking the endianness of EEPROM and
> swap its values if needed.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed the lists so this won't be tested. Resend following Linux
kernel submission process.


> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml          | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> index 0e5412cff2bc..ff9ca5e3674b 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> @@ -44,6 +44,11 @@ properties:
>  
>    ieee80211-freq-limit: true
>  
> +  qca,endian-check:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the EEPROM endianness should be checked

Does not look like hardware property. Do not instruct what driver should
or should not do. It's not the purpose of DT.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: wireless: ath9k: document endian check

[Kalle Valo]

Krzysztof Kozlowski <krzk@kernel.org> writes:

>> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>> @@ -44,6 +44,11 @@ properties:
>>  
>>    ieee80211-freq-limit: true
>>  
>> +  qca,endian-check:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates that the EEPROM endianness should be checked
>
> Does not look like hardware property. Do not instruct what driver should
> or should not do. It's not the purpose of DT.

Also the documentation is vague and is not really telling anything new
what could be figured out from the property name, I would not be able to
understand just from this what value should be used.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 1/2] dt-bindings: net: wireless: ath9k: document endian check

[Álvaro Fernández Rojas]

El lun, 17 abr 2023 a las 9:20, Krzysztof Kozlowski
(<krzk@kernel.org>) escribió:
>
> On 17/04/2023 07:35, Álvaro Fernández Rojas wrote:
> > Document new endian check flag to allow checking the endianness of EEPROM and
> > swap its values if needed.
> >
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.

I forgot to get the updated list for v2, sorry for that!

>
> You missed the lists so this won't be tested. Resend following Linux
> kernel submission process.

Looks like we will need v3 anyway, so I'll get all the maintainers in
the next version.

>
>
> > ---
> >  .../devicetree/bindings/net/wireless/qca,ath9k.yaml          | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > index 0e5412cff2bc..ff9ca5e3674b 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > @@ -44,6 +44,11 @@ properties:
> >
> >    ieee80211-freq-limit: true
> >
> > +  qca,endian-check:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that the EEPROM endianness should be checked
>
> Does not look like hardware property. Do not instruct what driver should
> or should not do. It's not the purpose of DT.
>
>
> Best regards,
> Krzysztof
>

[PATCH v2 2/2] ath9k: of_init: add endian check

[Álvaro Fernández Rojas]

BCM63xx (Big Endian MIPS) devices store the calibration data in MTD
partitions but it needs to be swapped in order to work, otherwise it fails:
ath9k 0000:00:01.0: enabling device (0000 -> 0002)
ath: phy0: Ignoring endianness difference in EEPROM magic bytes.
ath: phy0: Bad EEPROM VER 0x0001 or REV 0x00e0
ath: phy0: Unable to initialize hardware; initialization status: -22
ath9k 0000:00:01.0: Failed to initialize device
ath9k: probe of 0000:00:01.0 failed with error -22

For compatibility with current devices the AH_NO_EEP_SWAP flag will be
activated only when qca,endian-check isn't present in the device tree.
This is because some devices have the magic values swapped but not the actual
EEPROM data, so activating the flag for those devices will break them.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 drivers/net/wireless/ath/ath9k/init.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 4f00400c7ffb..abde953aec61 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -615,7 +615,6 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
 
 	ah->nvmem_blob_len = len;
 	ah->ah_flags &= ~AH_USE_EEPROM;
-	ah->ah_flags |= AH_NO_EEP_SWAP;
 
 	return 0;
 }
@@ -688,9 +687,11 @@ static int ath9k_of_init(struct ath_softc *sc)
 			return ret;
 
 		ah->ah_flags &= ~AH_USE_EEPROM;
-		ah->ah_flags |= AH_NO_EEP_SWAP;
 	}
 
+	if (!of_property_read_bool(np, "qca,endian-check"))
+		ah->ah_flags |= AH_NO_EEP_SWAP;
+
 	of_get_mac_address(np, common->macaddr);
 
 	return 0;
-- 
2.30.2

Re: [PATCH v2 2/2] ath9k: of_init: add endian check

[Toke Høiland-Jørgensen]

Álvaro Fernández Rojas <noltari@gmail.com> writes:

> BCM63xx (Big Endian MIPS) devices store the calibration data in MTD
> partitions but it needs to be swapped in order to work, otherwise it fails:
> ath9k 0000:00:01.0: enabling device (0000 -> 0002)
> ath: phy0: Ignoring endianness difference in EEPROM magic bytes.
> ath: phy0: Bad EEPROM VER 0x0001 or REV 0x00e0
> ath: phy0: Unable to initialize hardware; initialization status: -22
> ath9k 0000:00:01.0: Failed to initialize device
> ath9k: probe of 0000:00:01.0 failed with error -22
>
> For compatibility with current devices the AH_NO_EEP_SWAP flag will be
> activated only when qca,endian-check isn't present in the device tree.
> This is because some devices have the magic values swapped but not the actual
> EEPROM data, so activating the flag for those devices will break them.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  drivers/net/wireless/ath/ath9k/init.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index 4f00400c7ffb..abde953aec61 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -615,7 +615,6 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
>  
>  	ah->nvmem_blob_len = len;
>  	ah->ah_flags &= ~AH_USE_EEPROM;
> -	ah->ah_flags |= AH_NO_EEP_SWAP;
>  
>  	return 0;
>  }
> @@ -688,9 +687,11 @@ static int ath9k_of_init(struct ath_softc *sc)
>  			return ret;
>  
>  		ah->ah_flags &= ~AH_USE_EEPROM;
> -		ah->ah_flags |= AH_NO_EEP_SWAP;
>  	}
>  
> +	if (!of_property_read_bool(np, "qca,endian-check"))
> +		ah->ah_flags |= AH_NO_EEP_SWAP;
> +

So I'm not sure just setting (or not) this flag actually leads to
consistent behaviour. The code in ath9k_hw_nvram_swap_data() that reacts
to this flag does an endianness check before swapping, and the behaviour
of this check depends on the CPU endianness. However, the byte swapping
you're after here also swaps u8 members of the eeprom, so it's not
really a data endianness swap, and I don't think it should depend on the
endianness of the CPU?

So at least conceptually, the magic byte check in
ath9k_hw_nvram_swap_data() is wrong; instead the byteswap check should
just be checking against the little-endian version of the firmware
(i.e., 0xa55a; I think that's what your device has, right?). However,
since we're setting an explicit per-device property anyway (in the
device tree), maybe it's better to just have that be an "eeprom needs
swapping" flag and do the swap unconditionally if it's set? I think that
would address Krzysztof's comment as well ("needs swapping" is a
hardware property, "do the check" is not).

Now, the question becomes whether the "check" code path is actually used
for anything today? The old mail thread I quoted in the other thread
seems to indicate it's not, but it's not quite clear from the code
whether there's currently any way to call into
ath9k_hw_nvram_swap_data() without the NO_EEP_SWAP flag being set?

WDYT?

-Toke

Re: [PATCH v2 2/2] ath9k: of_init: add endian check

[Álvaro Fernández Rojas]

El lun, 17 abr 2023 a las 11:21, Toke Høiland-Jørgensen
(<toke@toke.dk>) escribió:
>
> Álvaro Fernández Rojas <noltari@gmail.com> writes:
>
> > BCM63xx (Big Endian MIPS) devices store the calibration data in MTD
> > partitions but it needs to be swapped in order to work, otherwise it fails:
> > ath9k 0000:00:01.0: enabling device (0000 -> 0002)
> > ath: phy0: Ignoring endianness difference in EEPROM magic bytes.
> > ath: phy0: Bad EEPROM VER 0x0001 or REV 0x00e0
> > ath: phy0: Unable to initialize hardware; initialization status: -22
> > ath9k 0000:00:01.0: Failed to initialize device
> > ath9k: probe of 0000:00:01.0 failed with error -22
> >
> > For compatibility with current devices the AH_NO_EEP_SWAP flag will be
> > activated only when qca,endian-check isn't present in the device tree.
> > This is because some devices have the magic values swapped but not the actual
> > EEPROM data, so activating the flag for those devices will break them.
> >
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > ---
> >  drivers/net/wireless/ath/ath9k/init.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> > index 4f00400c7ffb..abde953aec61 100644
> > --- a/drivers/net/wireless/ath/ath9k/init.c
> > +++ b/drivers/net/wireless/ath/ath9k/init.c
> > @@ -615,7 +615,6 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
> >
> >       ah->nvmem_blob_len = len;
> >       ah->ah_flags &= ~AH_USE_EEPROM;
> > -     ah->ah_flags |= AH_NO_EEP_SWAP;
> >
> >       return 0;
> >  }
> > @@ -688,9 +687,11 @@ static int ath9k_of_init(struct ath_softc *sc)
> >                       return ret;
> >
> >               ah->ah_flags &= ~AH_USE_EEPROM;
> > -             ah->ah_flags |= AH_NO_EEP_SWAP;
> >       }
> >
> > +     if (!of_property_read_bool(np, "qca,endian-check"))
> > +             ah->ah_flags |= AH_NO_EEP_SWAP;
> > +
>
> So I'm not sure just setting (or not) this flag actually leads to
> consistent behaviour. The code in ath9k_hw_nvram_swap_data() that reacts
> to this flag does an endianness check before swapping, and the behaviour
> of this check depends on the CPU endianness. However, the byte swapping
> you're after here also swaps u8 members of the eeprom, so it's not
> really a data endianness swap, and I don't think it should depend on the
> endianness of the CPU?
>
> So at least conceptually, the magic byte check in
> ath9k_hw_nvram_swap_data() is wrong; instead the byteswap check should
> just be checking against the little-endian version of the firmware
> (i.e., 0xa55a; I think that's what your device has, right?). However,
> since we're setting an explicit per-device property anyway (in the
> device tree), maybe it's better to just have that be an "eeprom needs
> swapping" flag and do the swap unconditionally if it's set? I think that
> would address Krzysztof's comment as well ("needs swapping" is a
> hardware property, "do the check" is not).

Yes, you're right, it's probably better to introduce a new and more
clear flag that swaps the content inconditionally.

>
> Now, the question becomes whether the "check" code path is actually used
> for anything today? The old mail thread I quoted in the other thread
> seems to indicate it's not, but it's not quite clear from the code
> whether there's currently any way to call into
> ath9k_hw_nvram_swap_data() without the NO_EEP_SWAP flag being set?

It's only used when endian_check is enabled in ath9k_platform_data:
https://github.com/torvalds/linux/blob/6a8f57ae2eb07ab39a6f0ccad60c760743051026/drivers/net/wireless/ath/ath9k/init.c#L645
We're currently using it on OpenWrt for bmips:
https://github.com/Noltari/openwrt/blob/457549665fcb93667453ef48c50bf43eddd776ef/target/linux/bmips/files/arch/mips/bmips/ath9k-fixup.c#L198-L199

>
> WDYT?
>
> -Toke

Re: [PATCH v2 2/2] ath9k: of_init: add endian check

[Toke Høiland-Jørgensen]

Álvaro Fernández Rojas <noltari@gmail.com> writes:

> El lun, 17 abr 2023 a las 11:21, Toke Høiland-Jørgensen
> (<toke@toke.dk>) escribió:
>>
>> Álvaro Fernández Rojas <noltari@gmail.com> writes:
>>
>> > BCM63xx (Big Endian MIPS) devices store the calibration data in MTD
>> > partitions but it needs to be swapped in order to work, otherwise it fails:
>> > ath9k 0000:00:01.0: enabling device (0000 -> 0002)
>> > ath: phy0: Ignoring endianness difference in EEPROM magic bytes.
>> > ath: phy0: Bad EEPROM VER 0x0001 or REV 0x00e0
>> > ath: phy0: Unable to initialize hardware; initialization status: -22
>> > ath9k 0000:00:01.0: Failed to initialize device
>> > ath9k: probe of 0000:00:01.0 failed with error -22
>> >
>> > For compatibility with current devices the AH_NO_EEP_SWAP flag will be
>> > activated only when qca,endian-check isn't present in the device tree.
>> > This is because some devices have the magic values swapped but not the actual
>> > EEPROM data, so activating the flag for those devices will break them.
>> >
>> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> > ---
>> >  drivers/net/wireless/ath/ath9k/init.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
>> > index 4f00400c7ffb..abde953aec61 100644
>> > --- a/drivers/net/wireless/ath/ath9k/init.c
>> > +++ b/drivers/net/wireless/ath/ath9k/init.c
>> > @@ -615,7 +615,6 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
>> >
>> >       ah->nvmem_blob_len = len;
>> >       ah->ah_flags &= ~AH_USE_EEPROM;
>> > -     ah->ah_flags |= AH_NO_EEP_SWAP;
>> >
>> >       return 0;
>> >  }
>> > @@ -688,9 +687,11 @@ static int ath9k_of_init(struct ath_softc *sc)
>> >                       return ret;
>> >
>> >               ah->ah_flags &= ~AH_USE_EEPROM;
>> > -             ah->ah_flags |= AH_NO_EEP_SWAP;
>> >       }
>> >
>> > +     if (!of_property_read_bool(np, "qca,endian-check"))
>> > +             ah->ah_flags |= AH_NO_EEP_SWAP;
>> > +
>>
>> So I'm not sure just setting (or not) this flag actually leads to
>> consistent behaviour. The code in ath9k_hw_nvram_swap_data() that reacts
>> to this flag does an endianness check before swapping, and the behaviour
>> of this check depends on the CPU endianness. However, the byte swapping
>> you're after here also swaps u8 members of the eeprom, so it's not
>> really a data endianness swap, and I don't think it should depend on the
>> endianness of the CPU?
>>
>> So at least conceptually, the magic byte check in
>> ath9k_hw_nvram_swap_data() is wrong; instead the byteswap check should
>> just be checking against the little-endian version of the firmware
>> (i.e., 0xa55a; I think that's what your device has, right?). However,
>> since we're setting an explicit per-device property anyway (in the
>> device tree), maybe it's better to just have that be an "eeprom needs
>> swapping" flag and do the swap unconditionally if it's set? I think that
>> would address Krzysztof's comment as well ("needs swapping" is a
>> hardware property, "do the check" is not).
>
> Yes, you're right, it's probably better to introduce a new and more
> clear flag that swaps the content inconditionally.
>
>>
>> Now, the question becomes whether the "check" code path is actually used
>> for anything today? The old mail thread I quoted in the other thread
>> seems to indicate it's not, but it's not quite clear from the code
>> whether there's currently any way to call into
>> ath9k_hw_nvram_swap_data() without the NO_EEP_SWAP flag being set?
>
> It's only used when endian_check is enabled in ath9k_platform_data:
> https://github.com/torvalds/linux/blob/6a8f57ae2eb07ab39a6f0ccad60c760743051026/drivers/net/wireless/ath/ath9k/init.c#L645

Right, I found that, but was looking for anywhere that sets it, and
couldn't find that anywhere. So this exists solely for the use of out of
tree code?

> We're currently using it on OpenWrt for bmips:
> https://github.com/Noltari/openwrt/blob/457549665fcb93667453ef48c50bf43eddd776ef/target/linux/bmips/files/arch/mips/bmips/ath9k-fixup.c#L198-L199

Hmm, but that is arguably wrong in the same way, in that it triggers the
code in ath9k_hw_nvram_swap_data() which swaps or not depending on the
CPU endianness.

Is the magic byte check actually useful for that user? I.e., are there
some devices that have 0xa55a as magic bytes, and some that have 0x5aa5?
Or could the device tree flag in that fixup driver above be replaced
with the one you're proposing here?

-Toke