[PATCH 0/4] wifi: wilc1000: fix RCU usage

[PATCH 0/4] wifi: wilc1000: fix RCU usage

[Alexis Lothoré]

This small series aims to fix multiple warnings observed when enabling
CONFIG_PROVE_RCU_LIST:
- add missing locks to create corresponding critical read sections
- fix mix between RCU and SRCU API usage

While at it, since SRCU API is already in use in the driver, any fix done
on RCU usage was also done with the SRCU variant of RCU API. I do not
really get why we are using SRCU in this driver instead of classic RCU, as
it seems to be done in any other wireless driver. My understanding is that
primary SRCU use case is for compatibility with realtime kernel, which
needs to be preemptible everywhere. Has the driver been really developped
with this constraint in mind ?
If you have more details about this, feel free to educate me.

To: <linux-wireless@vger.kernel.org>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: <linux-kernel@vger.kernel.org>

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Ajay Singh (1):
      wifi: wilc1000: add missing read critical sections around vif list traversal

Alexis Lothoré (3):
      wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper
      wifi: wilc1000: use SRCU instead of RCU for vif list traversal
      wifi: wilc1000: fix declarations ordering

 drivers/net/wireless/microchip/wilc1000/cfg80211.c |  2 +-
 drivers/net/wireless/microchip/wilc1000/hif.c      | 70 ++++++++++++----------
 drivers/net/wireless/microchip/wilc1000/netdev.c   | 51 +++++++++-------
 drivers/net/wireless/microchip/wilc1000/netdev.h   |  6 ++
 drivers/net/wireless/microchip/wilc1000/wlan.c     |  2 +-
 5 files changed, 75 insertions(+), 56 deletions(-)
---
base-commit: f4adde5c2f875c491670bc19f6abae91ae364ed6
change-id: 20240131-wilc_fix_rcu_usage-e60ecdffee25

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

[PATCH 1/4] wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper

[Alexis Lothoré]

Move netif_wake_queue and its surrounding RCU operations in a dedicated
function to clarify wilc_txq_task and ease refactoring

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

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 22f461f477f1..62414ab8846e 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -140,6 +140,19 @@ int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
 	return ret_val;
 }
 
+static void wilc_wake_tx_queues(struct wilc *wl)
+{
+	int srcu_idx;
+	struct wilc_vif *ifc;
+
+	srcu_idx = srcu_read_lock(&wl->srcu);
+	list_for_each_entry_rcu(ifc, &wl->vif_list, list) {
+		if (ifc->mac_opened && netif_queue_stopped(ifc->ndev))
+			netif_wake_queue(ifc->ndev);
+	}
+	srcu_read_unlock(&wl->srcu, srcu_idx);
+}
+
 static int wilc_txq_task(void *vp)
 {
 	int ret;
@@ -160,17 +173,7 @@ static int wilc_txq_task(void *vp)
 		do {
 			ret = wilc_wlan_handle_txq(wl, &txq_count);
 			if (txq_count < FLOW_CONTROL_LOWER_THRESHOLD) {
-				int srcu_idx;
-				struct wilc_vif *ifc;
-
-				srcu_idx = srcu_read_lock(&wl->srcu);
-				list_for_each_entry_rcu(ifc, &wl->vif_list,
-							list) {
-					if (ifc->mac_opened &&
-					    netif_queue_stopped(ifc->ndev))
-						netif_wake_queue(ifc->ndev);
-				}
-				srcu_read_unlock(&wl->srcu, srcu_idx);
+				wilc_wake_tx_queues(wl);
 			}
 			if (ret != WILC_VMM_ENTRY_FULL_RETRY)
 				break;

-- 
2.43.0

[PATCH 2/4] wifi: wilc1000: use SRCU instead of RCU for vif list traversal

[Alexis Lothoré]

Enabling CONFIG_PROVE_RCU_LIST raises many warnings in wilc driver, even on
some places already protected by a read critical section. An example of
such case is in wilc_get_available_idx:

=============================
WARNING: suspicious RCU usage
6.8.0-rc1+ #32 Not tainted
-----------------------------
drivers/net/wireless/microchip/wilc1000/netdev.c:944 RCU-list traversed in non-reader section!!
[...]
stack backtrace:
CPU: 0 PID: 26 Comm: kworker/0:3 Not tainted 6.8.0-rc1+ #32
Hardware name: Atmel SAMA5
Workqueue: events_freezable mmc_rescan
 unwind_backtrace from show_stack+0x18/0x1c
 show_stack from dump_stack_lvl+0x34/0x58
 dump_stack_lvl from wilc_netdev_ifc_init+0x788/0x8ec
 wilc_netdev_ifc_init from wilc_cfg80211_init+0x690/0x910
 wilc_cfg80211_init from wilc_sdio_probe+0x168/0x490
 wilc_sdio_probe from sdio_bus_probe+0x230/0x3f4
 sdio_bus_probe from really_probe+0x270/0xdf4
 really_probe from __driver_probe_device+0x1dc/0x580
 __driver_probe_device from driver_probe_device+0x60/0x140
 driver_probe_device from __device_attach_driver+0x268/0x364
 __device_attach_driver from bus_for_each_drv+0x15c/0x1cc
 bus_for_each_drv from __device_attach+0x1ec/0x3e8
 __device_attach from bus_probe_device+0x190/0x1c0
 bus_probe_device from device_add+0x10dc/0x18e4
 device_add from sdio_add_func+0x1c0/0x2c0
 sdio_add_func from mmc_attach_sdio+0xa08/0xe1c
 mmc_attach_sdio from mmc_rescan+0xa00/0xfe0
 mmc_rescan from process_one_work+0x8d4/0x169c
 process_one_work from worker_thread+0x8cc/0x1340
 worker_thread from kthread+0x448/0x510
 kthread from ret_from_fork+0x14/0x28

This warning is due to the section being protected by a srcu critical read
section, but the list traversal being done with classic RCU API. Fix the
warning by using corresponding SRCU read lock/unlock APIs. While doing so,
since we always manipulate the same list (managed through a pointer
embedded in struct_wilc), add a macro to reduce the corresponding
boilerplate in each call site.

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

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index f03fd15c0c97..33f8e3a41937 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -1518,7 +1518,7 @@ static struct wilc_vif *wilc_get_vif_from_type(struct wilc *wl, int type)
 {
 	struct wilc_vif *vif;
 
-	list_for_each_entry_rcu(vif, &wl->vif_list, list) {
+	wilc_for_each_vif(wl, vif) {
 		if (vif->iftype == type)
 			return vif;
 	}
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index d2b8c2630819..f3800aa9e9f8 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -107,7 +107,7 @@ static struct wilc_vif *wilc_get_vif_from_idx(struct wilc *wilc, int idx)
 	if (index < 0 || index >= WILC_NUM_CONCURRENT_IFC)
 		return NULL;
 
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+	wilc_for_each_vif(wilc, vif) {
 		if (vif->idx == index)
 			return vif;
 	}
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 62414ab8846e..96f239adc078 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -96,7 +96,7 @@ static struct net_device *get_if_handler(struct wilc *wilc, u8 *mac_header)
 	struct wilc_vif *vif;
 	struct ieee80211_hdr *h = (struct ieee80211_hdr *)mac_header;
 
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+	wilc_for_each_vif(wilc, vif) {
 		if (vif->iftype == WILC_STATION_MODE)
 			if (ether_addr_equal_unaligned(h->addr2, vif->bssid)) {
 				ndev = vif->ndev;
@@ -132,7 +132,7 @@ int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
 	struct wilc_vif *vif;
 
 	srcu_idx = srcu_read_lock(&wilc->srcu);
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+	wilc_for_each_vif(wilc, vif) {
 		if (!is_zero_ether_addr(vif->bssid))
 			ret_val++;
 	}
@@ -146,7 +146,7 @@ static void wilc_wake_tx_queues(struct wilc *wl)
 	struct wilc_vif *ifc;
 
 	srcu_idx = srcu_read_lock(&wl->srcu);
-	list_for_each_entry_rcu(ifc, &wl->vif_list, list) {
+	wilc_for_each_vif(wl, ifc) {
 		if (ifc->mac_opened && netif_queue_stopped(ifc->ndev))
 			netif_wake_queue(ifc->ndev);
 	}
@@ -668,7 +668,7 @@ static int wilc_set_mac_addr(struct net_device *dev, void *p)
 	/* Verify MAC Address is not already in use: */
 
 	srcu_idx = srcu_read_lock(&wilc->srcu);
-	list_for_each_entry_rcu(tmp_vif, &wilc->vif_list, list) {
+	wilc_for_each_vif(wilc, tmp_vif) {
 		wilc_get_mac_address(tmp_vif, mac_addr);
 		if (ether_addr_equal(addr->sa_data, mac_addr)) {
 			if (vif != tmp_vif) {
@@ -771,7 +771,7 @@ netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *ndev)
 		struct wilc_vif *vif;
 
 		srcu_idx = srcu_read_lock(&wilc->srcu);
-		list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+		wilc_for_each_vif(wilc, vif) {
 			if (vif->mac_opened)
 				netif_stop_queue(vif->ndev);
 		}
@@ -858,7 +858,7 @@ void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)
 	struct wilc_vif *vif;
 
 	srcu_idx = srcu_read_lock(&wilc->srcu);
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+	wilc_for_each_vif(wilc, vif) {
 		struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)buff;
 		u16 type = le16_to_cpup((__le16 *)buff);
 		u32 type_bit = BIT(type >> 4);
@@ -930,7 +930,7 @@ static u8 wilc_get_available_idx(struct wilc *wl)
 	int srcu_idx;
 
 	srcu_idx = srcu_read_lock(&wl->srcu);
-	list_for_each_entry_rcu(vif, &wl->vif_list, list) {
+	wilc_for_each_vif(wl, vif) {
 		if (vif->idx == 0)
 			idx = 1;
 		else
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index aafe3dc44ac6..5937d6d45695 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -13,6 +13,7 @@
 #include <net/ieee80211_radiotap.h>
 #include <linux/if_arp.h>
 #include <linux/gpio/consumer.h>
+#include <linux/rculist.h>
 
 #include "hif.h"
 #include "wlan.h"
@@ -29,6 +30,11 @@
 
 #define TX_BACKOFF_WEIGHT_MS			1
 
+#define wilc_for_each_vif(w, v) \
+	struct wilc *_w = w; \
+	list_for_each_entry_srcu(v, &_w->vif_list, list, \
+				 srcu_read_lock_held(&_w->srcu))
+
 struct wilc_wfi_stats {
 	unsigned long rx_packets;
 	unsigned long tx_packets;
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 68be233c36ce..a9e872a7b2c3 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -725,7 +725,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
 	mutex_lock(&wilc->txq_add_to_head_cs);
 
 	srcu_idx = srcu_read_lock(&wilc->srcu);
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list)
+	wilc_for_each_vif(wilc, vif)
 		wilc_wlan_txq_filter_dup_tcp_ack(vif->ndev);
 	srcu_read_unlock(&wilc->srcu, srcu_idx);
 

-- 
2.43.0

[PATCH 3/4] wifi: wilc1000: fix declarations ordering

[Alexis Lothoré]

Fix reverse-christmas tree order in some functions before adding more
variables

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

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index f3800aa9e9f8..c42859a727c3 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1567,11 +1567,11 @@ int wilc_deinit(struct wilc_vif *vif)
 
 void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 {
-	int result;
-	struct host_if_msg *msg;
-	int id;
 	struct host_if_drv *hif_drv;
+	struct host_if_msg *msg;
 	struct wilc_vif *vif;
+	int result;
+	int id;
 
 	id = get_unaligned_le32(&buffer[length - 4]);
 	vif = wilc_get_vif_from_idx(wilc, id);
@@ -1608,11 +1608,11 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 
 void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 {
-	int result;
-	struct host_if_msg *msg;
-	int id;
 	struct host_if_drv *hif_drv;
+	struct host_if_msg *msg;
 	struct wilc_vif *vif;
+	int result;
+	int id;
 
 	mutex_lock(&wilc->deinit_lock);
 
@@ -1654,10 +1654,10 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 
 void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
 {
-	int result;
-	int id;
 	struct host_if_drv *hif_drv;
 	struct wilc_vif *vif;
+	int result;
+	int id;
 
 	id = get_unaligned_le32(&buffer[length - 4]);
 	vif = wilc_get_vif_from_idx(wilc, id);
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 96f239adc078..092801d33915 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -814,12 +814,12 @@ static int wilc_mac_close(struct net_device *ndev)
 void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
 		       u32 pkt_offset)
 {
-	unsigned int frame_len = 0;
-	int stats;
 	unsigned char *buff_to_send = NULL;
-	struct sk_buff *skb;
 	struct net_device *wilc_netdev;
+	unsigned int frame_len = 0;
 	struct wilc_vif *vif;
+	struct sk_buff *skb;
+	int stats;
 
 	if (!wilc)
 		return;

-- 
2.43.0

[PATCH 4/4] wifi: wilc1000: add missing read critical sections around vif list traversal

[Alexis Lothoré]

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

Some code manipulating the vif list is still missing some srcu_read_lock /
srcu_read_unlock, and so can trigger RCU warnings:

=============================
WARNING: suspicious RCU usage
6.8.0-rc1+ #37 Not tainted
-----------------------------
drivers/net/wireless/microchip/wilc1000/hif.c:110 RCU-list traversed without holding the required lock!!
[...]
stack backtrace:
CPU: 0 PID: 6 Comm: kworker/0:0 Not tainted 6.8.0-rc1+ #37
Hardware name: Atmel SAMA5
Workqueue: events sdio_irq_work
 unwind_backtrace from show_stack+0x18/0x1c
 show_stack from dump_stack_lvl+0x34/0x58
 dump_stack_lvl from wilc_get_vif_from_idx+0x158/0x180
 wilc_get_vif_from_idx from wilc_network_info_received+0x80/0x48c
 wilc_network_info_received from wilc_handle_isr+0xa10/0xd30
 wilc_handle_isr from wilc_sdio_interrupt+0x44/0x58
 wilc_sdio_interrupt from process_sdio_pending_irqs+0x1c8/0x60c
 process_sdio_pending_irqs from sdio_irq_work+0x6c/0x14c
 sdio_irq_work from process_one_work+0x8d4/0x169c
 process_one_work from worker_thread+0x8cc/0x1340
 worker_thread from kthread+0x448/0x510
 kthread from ret_from_fork+0x14/0x28

Fix those warnings by adding the needed lock around the corresponding
critical sections

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/hif.c    | 52 +++++++++++++-----------
 drivers/net/wireless/microchip/wilc1000/netdev.c |  8 +++-
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index c42859a727c3..f1085ccb7eed 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1570,23 +1570,25 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 	struct host_if_drv *hif_drv;
 	struct host_if_msg *msg;
 	struct wilc_vif *vif;
+	int srcu_idx;
 	int result;
 	int id;
 
 	id = get_unaligned_le32(&buffer[length - 4]);
+	srcu_idx = srcu_read_lock(&wilc->srcu);
 	vif = wilc_get_vif_from_idx(wilc, id);
 	if (!vif)
-		return;
-	hif_drv = vif->hif_drv;
+		goto out;
 
+	hif_drv = vif->hif_drv;
 	if (!hif_drv) {
 		netdev_err(vif->ndev, "driver not init[%p]\n", hif_drv);
-		return;
+		goto out;
 	}
 
 	msg = wilc_alloc_work(vif, handle_rcvd_ntwrk_info, false);
 	if (IS_ERR(msg))
-		return;
+		goto out;
 
 	msg->body.net_info.frame_len = get_unaligned_le16(&buffer[6]) - 1;
 	msg->body.net_info.rssi = buffer[8];
@@ -1595,7 +1597,7 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 					  GFP_KERNEL);
 	if (!msg->body.net_info.mgmt) {
 		kfree(msg);
-		return;
+		goto out;
 	}
 
 	result = wilc_enqueue_work(msg);
@@ -1604,6 +1606,8 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 		kfree(msg->body.net_info.mgmt);
 		kfree(msg);
 	}
+out:
+	srcu_read_unlock(&wilc->srcu, srcu_idx);
 }
 
 void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
@@ -1611,36 +1615,32 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 	struct host_if_drv *hif_drv;
 	struct host_if_msg *msg;
 	struct wilc_vif *vif;
+	int srcu_idx;
 	int result;
 	int id;
 
 	mutex_lock(&wilc->deinit_lock);
 
 	id = get_unaligned_le32(&buffer[length - 4]);
+	srcu_idx = srcu_read_lock(&wilc->srcu);
 	vif = wilc_get_vif_from_idx(wilc, id);
-	if (!vif) {
-		mutex_unlock(&wilc->deinit_lock);
-		return;
-	}
+	if (!vif)
+		goto out;
 
 	hif_drv = vif->hif_drv;
 
 	if (!hif_drv) {
-		mutex_unlock(&wilc->deinit_lock);
-		return;
+		goto out;
 	}
 
 	if (!hif_drv->conn_info.conn_result) {
 		netdev_err(vif->ndev, "%s: conn_result is NULL\n", __func__);
-		mutex_unlock(&wilc->deinit_lock);
-		return;
+		goto out;
 	}
 
 	msg = wilc_alloc_work(vif, handle_rcvd_gnrl_async_info, false);
-	if (IS_ERR(msg)) {
-		mutex_unlock(&wilc->deinit_lock);
-		return;
-	}
+	if (IS_ERR(msg))
+		goto out;
 
 	msg->body.mac_info.status = buffer[7];
 	result = wilc_enqueue_work(msg);
@@ -1648,7 +1648,8 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
 		netdev_err(vif->ndev, "%s: enqueue work failed\n", __func__);
 		kfree(msg);
 	}
-
+out:
+	srcu_read_unlock(&wilc->srcu, srcu_idx);
 	mutex_unlock(&wilc->deinit_lock);
 }
 
@@ -1656,24 +1657,27 @@ void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
 {
 	struct host_if_drv *hif_drv;
 	struct wilc_vif *vif;
+	int srcu_idx;
 	int result;
 	int id;
 
 	id = get_unaligned_le32(&buffer[length - 4]);
+	srcu_idx = srcu_read_lock(&wilc->srcu);
 	vif = wilc_get_vif_from_idx(wilc, id);
 	if (!vif)
-		return;
-	hif_drv = vif->hif_drv;
+		goto out;
 
-	if (!hif_drv)
-		return;
+	hif_drv = vif->hif_drv;
+	if (!hif_drv) {
+		goto out;
+	}
 
 	if (hif_drv->usr_scan_req.scan_result) {
 		struct host_if_msg *msg;
 
 		msg = wilc_alloc_work(vif, handle_scan_complete, false);
 		if (IS_ERR(msg))
-			return;
+			goto out;
 
 		result = wilc_enqueue_work(msg);
 		if (result) {
@@ -1682,6 +1686,8 @@ void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
 			kfree(msg);
 		}
 	}
+out:
+	srcu_read_unlock(&wilc->srcu, srcu_idx);
 }
 
 int wilc_remain_on_channel(struct wilc_vif *vif, u64 cookie, u16 chan,
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 092801d33915..710e29bea560 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -819,14 +819,16 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
 	unsigned int frame_len = 0;
 	struct wilc_vif *vif;
 	struct sk_buff *skb;
+	int srcu_idx;
 	int stats;
 
 	if (!wilc)
 		return;
 
+	srcu_idx = srcu_read_lock(&wilc->srcu);
 	wilc_netdev = get_if_handler(wilc, buff);
 	if (!wilc_netdev)
-		return;
+		goto out;
 
 	buff += pkt_offset;
 	vif = netdev_priv(wilc_netdev);
@@ -837,7 +839,7 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
 
 		skb = dev_alloc_skb(frame_len);
 		if (!skb)
-			return;
+			goto out;
 
 		skb->dev = wilc_netdev;
 
@@ -850,6 +852,8 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
 		stats = netif_rx(skb);
 		netdev_dbg(wilc_netdev, "netif_rx ret value is: %d\n", stats);
 	}
+out:
+	srcu_read_unlock(&wilc->srcu, srcu_idx);
 }
 
 void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)

-- 
2.43.0

Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage

[Kalle Valo]

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

> This small series aims to fix multiple warnings observed when enabling
> CONFIG_PROVE_RCU_LIST:
> - add missing locks to create corresponding critical read sections
> - fix mix between RCU and SRCU API usage
>
> While at it, since SRCU API is already in use in the driver, any fix done
> on RCU usage was also done with the SRCU variant of RCU API. I do not
> really get why we are using SRCU in this driver instead of classic RCU, as
> it seems to be done in any other wireless driver.

And even more so, no other driver in drivers/net use SRCU.

> My understanding is that primary SRCU use case is for compatibility
> with realtime kernel, which needs to be preemptible everywhere. Has
> the driver been really developped with this constraint in mind ? If
> you have more details about this, feel free to educate me.

Alexis, if you have the time I recommend submitting a patchset
converting wilc1000 to use classic RCU. At least I have a hard time
understanding why SRCU is needed, especially after seeing the warning
you found.

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

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

Re: [PATCH 1/4] wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper

[Kalle Valo]

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

> Move netif_wake_queue and its surrounding RCU operations in a dedicated
> function to clarify wilc_txq_task and ease refactoring
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

4 patches applied to wireless-next.git, thanks.

5d2dbccc2b3c wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper
059d0e3876ab wifi: wilc1000: use SRCU instead of RCU for vif list traversal
51e4aa8c449b wifi: wilc1000: fix declarations ordering
dd66185c23f7 wifi: wilc1000: add missing read critical sections around vif list traversal

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240215-wilc_fix_rcu_usage-v1-1-f610e46c6f82@bootlin.com/

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

Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage

[Alexis Lothoré]

On 2/19/24 17:19, Kalle Valo wrote:
> Alexis Lothoré <alexis.lothore@bootlin.com> writes:
> 
>> This small series aims to fix multiple warnings observed when enabling
>> CONFIG_PROVE_RCU_LIST:
>> - add missing locks to create corresponding critical read sections
>> - fix mix between RCU and SRCU API usage
>>
>> While at it, since SRCU API is already in use in the driver, any fix done
>> on RCU usage was also done with the SRCU variant of RCU API. I do not
>> really get why we are using SRCU in this driver instead of classic RCU, as
>> it seems to be done in any other wireless driver.
> 
> And even more so, no other driver in drivers/net use SRCU.
> 
>> My understanding is that primary SRCU use case is for compatibility
>> with realtime kernel, which needs to be preemptible everywhere. Has
>> the driver been really developped with this constraint in mind ? If
>> you have more details about this, feel free to educate me.
> 
> Alexis, if you have the time I recommend submitting a patchset
> converting wilc1000 to use classic RCU. At least I have a hard time
> understanding why SRCU is needed, especially after seeing the warning
> you found.

If nobody else comes in with a strong argument in favor of keeping SRCU, yes I
can certainly add that to my backlog :)

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

Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage

[Kalle Valo]

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

> On 2/19/24 17:19, Kalle Valo wrote:
>
>> Alexis Lothoré <alexis.lothore@bootlin.com> writes:
>> 
>>> This small series aims to fix multiple warnings observed when enabling
>>> CONFIG_PROVE_RCU_LIST:
>>> - add missing locks to create corresponding critical read sections
>>> - fix mix between RCU and SRCU API usage
>>>
>>> While at it, since SRCU API is already in use in the driver, any fix done
>>> on RCU usage was also done with the SRCU variant of RCU API. I do not
>>> really get why we are using SRCU in this driver instead of classic RCU, as
>>> it seems to be done in any other wireless driver.
>> 
>> And even more so, no other driver in drivers/net use SRCU.
>> 
>>> My understanding is that primary SRCU use case is for compatibility
>>> with realtime kernel, which needs to be preemptible everywhere. Has
>>> the driver been really developped with this constraint in mind ? If
>>> you have more details about this, feel free to educate me.
>> 
>> Alexis, if you have the time I recommend submitting a patchset
>> converting wilc1000 to use classic RCU. At least I have a hard time
>> understanding why SRCU is needed, especially after seeing the warning
>> you found.
>
> If nobody else comes in with a strong argument in favor of keeping
> SRCU

And emphasis on the word "strong"...

> yes I can certainly add that to my backlog :)

Thanks! Your wilc1000 backlog is getting long, I hope your todo software
won't overload ;)

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

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