Re: [RFC PATCH v2 2/2] wifi: Add Nordic nRF70 series Wi-Fi driver

[Johannes Berg]

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.

> > > +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