Message-ID: <CAGhaMFO_f_bvFB+39-z6xVF+y446ONwm1ROHQ=rXj=s4MnL54w@mail.gmail.com>
Date: Fri, 25 Apr 2025 18:49:22 +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

Hi Johannes,
thanks for the review thus far!

Replies inline.

On Thu, Apr 24, 2025 at 5:07 PM Johannes Berg <johannes@...solutions.net> wrote:
>
> On Tue, 2025-04-22 at 19:59 +0200, Artur Rojek wrote:
> > Introduce support for Nordic Semiconductor nRF70 series wireless
> > companion IC.
>
> Seems simple enough ... but I notice you're not even adding a
> MAINTAINERS file entry. Does that mean you're not going to stick around
> to maintain it at all? I'm definitely _not_ going to. Please don't
> expect the community to.
>
> Are you doing this for your customers? Or are you just doing this a
> contract for someone who needs it? I don't really care all that much but
> contracts have a tendency to go away and then we're left with nothing
> upstream ...

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

>
> Also, related, what are your plans to help out with wireless in general,
> particularly reviews? You're building on the shoulders of everyone who
> did work before ... I'll do a _very_ cursory review, but if you want to
> get this merged I would expect you to also become a part of the
> community and help review other people's code:
>
> 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?

>
> > +config NRF70
> > +     tristate "Nordic Semiconductor nRF70 series wireless companion IC"
> > +     depends on CFG80211 && INET && SPI_MEM && CPU_LITTLE_ENDIAN
>
> 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. This makes it a challenge to prepare said
messages on a big-endian system. I am aware of the packing API [2],
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).
So I decided that until someone actually needs to use nRF70 on
a big-endian machine, implementation of big-endian support can be
postponed.
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)?

>
>
> > +#define      NRF70_RADIOTAP_PRESENT_FIELDS                           \
> > +     cpu_to_le32((1 << IEEE80211_RADIOTAP_RATE) |            \
> > +                 (1 << IEEE80211_RADIOTAP_CHANNEL) |         \
> > +                 (1 << IEEE80211_RADIOTAP_DBM_ANTSIGNAL))
>
> You did some work on making it little endian properly ..
>
>
> > +
> > +#define      NRF70_FW_FEATURE_RAW_MODE       BIT(3)
> > +struct __packed nrf70_fw_header {
> > +     u32 signature;
> > +     u32 num_images;
> > +     u32 version;
> > +     u32 feature_flags;
> > +     u32 length;
> > +     u8 hash[NRF70_FW_HASH_LEN];
> > +     u8 data[];
> > +};
> > +
> > +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;

I then went and applied the same style elsewhere in the driver. I guess
I can restore the latter syntax where it makes sense.

>
> > +static int nrf70_verify_firmware(struct device *dev,
> > +                              const struct nrf70_fw_header *fw)
>
>
> What's the point in doing this? The hash is trivially adjusted if
> someone wants to play with the file, if hw doesn't check anything, and
> ... not sure we really need such a thing for "file is corrupt by
> accident"? *shrug*

No idea if the hw does any verification of the hash, but sure, I can
drop this.

>
> > +     ret = request_firmware(&firmware, "nrf70.bin", dev);
>
>
> You might want to make that async so that the driver can be built-in
> without requiring the firmware to also be built-in.
>
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to request firmware: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     header = (const struct nrf70_fw_header *)firmware->data;
>
> (const void *) cast would probably be sufficient
>
>
> > +     return ret ? ret : (wait_for_completion_timeout(&priv->init_done, HZ) ?
> > +                         0 : -ETIMEDOUT);
>
> that construct seems a bit questionable :)
>
>
> > +static void nrf70_handle_rx_mgmt(struct spi_mem *mem,
> > +                              struct nrf70_event_mlme *ev)
> > +{
> > +     struct nrf70_priv *priv = spi_mem_get_drvdata(mem);
> > +     struct nrf70_vif *vif = nrf70_get_vif(priv, ev->header.idx.wdev_id);
> > +
> > +     if (IS_ERR(vif))
> > +             return;
> > +
> > +     (void)cfg80211_rx_mgmt(&vif->wdev, ev->frequency, ev->rx_signal_dbm,
> > +                            ev->frame.data, ev->frame.len, ev->wifi_flags);
>
>
> shouldn't need the (void) cast?
>
>
> > +static int nrf70_change_bss(struct wiphy *wiphy, struct net_device *ndev,
> > +                         struct bss_parameters *params)
>
>
> See also this discussion:
> https://lore.kernel.org/linux-wireless/29fa5ea7f4cc177bed823ec3489d610e1d69a08f.camel@sipsolutions.net/
>
> > +static int nrf70_dequeue_umac_event(struct spi_mem *mem, void *data)
> > +{
> > +     struct nrf70_priv *priv = spi_mem_get_drvdata(mem);
> > +     struct device *dev = &mem->spi->dev;
> > +     struct nrf70_umac_header *header = data;
> > +     struct nrf70_vif *vif = nrf70_get_vif(priv, header->idx.wdev_id);
> > +     struct cfg80211_scan_info scan_info = { .aborted = true };
> > +
> > +     if (IS_ERR(vif))
> > +             return PTR_ERR(vif);
> > +
> > +     switch (header->id) {
> > +     case NRF70_UMAC_EVENT_TRIGGER_SCAN_START:
> > +             break;
>
>
> This sounds like you pretty much built the firmware for cfg80211 ;-)

That's because the firmware *is* cfg80211. Perhaps I am opening a can of
worms here, 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 :)

>
>
> > +#define      NRF70_MSG_SYSTEM                0
> > +#define      NRF70_MSG_DATA                  2
> > +#define      NRF70_MSG_UMAC                  3
> > +
> > +struct __packed nrf70_msg {
> > +     u32 len;
> > +     u32 resubmit;
> > +     u32 type;
> > +     u8 data[];
>
>
> similar comments here throughout this entire file wrt __packed and
> __le32, obviously

Addressed above wrt __packed.

>
> > +/* Undocumented PHY configuration parameters. */
> >
>
> haha :)

Yep :)
To be clear on the development process of this driver, no proprietary
documentation has been used. I've written it entirely based on the
publicly available Nordic's SDK [4], their Zephyr driver [5], the
aforementioned UMAC source code, and a fair amount of guess work. This
means there is some undocumented stuff available only as magic numbers.

>
>
> Oh and before I forget, how about firmware availability?

Nordic gave us permission to upstream it, see the cover letter.

PS. I was oblivious to the specific patch submission rules for
linux-wireless until after I've sent v2 series. Sorry for any
inconvenience! The v3 will be formatted appropriately.

Cheers,
Artur

[1] https://conclusive.tech/products/kstr-imx93-sbc/
[2] https://www.kernel.org/doc/html/latest/core-api/packing.html
[3] https://files.nordicsemi.com/ui/native/developerDoc/external/oss/nRF700x/
[4] https://github.com/nrfconnect/sdk-nrfxlib/tree/v2.7-branch/nrf_wifi
[5] https://github.com/zephyrproject-rtos/zephyr/tree/main/drivers/wifi/nrf_wifi

>
> johannes

Powered by blists - more mailing lists