On Fri, Apr 25, 2025 at 8:09 PM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > > Hi, > > > This is commercial work. I am employed by Conclusive Engineering, and > > was tasked with writing this driver. It was done for our internal needs > > (we sell hardware [1] with nRF70 on-board), however I was also asked to > > send the series upstream. > > Makes sense. > > > Nordic showed interest in this work, hence why their representative is > > CCd to this conversation. They agreed to use our hardware as a reference > > board for nRF70 used in Linux context. > > :) > > > I fully understand your concerns with maintenance (I am privately > > a kernel contributor as well), and discussed this topic internally with > > appropriate decision making people. They understand the responsibilities > > involved and agreed to allocate time for me to support this driver long > > term. As such, I will add myself to MAINTAINERS in v3. > > Cool good to hear :) > > > > https://lore.kernel.org/linux-wireless/21896d2788b8bc6c7fcb534cd43e75671a57f494.camel@xxxxxxxxxxxxxxxx/ > > > > Bearing in mind above time constraints, I have no objections to helping > > out. That said, this is my first Wi-Fi driver, and as such I am not that > > familiar with the cfg80211 subsystem (hence why this series is RFC), so > > my expertise will be limited at best. > > What sort of help would you expect from me with the reviews? > > I'm just handwaving I guess ;-) It'd just be good to see people a bit > involved in the community. Some apparently don't even have anyone who > follows the list and what happens in the community at all. > > But it's a bit of a tit-for-tat thing - why would anyone review your > code? Why would you even expect anyone to? The already overloaded > maintainer? But on the other hand PHBs often think that sending their > code upstream magically makes it better. There's real effort involved in > keeping that true. :) > > > > That CPU_LITTLE_ENDIAN seems like a cop-out. Do we really want that? > > > Asking not specifically you I guess... > > > > I addressed this in the cover letter (Patch 0/2), but nRF70 communicates > > using little-endian, byte packed messages, where each message type has > > a unique set of fields. > > Sorry. Reading comprehension: 1, Johannes: 0. Guess I should look at > that and reply there too. > > > This makes it a challenge to prepare said > > messages on a big-endian system. I am aware of the packing API [2], > > I'm not familiar with it, tbh. > > > however a cursory look at it indicates that I would need to provide > > custom code for each and every message (there's almost 150 of those in > > total, even if the driver doesn't support all of them at the moment - > > take a look at nrf70_cmds.h). > > Looking at this, I don't see why? They're all just fixed structures? The > packing API appears (I never saw it before) to be for some form of bit- > packing, I'd think? > > Looking at how you define the structures and how you use them, I'd say > all you need to do is replace u32/s32 by __le32, u16 by __le16, and then > fix the resulting sparse warnings by doing cpu_to_leXY()? > > > Unless the __packed attribute is guaranteed to align the bytes the same > > way regardless of the endianness, and so calling cpu_to_le* for every > > field of a message is good enough (these messages are byte packed, not > > bit packed)? > > Now I'm confused, what did you think would happen? If you have > > struct foo { > u16 a; > u32 b; > } __packed; > > you will get the 6 bytes, regardless of whether that's __le16/__le32 or > u16/u32. 'a' will be two bytes, and 'b' will be 4 bytes, and all you > need to do is convert the individual fields? > > Maybe I don't understand the question. No, you're right. I somehow imagined that unaligned access will play a role, but access to packed members should be completely independent from byte swapping the lvalues. PS. I expected this to be more complicated, but it turns out aarch64 can switch endianness at runtime, so I can continue testing on the same hardware as before. All I had to do is tick CONFIG_CPU_BIG_ENDIAN=y, rebuild the rootfs for BE, and do minor hacks to the QSPI controller driver, and now nRF70 driver is beginning to work with BE. On a side note, I am taking a few days off, so v3 will come with a slight delay. Cheers, Artur > > > > > +struct __packed nrf70_fw_img { > > > > + u32 type; > > > > + u32 length; > > > > + u8 data[]; > > > > +}; > > > > > > making the u32's here __le32's (and fixing sparse) would probably go a > > > long way of making it endian clean. The __packed is also placed oddly. > > > > When declaring structure members for the messages (in nrf70_cmds.h), > > I noticed that this attribute has to go before the braces: > > > struct __packed { ... } name; > > rather than after braces: > > > struct { ... } __packed name; > > Wait .. that syntax isn't right either way? But it can be > > struct name { ... } __packed; > > and that's what roughly everyone else does? > > > > This sounds like you pretty much built the firmware for cfg80211 ;-) > > > > That's because the firmware *is* cfg80211. > > Actually it appears to be also mac80211? > > > Perhaps I am opening a can of worms here, > > Heh, I guess. > > > but it has to be opened at some point during firmware > > upstream. From what I've seen, part of the nRF70 firmware (called UMAC) > > is derived from the cfg80211 project. Nordic makes the source code > > publicly available at this location [3]. I have also asked Nordic to > > provide a matching version of the source code for the fw blob they will > > be upstreaming to the linux-firmware project (I believe I will be > > assisting in that process as well). I hope everything there is dandy > > license-wise, as I am not a lawyer :) > > Neither am I but the SFC says you have to have a way to build it. That > might be a real challenge since this integrates cfg80211/mac80211 (GPL) > and clearly unpublished proprietary code ("lmac"). > > Nordic folks might want to consult their lawyers on this. > > johannes
