netdev - Re: [PATCH v2 2/2] ath9k: of

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87jzyaw210.fsf@toke.dk>
Date:   Tue, 18 Apr 2023 00:56:59 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Álvaro Fernández Rojas <noltari@...il.com>
Cc:     f.fainelli@...il.com, jonas.gorski@...il.com, nbd@....name,
        kvalo@...nel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, chunkeey@...il.com,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ath9k: of_init: add endian check

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

> El lun, 17 abr 2023 a las 11:21, Toke Høiland-Jørgensen
> (<toke@...e.dk>) escribió:
>>
>> Álvaro Fernández Rojas <noltari@...il.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@...il.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