Message-ID: <CAGhaMFOt01SXNejP9a07cQ-0czFGGbm0S08EBevO=dno0UpQ=A@mail.gmail.com>
Date: Thu, 1 May 2025 12:34:32 +0200
From: Artur Rojek <artur@...clusive.pl>
To: Johannes Berg <johannes@...solutions.net>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, 
	Mark Brown <broonie@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	linux-wireless@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Jakub Klama <jakub@...clusive.pl>, 
	Wojciech Kloska <wojciech@...clusive.pl>, Ulf Axelsson <ulf.axelsson@...dicsemi.no>
Subject: Re: [RFC PATCH v2 2/2] wifi: Add Nordic nRF70 series Wi-Fi driver

On Fri, Apr 25, 2025 at 8:09 PM Johannes Berg <johannes@...solutions.net> 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@sipsolutions.net/
> >
> > 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

Powered by blists - more mailing lists