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

[Johannes Berg <johannes@sipsolutions.net>]

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

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/

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


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

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

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


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

> +/* Undocumented PHY configuration parameters. */
> 

haha :)


Oh and before I forget, how about firmware availability?

johannes