[PATCH 0/6] wifi: wilc1000: fix default mac address

[PATCH 0/6] wifi: wilc1000: fix default mac address

[Alexis Lothoré]

This series aims to fix invalid mac address issue raised by Thiery Heiko
([1]). WILC1000 mac address is currently not set until device is opened, at
which point firmware is loaded and started. This results in default mac
address being 00:00:00:00:00:00.

This series, based on an initial patch from Ajay, reads the MAC address
from chip eFuse whenever we set the first interface (at probe time). To do
so, we need to ensure that any bus communication is properly initialized,
so the first commits slightly rearrange/fix initialization/registration
order to allow to read MAC address properly.
Based on the tests I did during this series adjustments, there are still a
few corner cased not properly handled, especially when dealing with two
interfaces on top of the same wphy, but it fixes at least the user-facing
mac address for those interfaces so user space network managers are not
confused anymore.

[1] https://lore.kernel.org/netdev/CAEyMn7aV-B4OEhHR4Ad0LM3sKCz1-nDqSb9uZNmRWR-hMZ=z+A@mail.gmail.com/

---
Ajay Singh (1):
      wifi: wilc1000: read MAC address from fuse at probe

Alexis Lothoré (5):
      wifi: wilc1000: set net device registration as last step during interface creation
      wifi: wilc1000: register net device only after bus being fully initialized
      wifi: wilc1000: set wilc_set_mac_address parameter as const
      wifi: wilc1000: add function to read mac address from eFuse
      wifi: wilc1000: make sdio deinit function really deinit the sdio card

 drivers/net/wireless/microchip/wilc1000/cfg80211.c | 10 ---
 drivers/net/wireless/microchip/wilc1000/fw.h       | 13 ++++
 drivers/net/wireless/microchip/wilc1000/hif.c      |  4 +-
 drivers/net/wireless/microchip/wilc1000/hif.h      |  2 +-
 drivers/net/wireless/microchip/wilc1000/netdev.c   | 71 ++++++++++++----------
 drivers/net/wireless/microchip/wilc1000/netdev.h   |  2 +
 drivers/net/wireless/microchip/wilc1000/sdio.c     | 71 +++++++++++++++++++++-
 drivers/net/wireless/microchip/wilc1000/spi.c      | 17 +++++-
 drivers/net/wireless/microchip/wilc1000/wlan.c     | 48 +++++++++++++++
 drivers/net/wireless/microchip/wilc1000/wlan.h     |  1 +
 10 files changed, 191 insertions(+), 48 deletions(-)
---
base-commit: db5037bd459adeba309af1f97fd5c9b6a0788b2a
change-id: 20231221-mac_addr_at_probe-3cb6044b251d

Best regards,
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 1/6] wifi: wilc1000: set net device registration as last step during interface creation

[Alexis Lothoré]

net device registration is currently done in wilc_netdev_ifc_init but
other initialization operations are still done after this registration.
Since net device is assumed to be usable right after registration, it
should be the very last step of initialization.

Move netdev registration at the very end of wilc_netdev_ifc_init to let
this function completely initialize netdevice before registering it.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/netdev.c | 31 ++++++++++++------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 73f56f7b002b..acc9b9a64552 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -965,16 +965,6 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
 	vif->priv.wdev.iftype = type;
 	vif->priv.dev = ndev;
 
-	if (rtnl_locked)
-		ret = cfg80211_register_netdevice(ndev);
-	else
-		ret = register_netdev(ndev);
-
-	if (ret) {
-		ret = -EFAULT;
-		goto error;
-	}
-
 	ndev->needs_free_netdev = true;
 	vif->iftype = vif_type;
 	vif->idx = wilc_get_available_idx(wl);
@@ -985,13 +975,24 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
 	mutex_unlock(&wl->vif_mutex);
 	synchronize_rcu();
 
-	return vif;
-
-error:
 	if (rtnl_locked)
-		cfg80211_unregister_netdevice(ndev);
+		ret = cfg80211_register_netdevice(ndev);
 	else
-		unregister_netdev(ndev);
+		ret = register_netdev(ndev);
+
+	if (ret) {
+		ret = -EFAULT;
+		goto error_remove_vif;
+	}
+
+	return vif;
+
+error_remove_vif:
+	mutex_lock(&wl->vif_mutex);
+	list_del_rcu(&vif->list);
+	wl->vif_num -= 1;
+	mutex_unlock(&wl->vif_mutex);
+	synchronize_rcu();
 	free_netdev(ndev);
 	return ERR_PTR(ret);
 }

-- 
2.44.0

[PATCH 2/6] wifi: wilc1000: register net device only after bus being fully initialized

[Alexis Lothoré]

SDIO/SPI probes functions automatically add a default wlan interface on top
of registered wiphy, through wilc_cfg80211_init which in turn calls
wilc_netdev_ifc_init. However, bus is still not fully initialized when we
register corresponding net device (for example we still miss some private
driver data pointers), which for example makes it impossible to
retrieve MAC address from chip (which is supposed to be set on net device
before its registration) before registering net device. More generally, net
device registration should not be done until driver has fully initialized
everything and is ready to handle any operation  on the net device.

Prevent net device from being registered so early by doing it at the end of
probe functions. Apply this logic to both sdio and spi buses.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/cfg80211.c | 10 ----------
 drivers/net/wireless/microchip/wilc1000/sdio.c     | 14 ++++++++++++--
 drivers/net/wireless/microchip/wilc1000/spi.c      | 11 +++++++++--
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index 7d9fb9f2d527..f716981f62ad 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -1773,7 +1773,6 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type,
 		       const struct wilc_hif_func *ops)
 {
 	struct wilc *wl;
-	struct wilc_vif *vif;
 	int ret, i;
 
 	wl = wilc_create_wiphy(dev);
@@ -1802,18 +1801,9 @@ int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type,
 		ret = -ENOMEM;
 		goto free_cfg;
 	}
-	vif = wilc_netdev_ifc_init(wl, "wlan%d", WILC_STATION_MODE,
-				   NL80211_IFTYPE_STATION, false);
-	if (IS_ERR(vif)) {
-		ret = PTR_ERR(vif);
-		goto free_hq;
-	}
 
 	return 0;
 
-free_hq:
-	destroy_workqueue(wl->hif_workqueue);
-
 free_cfg:
 	wilc_wlan_cfg_deinit(wl);
 
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 52a770c5e76f..a841dad08410 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -136,9 +136,11 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
 static int wilc_sdio_probe(struct sdio_func *func,
 			   const struct sdio_device_id *id)
 {
+	struct wilc_sdio *sdio_priv;
+	struct wilc_vif *vif;
 	struct wilc *wilc;
 	int ret;
-	struct wilc_sdio *sdio_priv;
+
 
 	sdio_priv = kzalloc(sizeof(*sdio_priv), GFP_KERNEL);
 	if (!sdio_priv)
@@ -176,9 +178,17 @@ static int wilc_sdio_probe(struct sdio_func *func,
 	}
 	clk_prepare_enable(wilc->rtc_clk);
 
+	vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
+				   NL80211_IFTYPE_STATION, false);
+	if (IS_ERR(vif)) {
+		ret = PTR_ERR(vif);
+		goto clk_disable_unprepare;
+	}
+
 	dev_info(&func->dev, "Driver Initializing success\n");
 	return 0;
-
+clk_disable_unprepare:
+	clk_disable_unprepare(wilc->rtc_clk);
 dispose_irq:
 	irq_dispose_mapping(wilc->dev_irq_num);
 	wilc_netdev_cleanup(wilc);
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 61c3572ce321..add0e70a09ea 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -206,9 +206,10 @@ static void wilc_wlan_power(struct wilc *wilc, bool on)
 
 static int wilc_bus_probe(struct spi_device *spi)
 {
-	int ret;
-	struct wilc *wilc;
 	struct wilc_spi *spi_priv;
+	struct wilc_vif *vif;
+	struct wilc *wilc;
+	int ret;
 
 	spi_priv = kzalloc(sizeof(*spi_priv), GFP_KERNEL);
 	if (!spi_priv)
@@ -250,6 +251,12 @@ static int wilc_bus_probe(struct spi_device *spi)
 		goto power_down;
 
 	wilc_wlan_power(wilc, false);
+	vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
+				   NL80211_IFTYPE_STATION, false);
+	if (IS_ERR(vif)) {
+		ret = PTR_ERR(vif);
+		goto power_down;
+	}
 	return 0;
 
 power_down:

-- 
2.44.0

[PATCH 3/6] wifi: wilc1000: set wilc_set_mac_address parameter as const

[Alexis Lothoré]

Any attempt to provide a const mac address to wilc_set_mac_address results
in the following warning:

warning: passing argument 2 of 'wilc_set_mac_address' discards 'const'
qualifier from pointer target type [-Wdiscarded-qualifiers]
[...]
drivers/net/wireless/microchip/wilc1000/hif.h:170:52: note: expected 'u8 *'
{aka 'unsigned char *'} but argument is of type 'const unsigned char *'a
int wilc_set_mac_address(struct wilc_vif *vif, u8 *mac_addr);

Instead of using an explicit cast each time we need provide a MAC address,
set the function parameter as const

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/hif.c    | 4 ++--
 drivers/net/wireless/microchip/wilc1000/hif.h    | 2 +-
 drivers/net/wireless/microchip/wilc1000/netdev.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 919de6ffb821..c6892b7e190f 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1293,7 +1293,7 @@ int wilc_get_mac_address(struct wilc_vif *vif, u8 *mac_addr)
 	return result;
 }
 
-int wilc_set_mac_address(struct wilc_vif *vif, u8 *mac_addr)
+int wilc_set_mac_address(struct wilc_vif *vif, const u8 *mac_addr)
 {
 	struct wid wid;
 	int result;
@@ -1301,7 +1301,7 @@ int wilc_set_mac_address(struct wilc_vif *vif, u8 *mac_addr)
 	wid.id = WID_MAC_ADDR;
 	wid.type = WID_STR;
 	wid.size = ETH_ALEN;
-	wid.val = mac_addr;
+	wid.val = (u8 *)mac_addr;
 
 	result = wilc_send_config_pkt(vif, WILC_SET_CFG, &wid, 1);
 	if (result)
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.h b/drivers/net/wireless/microchip/wilc1000/hif.h
index 0d380586b1d9..96eeaf31d237 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.h
+++ b/drivers/net/wireless/microchip/wilc1000/hif.h
@@ -167,7 +167,7 @@ int wilc_add_rx_gtk(struct wilc_vif *vif, const u8 *rx_gtk, u8 gtk_key_len,
 		    u8 cipher_mode);
 int wilc_set_pmkid_info(struct wilc_vif *vif, struct wilc_pmkid_attr *pmkid);
 int wilc_get_mac_address(struct wilc_vif *vif, u8 *mac_addr);
-int wilc_set_mac_address(struct wilc_vif *vif, u8 *mac_addr);
+int wilc_set_mac_address(struct wilc_vif *vif, const u8 *mac_addr);
 int wilc_set_join_req(struct wilc_vif *vif, u8 *bssid, const u8 *ies,
 		      size_t ies_len);
 int wilc_disconnect(struct wilc_vif *vif);
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index acc9b9a64552..5ab448d0b643 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -678,7 +678,7 @@ static int wilc_set_mac_addr(struct net_device *dev, void *p)
 	}
 	rcu_read_unlock();
 
-	result = wilc_set_mac_address(vif, (u8 *)addr->sa_data);
+	result = wilc_set_mac_address(vif, addr->sa_data);
 	if (result)
 		return result;
 

-- 
2.44.0

[PATCH 4/6] wifi: wilc1000: add function to read mac address from eFuse

[Alexis Lothoré]

wilc driver currently reads and sets mac address by firmware calls. It
means that we can not access mac address if no interface has been brought
up (so firmware is up and running). Another way to get mac address is to
read it directly from eFUSE.

Add a function helper to read the mac address written in eFuse, without
firmware assistance

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/fw.h     | 13 +++++++
 drivers/net/wireless/microchip/wilc1000/netdev.h |  2 +
 drivers/net/wireless/microchip/wilc1000/wlan.c   | 48 ++++++++++++++++++++++++
 drivers/net/wireless/microchip/wilc1000/wlan.h   |  1 +
 4 files changed, 64 insertions(+)

diff --git a/drivers/net/wireless/microchip/wilc1000/fw.h b/drivers/net/wireless/microchip/wilc1000/fw.h
index 5c5cac4aab02..7a930e89614c 100644
--- a/drivers/net/wireless/microchip/wilc1000/fw.h
+++ b/drivers/net/wireless/microchip/wilc1000/fw.h
@@ -13,6 +13,12 @@
 #define WILC_MAX_RATES_SUPPORTED		12
 #define WILC_MAX_NUM_PMKIDS			16
 #define WILC_MAX_NUM_SCANNED_CH			14
+#define WILC_NVMEM_MAX_NUM_BANK			6
+#define WILC_NVMEM_BANK_BASE			0x30000000
+#define WILC_NVMEM_LOW_BANK_OFFSET		0x102c
+#define WILC_NVMEM_HIGH_BANK_OFFSET		0x1380
+#define WILC_NVMEM_IS_BANK_USED			BIT(31)
+#define WILC_NVMEM_IS_BANK_INVALID		BIT(30)
 
 struct wilc_assoc_resp {
 	__le16 capab_info;
@@ -127,4 +133,11 @@ struct wilc_external_auth_param {
 	__le32 key_mgmt_suites;
 	__le16 status;
 } __packed;
+
+static inline u32 get_bank_offset_from_bank_index(unsigned int i)
+{
+	return (((i) < 2) ? WILC_NVMEM_LOW_BANK_OFFSET + ((i) * 32) :
+		WILC_NVMEM_HIGH_BANK_OFFSET + ((i) - 2) * 16);
+}
+
 #endif
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index eecee3973d6a..20ba030022a1 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -14,6 +14,7 @@
 #include <linux/if_arp.h>
 #include <linux/gpio/consumer.h>
 #include <linux/rculist.h>
+#include <uapi/linux/if_ether.h>
 
 #include "hif.h"
 #include "wlan.h"
@@ -278,6 +279,7 @@ struct wilc {
 	struct ieee80211_rate bitrates[ARRAY_SIZE(wilc_bitrates)];
 	struct ieee80211_supported_band band;
 	u32 cipher_suites[ARRAY_SIZE(wilc_cipher_suites)];
+	u8 nv_mac_address[ETH_ALEN];
 };
 
 struct wilc_wfi_mon_priv {
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 37c32d17856e..a7a213e161f3 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -26,6 +26,54 @@ static inline void release_bus(struct wilc *wilc, enum bus_release release)
 	mutex_unlock(&wilc->hif_cs);
 }
 
+int wilc_load_mac_from_nv(struct wilc *wl)
+{
+	int ret = -EINVAL;
+	unsigned int i;
+
+	acquire_bus(wl, WILC_BUS_ACQUIRE_AND_WAKEUP);
+
+	for (i = 0; i < WILC_NVMEM_MAX_NUM_BANK; i++) {
+		int bank_offset = get_bank_offset_from_bank_index(i);
+		u32 reg1, reg2;
+		u8 invalid;
+		u8 used;
+
+		ret = wl->hif_func->hif_read_reg(wl,
+						 WILC_NVMEM_BANK_BASE + bank_offset,
+						 &reg1);
+		if (ret) {
+			pr_err("Can not read address %d lower part", i);
+			break;
+		}
+		ret = wl->hif_func->hif_read_reg(wl,
+						 WILC_NVMEM_BANK_BASE + bank_offset + 4,
+						 &reg2);
+		if (ret) {
+			pr_err("Can not read address %d upper part", i);
+			break;
+		}
+
+		used = FIELD_GET(WILC_NVMEM_IS_BANK_USED, reg1);
+		invalid = FIELD_GET(WILC_NVMEM_IS_BANK_INVALID, reg1);
+		if (!used || invalid)
+			continue;
+
+		wl->nv_mac_address[0] = FIELD_GET(GENMASK(23, 16), reg1);
+		wl->nv_mac_address[1] = FIELD_GET(GENMASK(15, 8), reg1);
+		wl->nv_mac_address[2] = FIELD_GET(GENMASK(7, 0), reg1);
+		wl->nv_mac_address[3] = FIELD_GET(GENMASK(31, 24), reg2);
+		wl->nv_mac_address[4] = FIELD_GET(GENMASK(23, 16), reg2);
+		wl->nv_mac_address[5] = FIELD_GET(GENMASK(15, 8), reg2);
+
+		ret = 0;
+		break;
+	}
+
+	release_bus(wl, WILC_BUS_RELEASE_ALLOW_SLEEP);
+	return ret;
+}
+
 static void wilc_wlan_txq_remove(struct wilc *wilc, u8 q_num,
 				 struct txq_entry_t *tqe)
 {
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index 54643d8fef04..d72a0a81bbda 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -445,4 +445,5 @@ int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 			 u32 count);
 int wilc_wlan_init(struct net_device *dev);
 u32 wilc_get_chipid(struct wilc *wilc, bool update);
+int wilc_load_mac_from_nv(struct wilc *wilc);
 #endif

-- 
2.44.0

[PATCH 5/6] wifi: wilc1000: make sdio deinit function really deinit the sdio card

[Alexis Lothoré]

In order to be able to read raw registers (eg the nv mac address) in
wilc1000 during probe before the firmware is loaded and running, we need to
run the basic sdio functions initialization, but then we also need to
properly deinitialize those right after, to preserve the current driver
behavior (keeping the chip idle/unconfigured until the corresponding
interface is brought up). Calling wilc_sdio_deinit in its current form is
not enough because it merely resets an internal flag.

Implement a deinit sequence which symmetrically reset all steps performed
in wilc_sdio_init (only for parts activating/deactivating features, for the
sake of simplicity, let's ignore blocks size configuration reset)

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/sdio.c | 45 ++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index a841dad08410..04d6565df2cb 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -627,7 +627,52 @@ static int wilc_sdio_read(struct wilc *wilc, u32 addr, u8 *buf, u32 size)
 
 static int wilc_sdio_deinit(struct wilc *wilc)
 {
+	struct sdio_func *func = dev_to_sdio_func(wilc->dev);
 	struct wilc_sdio *sdio_priv = wilc->bus_data;
+	struct sdio_cmd52 cmd;
+	int ret;
+
+	cmd.read_write = 1;
+	cmd.function = 0;
+	cmd.raw = 1;
+
+	/* Disable all functions interrupts */
+	cmd.address = SDIO_CCCR_IENx;
+	cmd.data = 0;
+	ret = wilc_sdio_cmd52(wilc, &cmd);
+	if (ret) {
+		dev_err(&func->dev, "Failed to disable functions interrupts\n");
+		return ret;
+	}
+
+	/* Disable all functions */
+	cmd.address = SDIO_CCCR_IOEx;
+	cmd.data = 0;
+	ret = wilc_sdio_cmd52(wilc, &cmd);
+	if (ret) {
+		dev_err(&func->dev,
+			"Failed to reset all functions\n");
+		return ret;
+	}
+
+	/* Disable CSA */
+	cmd.read_write = 0;
+	cmd.address = SDIO_FBR_BASE(1);
+	ret = wilc_sdio_cmd52(wilc, &cmd);
+	if (ret) {
+		dev_err(&func->dev,
+			"Failed to read CSA for function 1\n");
+		return ret;
+	}
+	cmd.read_write = 1;
+	cmd.address = SDIO_FBR_BASE(1);
+	cmd.data &= ~SDIO_FBR_ENABLE_CSA;
+	ret = wilc_sdio_cmd52(wilc, &cmd);
+	if (ret) {
+		dev_err(&func->dev,
+			"Failed to disable CSA for function 1\n");
+		return ret;
+	}
 
 	sdio_priv->isinit = false;
 	return 0;

-- 
2.44.0

[PATCH 6/6] wifi: wilc1000: read MAC address from fuse at probe

[Alexis Lothoré]

From: Ajay Singh <ajay.kathat@microchip.com>

The default netdev interface exposed by WILC1000 is registered at probe,
but the chip mac address is not known until ndo_open, which will load and
start chip firmware and then retrieve stored MAC address from it. As a
consequence, the interface has uninitialized value (00:00:00:00:00) until a
user brings up the interface.

Fix MAC address at probe by setting the following steps:
- at probe, read MAC address directly from fuse
- whenever a new netdevice is created, apply saved mac address (which can
  be a user-provided address, or the eFuse Mac address if no address has
  been passed by user)
- whenever an interface is brought up for the first time (and so the
  firmware is loaded and started), enforce netdevice mac address to the
  chip (in case user has changed it)

Reported-by: Heiko Thiery <heiko.thiery@gmail.com>
Closes: https://lore.kernel.org/netdev/CAEyMn7aV-B4OEhHR4Ad0LM3sKCz1-nDqSb9uZNmRWR-hMZ=z+A@mail.gmail.com/T/
Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
Co-developed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/netdev.c | 38 ++++++++++++++----------
 drivers/net/wireless/microchip/wilc1000/sdio.c   | 14 +++++++++
 drivers/net/wireless/microchip/wilc1000/spi.c    |  6 ++++
 3 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 5ab448d0b643..f1fbc6e8a82a 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -588,7 +588,6 @@ static int wilc_mac_open(struct net_device *ndev)
 	struct wilc *wl = vif->wilc;
 	int ret = 0;
 	struct mgmt_frame_regs mgmt_regs = {};
-	u8 addr[ETH_ALEN] __aligned(2);
 
 	if (!wl || !wl->dev) {
 		netdev_err(ndev, "device not ready\n");
@@ -607,25 +606,19 @@ static int wilc_mac_open(struct net_device *ndev)
 		return ret;
 	}
 
-	wilc_set_operation_mode(vif, wilc_get_vif_idx(vif), vif->iftype,
-				vif->idx);
-
-	if (is_valid_ether_addr(ndev->dev_addr)) {
-		ether_addr_copy(addr, ndev->dev_addr);
-		wilc_set_mac_address(vif, addr);
-	} else {
-		wilc_get_mac_address(vif, addr);
-		eth_hw_addr_set(ndev, addr);
-	}
 	netdev_dbg(ndev, "Mac address: %pM\n", ndev->dev_addr);
-
-	if (!is_valid_ether_addr(ndev->dev_addr)) {
-		netdev_err(ndev, "Wrong MAC address\n");
+	ret = wilc_set_mac_address(vif, ndev->dev_addr);
+	if (ret) {
+		netdev_err(ndev, "Failed to enforce MAC address in chip");
 		wilc_deinit_host_int(ndev);
-		wilc_wlan_deinitialize(ndev);
-		return -EINVAL;
+		if (!wl->open_ifcs)
+			wilc_wlan_deinitialize(ndev);
+		return ret;
 	}
 
+	wilc_set_operation_mode(vif, wilc_get_vif_idx(vif), vif->iftype,
+				vif->idx);
+
 	mgmt_regs.interface_stypes = vif->mgmt_reg_stypes;
 	/* so we detect a change */
 	vif->mgmt_reg_stypes = 0;
@@ -941,6 +934,7 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
 				      int vif_type, enum nl80211_iftype type,
 				      bool rtnl_locked)
 {
+	u8 mac_address[ETH_ALEN];
 	struct net_device *ndev;
 	struct wilc_vif *vif;
 	int ret;
@@ -969,6 +963,18 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
 	vif->iftype = vif_type;
 	vif->idx = wilc_get_available_idx(wl);
 	vif->mac_opened = 0;
+
+	memcpy(mac_address, wl->nv_mac_address, ETH_ALEN);
+	/* WILC firmware uses locally administered MAC address for the
+	 * second virtual interface (bit 1 of first byte set), but
+	 * since it is possibly not loaded/running yet, reproduce this behavior
+	 * in the driver during interface creation.
+	 */
+	if (vif->idx)
+		mac_address[0] |= 0x2;
+
+	eth_hw_addr_set(vif->ndev, mac_address);
+
 	mutex_lock(&wl->vif_mutex);
 	list_add_tail_rcu(&vif->list, &wl->vif_list);
 	wl->vif_num += 1;
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 04d6565df2cb..e6e20c86b791 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -24,6 +24,9 @@ MODULE_DEVICE_TABLE(sdio, wilc_sdio_ids);
 
 #define WILC_SDIO_BLOCK_SIZE 512
 
+static int wilc_sdio_init(struct wilc *wilc, bool resume);
+static int wilc_sdio_deinit(struct wilc *wilc);
+
 struct wilc_sdio {
 	bool irq_gpio;
 	u32 block_size;
@@ -178,6 +181,16 @@ static int wilc_sdio_probe(struct sdio_func *func,
 	}
 	clk_prepare_enable(wilc->rtc_clk);
 
+	wilc_sdio_init(wilc, false);
+
+	ret = wilc_load_mac_from_nv(wilc);
+	if (ret) {
+		pr_err("Can not retrieve MAC address from chip\n");
+		goto clk_disable_unprepare;
+	}
+
+	wilc_sdio_deinit(wilc);
+
 	vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
 				   NL80211_IFTYPE_STATION, false);
 	if (IS_ERR(vif)) {
@@ -187,6 +200,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
 
 	dev_info(&func->dev, "Driver Initializing success\n");
 	return 0;
+
 clk_disable_unprepare:
 	clk_disable_unprepare(wilc->rtc_clk);
 dispose_irq:
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index add0e70a09ea..5ff940c53ad9 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -250,6 +250,12 @@ static int wilc_bus_probe(struct spi_device *spi)
 	if (ret)
 		goto power_down;
 
+	ret = wilc_load_mac_from_nv(wilc);
+	if (ret) {
+		pr_err("Can not retrieve MAC address from chip\n");
+		goto power_down;
+	}
+
 	wilc_wlan_power(wilc, false);
 	vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
 				   NL80211_IFTYPE_STATION, false);

-- 
2.44.0

Re: [PATCH 6/6] wifi: wilc1000: read MAC address from fuse at probe

[Heiko Thiery]

Hi Alexis, All

Am Mi., 17. Apr. 2024 um 11:34 Uhr schrieb Alexis Lothoré
<alexis.lothore@bootlin.com>:
>
> From: Ajay Singh <ajay.kathat@microchip.com>
>
> The default netdev interface exposed by WILC1000 is registered at probe,
> but the chip mac address is not known until ndo_open, which will load and
> start chip firmware and then retrieve stored MAC address from it. As a
> consequence, the interface has uninitialized value (00:00:00:00:00) until a
> user brings up the interface.
>
> Fix MAC address at probe by setting the following steps:
> - at probe, read MAC address directly from fuse
> - whenever a new netdevice is created, apply saved mac address (which can
>   be a user-provided address, or the eFuse Mac address if no address has
>   been passed by user)
> - whenever an interface is brought up for the first time (and so the
>   firmware is loaded and started), enforce netdevice mac address to the
>   chip (in case user has changed it)
>
> Reported-by: Heiko Thiery <heiko.thiery@gmail.com>
> Closes: https://lore.kernel.org/netdev/CAEyMn7aV-B4OEhHR4Ad0LM3sKCz1-nDqSb9uZNmRWR-hMZ=z+A@mail.gmail.com/T/
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> Co-developed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

I applied the series on next and tested it on my env. It looks good.

Tested-by: Heiko Thiery <heiko.thiery@gmail.com>

Thank you!
Heiko

> ---
>  drivers/net/wireless/microchip/wilc1000/netdev.c | 38 ++++++++++++++----------
>  drivers/net/wireless/microchip/wilc1000/sdio.c   | 14 +++++++++
>  drivers/net/wireless/microchip/wilc1000/spi.c    |  6 ++++
>  3 files changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
> index 5ab448d0b643..f1fbc6e8a82a 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.c
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
> @@ -588,7 +588,6 @@ static int wilc_mac_open(struct net_device *ndev)
>         struct wilc *wl = vif->wilc;
>         int ret = 0;
>         struct mgmt_frame_regs mgmt_regs = {};
> -       u8 addr[ETH_ALEN] __aligned(2);
>
>         if (!wl || !wl->dev) {
>                 netdev_err(ndev, "device not ready\n");
> @@ -607,25 +606,19 @@ static int wilc_mac_open(struct net_device *ndev)
>                 return ret;
>         }
>
> -       wilc_set_operation_mode(vif, wilc_get_vif_idx(vif), vif->iftype,
> -                               vif->idx);
> -
> -       if (is_valid_ether_addr(ndev->dev_addr)) {
> -               ether_addr_copy(addr, ndev->dev_addr);
> -               wilc_set_mac_address(vif, addr);
> -       } else {
> -               wilc_get_mac_address(vif, addr);
> -               eth_hw_addr_set(ndev, addr);
> -       }
>         netdev_dbg(ndev, "Mac address: %pM\n", ndev->dev_addr);
> -
> -       if (!is_valid_ether_addr(ndev->dev_addr)) {
> -               netdev_err(ndev, "Wrong MAC address\n");
> +       ret = wilc_set_mac_address(vif, ndev->dev_addr);
> +       if (ret) {
> +               netdev_err(ndev, "Failed to enforce MAC address in chip");
>                 wilc_deinit_host_int(ndev);
> -               wilc_wlan_deinitialize(ndev);
> -               return -EINVAL;
> +               if (!wl->open_ifcs)
> +                       wilc_wlan_deinitialize(ndev);
> +               return ret;
>         }
>
> +       wilc_set_operation_mode(vif, wilc_get_vif_idx(vif), vif->iftype,
> +                               vif->idx);
> +
>         mgmt_regs.interface_stypes = vif->mgmt_reg_stypes;
>         /* so we detect a change */
>         vif->mgmt_reg_stypes = 0;
> @@ -941,6 +934,7 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
>                                       int vif_type, enum nl80211_iftype type,
>                                       bool rtnl_locked)
>  {
> +       u8 mac_address[ETH_ALEN];
>         struct net_device *ndev;
>         struct wilc_vif *vif;
>         int ret;
> @@ -969,6 +963,18 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
>         vif->iftype = vif_type;
>         vif->idx = wilc_get_available_idx(wl);
>         vif->mac_opened = 0;
> +
> +       memcpy(mac_address, wl->nv_mac_address, ETH_ALEN);
> +       /* WILC firmware uses locally administered MAC address for the
> +        * second virtual interface (bit 1 of first byte set), but
> +        * since it is possibly not loaded/running yet, reproduce this behavior
> +        * in the driver during interface creation.
> +        */
> +       if (vif->idx)
> +               mac_address[0] |= 0x2;
> +
> +       eth_hw_addr_set(vif->ndev, mac_address);
> +
>         mutex_lock(&wl->vif_mutex);
>         list_add_tail_rcu(&vif->list, &wl->vif_list);
>         wl->vif_num += 1;
> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
> index 04d6565df2cb..e6e20c86b791 100644
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -24,6 +24,9 @@ MODULE_DEVICE_TABLE(sdio, wilc_sdio_ids);
>
>  #define WILC_SDIO_BLOCK_SIZE 512
>
> +static int wilc_sdio_init(struct wilc *wilc, bool resume);
> +static int wilc_sdio_deinit(struct wilc *wilc);
> +
>  struct wilc_sdio {
>         bool irq_gpio;
>         u32 block_size;
> @@ -178,6 +181,16 @@ static int wilc_sdio_probe(struct sdio_func *func,
>         }
>         clk_prepare_enable(wilc->rtc_clk);
>
> +       wilc_sdio_init(wilc, false);
> +
> +       ret = wilc_load_mac_from_nv(wilc);
> +       if (ret) {
> +               pr_err("Can not retrieve MAC address from chip\n");
> +               goto clk_disable_unprepare;
> +       }
> +
> +       wilc_sdio_deinit(wilc);
> +
>         vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
>                                    NL80211_IFTYPE_STATION, false);
>         if (IS_ERR(vif)) {
> @@ -187,6 +200,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
>
>         dev_info(&func->dev, "Driver Initializing success\n");
>         return 0;
> +
>  clk_disable_unprepare:
>         clk_disable_unprepare(wilc->rtc_clk);
>  dispose_irq:
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index add0e70a09ea..5ff940c53ad9 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -250,6 +250,12 @@ static int wilc_bus_probe(struct spi_device *spi)
>         if (ret)
>                 goto power_down;
>
> +       ret = wilc_load_mac_from_nv(wilc);
> +       if (ret) {
> +               pr_err("Can not retrieve MAC address from chip\n");
> +               goto power_down;
> +       }
> +
>         wilc_wlan_power(wilc, false);
>         vif = wilc_netdev_ifc_init(wilc, "wlan%d", WILC_STATION_MODE,
>                                    NL80211_IFTYPE_STATION, false);
>
> --
> 2.44.0
>

Re: [PATCH 1/6] wifi: wilc1000: set net device registration as last step during interface creation

[Kalle Valo]

Alexis Lothoré <alexis.lothore@bootlin.com> wrote:

> net device registration is currently done in wilc_netdev_ifc_init but
> other initialization operations are still done after this registration.
> Since net device is assumed to be usable right after registration, it
> should be the very last step of initialization.
> 
> Move netdev registration at the very end of wilc_netdev_ifc_init to let
> this function completely initialize netdevice before registering it.
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

I see errors:

ERROR: modpost: "wilc_load_mac_from_nv" [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
ERROR: modpost: "wilc_netdev_ifc_init" [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
ERROR: modpost: "wilc_load_mac_from_nv" [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
ERROR: modpost: "wilc_netdev_ifc_init" [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
make[1]: *** [/home/kvalo/projects/personal/wireless-drivers/src/wireless-next/Makefile:1871: modpost] Error 2
make: *** [Makefile:240: __sub-make] Error 2

6 patches set to Changes Requested.

13633102 [1/6] wifi: wilc1000: set net device registration as last step during interface creation
13633103 [2/6] wifi: wilc1000: register net device only after bus being fully initialized
13633104 [3/6] wifi: wilc1000: set wilc_set_mac_address parameter as const
13633105 [4/6] wifi: wilc1000: add function to read mac address from eFuse
13633106 [5/6] wifi: wilc1000: make sdio deinit function really deinit the sdio card
13633107 [6/6] wifi: wilc1000: read MAC address from fuse at probe

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240417-mac_addr_at_probe-v1-1-67d6c9b3bc2b@bootlin.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/6] wifi: wilc1000: set net device registration as last step during interface creation

[Alexis Lothoré]

Hello Kalle,

On 5/14/24 14:45, Kalle Valo wrote:
> Alexis Lothoré <alexis.lothore@bootlin.com> wrote:
> 
>> net device registration is currently done in wilc_netdev_ifc_init but
>> other initialization operations are still done after this registration.
>> Since net device is assumed to be usable right after registration, it
>> should be the very last step of initialization.
>>
>> Move netdev registration at the very end of wilc_netdev_ifc_init to let
>> this function completely initialize netdevice before registering it.
>>
>> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> I see errors:
> 
> ERROR: modpost: "wilc_load_mac_from_nv" [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
> ERROR: modpost: "wilc_netdev_ifc_init" [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
> ERROR: modpost: "wilc_load_mac_from_nv" [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
> ERROR: modpost: "wilc_netdev_ifc_init" [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
> make[1]: *** [/home/kvalo/projects/personal/wireless-drivers/src/wireless-next/Makefile:1871: modpost] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
> 
> 6 patches set to Changes Requested.
> 
> 13633102 [1/6] wifi: wilc1000: set net device registration as last step during interface creation
> 13633103 [2/6] wifi: wilc1000: register net device only after bus being fully initialized
> 13633104 [3/6] wifi: wilc1000: set wilc_set_mac_address parameter as const
> 13633105 [4/6] wifi: wilc1000: add function to read mac address from eFuse
> 13633106 [5/6] wifi: wilc1000: make sdio deinit function really deinit the sdio card
> 13633107 [6/6] wifi: wilc1000: read MAC address from fuse at probe

Shame on me, I missed those basic errors since I worked with drivers as built-in
instead of modules. I'll update my workflow and send a v2.

Thanks,

Alexis
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/6] wifi: wilc1000: set net device registration as last step during interface creation

[Kalle Valo]

Alexis Lothoré <alexis.lothore@bootlin.com> writes:

> Hello Kalle,
>
> On 5/14/24 14:45, Kalle Valo wrote:
>> Alexis Lothoré <alexis.lothore@bootlin.com> wrote:
>> 
>>> net device registration is currently done in wilc_netdev_ifc_init but
>>> other initialization operations are still done after this registration.
>>> Since net device is assumed to be usable right after registration, it
>>> should be the very last step of initialization.
>>>
>>> Move netdev registration at the very end of wilc_netdev_ifc_init to let
>>> this function completely initialize netdevice before registering it.
>>>
>>> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
>> 
>> I see errors:
>> 
>> ERROR: modpost: "wilc_load_mac_from_nv"
>> [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko]
>> undefined!
>> ERROR: modpost: "wilc_netdev_ifc_init"
>> [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko]
>> undefined!
>> ERROR: modpost: "wilc_load_mac_from_nv"
>> [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
>> ERROR: modpost: "wilc_netdev_ifc_init"
>> [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
>> make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
>> make[1]: ***
>> [/home/kvalo/projects/personal/wireless-drivers/src/wireless-next/Makefile:1871:
>> modpost] Error 2
>> make: *** [Makefile:240: __sub-make] Error 2
>> 
>> 6 patches set to Changes Requested.
>> 
>> 13633102 [1/6] wifi: wilc1000: set net device registration as last
>> step during interface creation
>> 13633103 [2/6] wifi: wilc1000: register net device only after bus
>> being fully initialized
>> 13633104 [3/6] wifi: wilc1000: set wilc_set_mac_address parameter as const
>> 13633105 [4/6] wifi: wilc1000: add function to read mac address from eFuse
>> 13633106 [5/6] wifi: wilc1000: make sdio deinit function really deinit the sdio card
>> 13633107 [6/6] wifi: wilc1000: read MAC address from fuse at probe
>
> Shame on me, I missed those basic errors since I worked with drivers as built-in
> instead of modules. I'll update my workflow and send a v2.

No worries, but I'm surprised that Intel's kernel test robot didn't
report anything.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches