diff mbox

[5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

Message ID 20160821144906.30984-6-martin.blumenstingl@googlemail.com
State Changes Requested, archived
Headers show

Commit Message

Martin Blumenstingl Aug. 21, 2016, 2:49 p.m. UTC 
The endianness swapping is disabled by default since many devices have
EEPROMs with incorrect endianness magic bytes (at the beginning).
Devices where the EEPROM is known to indicate the correct endianness
can enable the new flag to enable swapping when required.
This behavior is consistent with ath9k_platform_data where the endian
check also has to be enabled explicitly.
---
 .../devicetree/bindings/net/wireless/qca,ath9k.txt       | 16 ++++++++++++++++
 drivers/net/wireless/ath/ath9k/init.c                    |  6 +++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

Comments

Arnd Bergmann Aug. 22, 2016, 11:52 a.m. UTC | #1 
On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
> +                               endianness of the EEPROM (using two checks,
> +                               one is based on the two magic bytes at the
> +                               start of the EEPROM and a second one which
> +                               relies on a flag within the EEPROM data)
> +                               matches the host system's native endianness.
> +                               The data will be swapped accordingly if there
> +                               is a mismatch.
> +                               Leaving this disabled means that the EEPROM
> +                               data will always be interpreted in the
> +                               system's native endianness.
> +                               Enable this option only when the EEPROMs
> +                               endianness identifiers are known to be
> +                               correct, because otherwise the EEPROM data
> +                               may be swapped and thus interpreted
> +                               incorrectly.

The property should not describe the driver behavior, but instead
describe what the hardware is like.

A default of "system's native endianess" should not be specified
in a binding, as the same DTB can be used with both big-endian or
little-endian kernels on some architectures (ARM and PowerPC among
others), so disabling the property means that it is guaranteed to
be broken on one of the two.

If we have no reliable way to check the endianess in all combinations,
could we have two properties "eeprom-big-endian" and
"eeprom-little-endian" and mandate that one of them is always used
when discovering the device using DT?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Blumenstingl Aug. 28, 2016, 9:10 p.m. UTC | #2 
On Mon, Aug 22, 2016 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
>> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
>> +                               endianness of the EEPROM (using two checks,
>> +                               one is based on the two magic bytes at the
>> +                               start of the EEPROM and a second one which
>> +                               relies on a flag within the EEPROM data)
>> +                               matches the host system's native endianness.
>> +                               The data will be swapped accordingly if there
>> +                               is a mismatch.
>> +                               Leaving this disabled means that the EEPROM
>> +                               data will always be interpreted in the
>> +                               system's native endianness.
>> +                               Enable this option only when the EEPROMs
>> +                               endianness identifiers are known to be
>> +                               correct, because otherwise the EEPROM data
>> +                               may be swapped and thus interpreted
>> +                               incorrectly.
>
> The property should not describe the driver behavior, but instead
> describe what the hardware is like.
>
> A default of "system's native endianess" should not be specified
> in a binding, as the same DTB can be used with both big-endian or
> little-endian kernels on some architectures (ARM and PowerPC among
> others), so disabling the property means that it is guaranteed to
> be broken on one of the two.
OK, I went back to have a separate look at the whole issue again.
Let's recap, there are two types of swapping:
1. swab16 for the whole EEPROM data
2. EEPROM format specific swapping (for all u16 and u32 values)

Actually I am not 100% sure what #1 exists. In OpenWrt and LEDE it's
only used by the brcm63xx and lantiq targets (I personally have only
lantiq based devices, so that's what I can test with).
Without patch 4 from this series the EEPROM data is loaded like this:
1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
2. board specific entry (usually device-tree) tells the code to apply
swab16 to the data
3. board specific entry (usually device-tree again) sets the
"endian_check" property in the ath9k_platform_data to true
4.a ath9k sees that the magic bytes are not matching and applies swab16
4.b while doing that it notifies the EEPROM format specific swapping

However, with patch 4 from this series steps 4.a and 4.b are replaced with:
4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
format specific swapping

Currently this code is still guarded by a check whether swapping is
enabled or not.
However, FreeBSD uses the same check but has no guarding if-clause for it.

TL;DR: if we remove if (ah->ah_flags & AH_NO_EEP_SWAP) from patch 4 we
don't need an extra device-tree property.

The question is how we can test this properly:
I can test this on the boards I own (3 in total) - but I don't think
that this is enough.
Maybe we can test this together with some LEDE people - this should
get us test-coverage for embedded devices.
I'm open to more/better suggestions


Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 29, 2016, 12:10 p.m. UTC | #3 
On Sunday 28 August 2016, Martin Blumenstingl wrote:
> On Mon, Aug 22, 2016 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
> >> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
> >> +                               endianness of the EEPROM (using two checks,
> >> +                               one is based on the two magic bytes at the
> >> +                               start of the EEPROM and a second one which
> >> +                               relies on a flag within the EEPROM data)
> >> +                               matches the host system's native endianness.
> >> +                               The data will be swapped accordingly if there
> >> +                               is a mismatch.
> >> +                               Leaving this disabled means that the EEPROM
> >> +                               data will always be interpreted in the
> >> +                               system's native endianness.
> >> +                               Enable this option only when the EEPROMs
> >> +                               endianness identifiers are known to be
> >> +                               correct, because otherwise the EEPROM data
> >> +                               may be swapped and thus interpreted
> >> +                               incorrectly.
> >
> > The property should not describe the driver behavior, but instead
> > describe what the hardware is like.
> >
> > A default of "system's native endianess" should not be specified
> > in a binding, as the same DTB can be used with both big-endian or
> > little-endian kernels on some architectures (ARM and PowerPC among
> > others), so disabling the property means that it is guaranteed to
> > be broken on one of the two.
> OK, I went back to have a separate look at the whole issue again.
> Let's recap, there are two types of swapping:
> 1. swab16 for the whole EEPROM data
> 2. EEPROM format specific swapping (for all u16 and u32 values)

Right, this is part of what makes the suggested DT property
a bit problematic (it's not obvious which swap is referred to),
though the other part is more important.

Note that for #1, there isn't really a big-endian and a little-endian
variant, only one that is correct and one that is incorrect (i.e.
fields are in the wrong place). In either case, it should be
independent of the CPU endianess.

> Actually I am not 100% sure what #1 exists. In OpenWrt and LEDE it's
> only used by the brcm63xx and lantiq targets (I personally have only
> lantiq based devices, so that's what I can test with).

Ok.

> Without patch 4 from this series the EEPROM data is loaded like this:
> 1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
> 2. board specific entry (usually device-tree) tells the code to apply
> swab16 to the data
> 3. board specific entry (usually device-tree again) sets the
> "endian_check" property in the ath9k_platform_data to true
> 4.a ath9k sees that the magic bytes are not matching and applies swab16
> 4.b while doing that it notifies the EEPROM format specific swapping
> 
> However, with patch 4 from this series steps 4.a and 4.b are replaced with:
> 4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
> format specific swapping

I think the intention of the patch is right, but it needs to go further:
It seems wrong to have 'u32' members e.g. in

struct modal_eep_ar9287_header {
        u32 antCtrlChain[AR9287_MAX_CHAINS];
        u32 antCtrlCommon;

and then do a swab32() on them. I suspect these should just be __le32 
(and __le16, respectively where needed), using appropriate accessors
everywhere that those fields are being read instead of having one function
that tries to turn them into cpu-native ordering.

If we can manage to convert the code into doing this consistently,
maybe only the 16-bit swaps of the data stream are left over.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Blumenstingl Aug. 29, 2016, 7:45 p.m. UTC | #4 
On Mon, Aug 29, 2016 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Without patch 4 from this series the EEPROM data is loaded like this:
>> 1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
>> 2. board specific entry (usually device-tree) tells the code to apply
>> swab16 to the data
>> 3. board specific entry (usually device-tree again) sets the
>> "endian_check" property in the ath9k_platform_data to true
>> 4.a ath9k sees that the magic bytes are not matching and applies swab16
>> 4.b while doing that it notifies the EEPROM format specific swapping
>>
>> However, with patch 4 from this series steps 4.a and 4.b are replaced with:
>> 4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
>> format specific swapping
>
> I think the intention of the patch is right, but it needs to go further:
> It seems wrong to have 'u32' members e.g. in
>
> struct modal_eep_ar9287_header {
>         u32 antCtrlChain[AR9287_MAX_CHAINS];
>         u32 antCtrlCommon;
>
> and then do a swab32() on them. I suspect these should just be __le32
> (and __le16, respectively where needed), using appropriate accessors
> everywhere that those fields are being read instead of having one function
> that tries to turn them into cpu-native ordering.
I'm not sure if we want those fields to be __le32:
BIT(0) in the eepMisc field indicates whether to interpret the data as
big or little endian.
When this bit is set we would want these fields to be __be32 instead -
so I guess the current implementation is "OK" for this specific
use-case.

> If we can manage to convert the code into doing this consistently,
> maybe only the 16-bit swaps of the data stream are left over.
 we don't need the 16bit swap anymore - at least on the lantiq devices
that I know (not sure about other platforms / devices though).


Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 29, 2016, 9:25 p.m. UTC | #5 
On Monday 29 August 2016, Martin Blumenstingl wrote:
> > and then do a swab32() on them. I suspect these should just be __le32
> > (and __le16, respectively where needed), using appropriate accessors
> > everywhere that those fields are being read instead of having one function
> > that tries to turn them into cpu-native ordering.
> I'm not sure if we want those fields to be __le32:
> BIT(0) in the eepMisc field indicates whether to interpret the data as
> big or little endian.
> When this bit is set we would want these fields to be __be32 instead -
> so I guess the current implementation is "OK" for this specific
> use-case.

Ok, I see. It's still confusing because it's different from how other
drivers handle this (case in point: I was very confused by this). 

Do you see any downsides in changing the code to consistently annotate
the eeprom as either __le or __be (whichever is more common), using
the respective endianess accessors, and then doing the swap if the
data we read is the opposite way?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Blumenstingl Aug. 29, 2016, 10:07 p.m. UTC | #6 
On Mon, Aug 29, 2016 at 11:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 29 August 2016, Martin Blumenstingl wrote:
>> > and then do a swab32() on them. I suspect these should just be __le32
>> > (and __le16, respectively where needed), using appropriate accessors
>> > everywhere that those fields are being read instead of having one function
>> > that tries to turn them into cpu-native ordering.
>> I'm not sure if we want those fields to be __le32:
>> BIT(0) in the eepMisc field indicates whether to interpret the data as
>> big or little endian.
>> When this bit is set we would want these fields to be __be32 instead -
>> so I guess the current implementation is "OK" for this specific
>> use-case.
>
> Ok, I see. It's still confusing because it's different from how other
> drivers handle this (case in point: I was very confused by this).
lesson learned: specification should state that data is _either_ big
or little endian, no mix between these two

> Do you see any downsides in changing the code to consistently annotate
> the eeprom as either __le or __be (whichever is more common), using
> the respective endianess accessors, and then doing the swap if the
> data we read is the opposite way?
I am assuming you mean leNN_to_cpu() and beNN_to_cpu() here?

old logic:
- reading data: simply access the struct fields

LE system:
- LE EEPROM -> no swap will be applied
- BE EEPROM -> swab16 / swab32 the fields
BE system:
- LE EEPROM -> swab16 / swab32 the fields
- BE EEPROM -> no swap will be applied

new logic (assuming that we went for __le16/__le32 fields):
- reading data: use le16_to_cpu and le32_to_cpu for all fields

LE system:
- LE EEPROM -> no swap will be applied
- BE EEPROM -> be16_to_cpu / be32_to_cpu (or swab16 / swab32 as before)
BE system:
- LE EEPROM -> le16_to_cpu / le32_to_cpu (or swab16 / swab32 as before)
- BE EEPROM -> no swap will be applied

There is one downside of the "new approach" I can think of: you need
to swap the data twice in some cases (BE EEPROM on a BE machine).
- first swap while writing the data to __le16/__le32 fields
- second swap while reading the data from the __le16/__le32 fields
If you forget to swap a field in either place then things will be broken.

Maybe someone else wants to state his/her opinion on this - I guess
some fresh thoughts could help us a lot!


Regards,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 30, 2016, 7:57 a.m. UTC | #7 
On Tuesday 30 August 2016, Martin Blumenstingl wrote:
> new logic (assuming that we went for __le16/__le32 fields):
> - reading data: use le16_to_cpu and le32_to_cpu for all fields
> 
> LE system:
> - LE EEPROM -> no swap will be applied
> - BE EEPROM -> be16_to_cpu / be32_to_cpu (or swab16 / swab32 as before)
> BE system:
> - LE EEPROM -> le16_to_cpu / le32_to_cpu (or swab16 / swab32 as before)
> - BE EEPROM -> no swap will be applied

I think this should be:

LE and BE systems:
 - LE EEPROM -> no swap will be applied
 - BE EEPROM -> or swab16 / swab32

The upside of this is that we no longer care about what the CPU is,
and in my opinion that makes the code easier to understand.

> There is one downside of the "new approach" I can think of: you need
> to swap the data twice in some cases (BE EEPROM on a BE machine).
> - first swap while writing the data to __le16/__le32 fields
> - second swap while reading the data from the __le16/__le32 fields
> If you forget to swap a field in either place then things will be broken.

Correct. Fortunately, "make C=1" with sparse helps you find those bugs
at compile time.

> Maybe someone else wants to state his/her opinion on this - I guess
> some fresh thoughts could help us a lot!

Yes, that would be helpful. It's possible I'm missing something here,
or that changing this will just add confusion with other people.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
index 98065ad..05c54c4 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -17,6 +17,22 @@  Optional properties:
 			ath9k wireless chip (in this case the calibration /
 			EEPROM data will be loaded from userspace using the
 			kernel firmware loader).
+- qca,check-eeprom-endianness: When enabled, the driver checks if the
+				endianness of the EEPROM (using two checks,
+				one is based on the two magic bytes at the
+				start of the EEPROM and a second one which
+				relies on a flag within the EEPROM data)
+				matches the host system's native endianness.
+				The data will be swapped accordingly if there
+				is a mismatch.
+				Leaving this disabled means that the EEPROM
+				data will always be interpreted in the
+				system's native endianness.
+				Enable this option only when the EEPROMs
+				endianness identifiers are known to be
+				correct, because otherwise the EEPROM data
+				may be swapped and thus interpreted
+				incorrectly.
 - qca,disable-2ghz: Overrides the settings from the EEPROM and disables the
 			2.4GHz band. Setting this property is only needed
 			when the RF circuit does not support the 2.4GHz band
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index c123145..d123977 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -581,6 +581,11 @@  static int ath9k_of_init(struct ath_softc *sc)
 	if (of_property_read_bool(np, "qca,disable-5ghz"))
 		ah->disable_5ghz = true;
 
+	if (of_property_read_bool(np, "qca,check-eeprom-endianness"))
+		ah->ah_flags &= ~AH_NO_EEP_SWAP;
+	else
+		ah->ah_flags |= AH_NO_EEP_SWAP;
+
 	if (of_property_read_bool(np, "qca,no-eeprom")) {
 		/* ath9k-eeprom-<bus>-<id>.bin */
 		scnprintf(eeprom_name, sizeof(eeprom_name),
@@ -597,7 +602,6 @@  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;
 
 	return 0;
 }