* [PATCH 1/4] wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper
2024-02-15 15:36 [PATCH 0/4] wifi: wilc1000: fix RCU usage Alexis Lothoré
@ 2024-02-15 15:36 ` Alexis Lothoré
2024-02-19 16:21 ` Kalle Valo
2024-02-15 15:36 ` [PATCH 2/4] wifi: wilc1000: use SRCU instead of RCU for vif list traversal Alexis Lothoré
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Alexis Lothoré @ 2024-02-15 15:36 UTC (permalink / raw)
To: linux-wireless
Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/4] wifi: wilc1000: use SRCU instead of RCU for vif list traversal
2024-02-15 15:36 [PATCH 0/4] wifi: wilc1000: fix RCU usage Alexis Lothoré
2024-02-15 15:36 ` [PATCH 1/4] wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper Alexis Lothoré
@ 2024-02-15 15:36 ` Alexis Lothoré
2024-02-15 15:36 ` [PATCH 3/4] wifi: wilc1000: fix declarations ordering Alexis Lothoré
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Alexis Lothoré @ 2024-02-15 15:36 UTC (permalink / raw)
To: linux-wireless
Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/4] wifi: wilc1000: fix declarations ordering
2024-02-15 15:36 [PATCH 0/4] wifi: wilc1000: fix RCU usage Alexis Lothoré
2024-02-15 15:36 ` [PATCH 1/4] wifi: wilc1000: split deeply nested RCU list traversal in dedicated helper Alexis Lothoré
2024-02-15 15:36 ` [PATCH 2/4] wifi: wilc1000: use SRCU instead of RCU for vif list traversal Alexis Lothoré
@ 2024-02-15 15:36 ` Alexis Lothoré
2024-02-15 15:36 ` [PATCH 4/4] wifi: wilc1000: add missing read critical sections around vif list traversal Alexis Lothoré
2024-02-19 16:19 ` [PATCH 0/4] wifi: wilc1000: fix RCU usage Kalle Valo
4 siblings, 0 replies; 9+ messages in thread
From: Alexis Lothoré @ 2024-02-15 15:36 UTC (permalink / raw)
To: linux-wireless
Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/4] wifi: wilc1000: add missing read critical sections around vif list traversal
2024-02-15 15:36 [PATCH 0/4] wifi: wilc1000: fix RCU usage Alexis Lothoré
` (2 preceding siblings ...)
2024-02-15 15:36 ` [PATCH 3/4] wifi: wilc1000: fix declarations ordering Alexis Lothoré
@ 2024-02-15 15:36 ` Alexis Lothoré
2024-02-19 16:19 ` [PATCH 0/4] wifi: wilc1000: fix RCU usage Kalle Valo
4 siblings, 0 replies; 9+ messages in thread
From: Alexis Lothoré @ 2024-02-15 15:36 UTC (permalink / raw)
To: linux-wireless
Cc: Ajay Singh, Claudiu Beznea, Kalle Valo, Thomas Petazzoni,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage
2024-02-15 15:36 [PATCH 0/4] wifi: wilc1000: fix RCU usage Alexis Lothoré
` (3 preceding siblings ...)
2024-02-15 15:36 ` [PATCH 4/4] wifi: wilc1000: add missing read critical sections around vif list traversal Alexis Lothoré
@ 2024-02-19 16:19 ` Kalle Valo
2024-02-19 16:24 ` Alexis Lothoré
4 siblings, 1 reply; 9+ messages in thread
From: Kalle Valo @ 2024-02-19 16:19 UTC (permalink / raw)
To: Alexis Lothoré
Cc: linux-wireless, Ajay Singh, Claudiu Beznea, Thomas Petazzoni,
linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage
2024-02-19 16:19 ` [PATCH 0/4] wifi: wilc1000: fix RCU usage Kalle Valo
@ 2024-02-19 16:24 ` Alexis Lothoré
2024-02-19 16:34 ` Kalle Valo
0 siblings, 1 reply; 9+ messages in thread
From: Alexis Lothoré @ 2024-02-19 16:24 UTC (permalink / raw)
To: Kalle Valo
Cc: linux-wireless, Ajay Singh, Claudiu Beznea, Thomas Petazzoni,
linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] wifi: wilc1000: fix RCU usage
2024-02-19 16:24 ` Alexis Lothoré
@ 2024-02-19 16:34 ` Kalle Valo
0 siblings, 0 replies; 9+ messages in thread
From: Kalle Valo @ 2024-02-19 16:34 UTC (permalink / raw)
To: Alexis Lothoré
Cc: linux-wireless, Ajay Singh, Claudiu Beznea, Thomas Petazzoni,
linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread