Skip to content

Commit ebfb5e8

Browse files
TropicaoKalle Valo
Tropicao
authored and
Kalle Valo
committed
Revert "wifi: wilc1000: convert list management to RCU"
This reverts commit f236464 Commit f236464 ("wifi: wilc1000: convert list management to RCU") replaced SRCU with RCU, aiming to simplify RCU usage in the driver. No documentation or commit history hinted about why SRCU has been preferred in original design, so it has been assumed to be safe to do this conversion. Unfortunately, some static analyzers raised warnings, confirmed by runtime checker, not long after the merge. At least three different issues arose when switching to RCU: - wilc_wlan_txq_filter_dup_tcp_ack is executed in a RCU read critical section yet calls wait_for_completion_timeout - wilc_wfi_init_mon_interface calls kmalloc and register_netdevice while manipulating a vif retrieved from vif list - set_channel sends command to chip (and so, also waits for a completion) while holding a vif retrieved from vif list (so, in RCU read critical section) Some of those issues are not trivial to fix and would need bigger driver rework. Fix those issues by reverting the SRCU to RCU conversion commit Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/linux-wireless/3b46ec7c-baee-49fd-b760-3bc12fb12eaf@moroto.mountain/ Fixes: f236464 ("wifi: wilc1000: convert list management to RCU") Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com> Signed-off-by: Kalle Valo <kvalo@kernel.org> Link: https://msgid.link/20240528-wilc_revert_srcu_to_rcu-v1-1-bce096e0798c@bootlin.com
1 parent 10bc855 commit ebfb5e8

5 files changed

Lines changed: 64 additions & 45 deletions

File tree

‎drivers/net/wireless/microchip/wilc1000/cfg80211.c‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -237,11 +237,12 @@ static int set_channel(struct wiphy *wiphy,
237237
struct wilc_vif *vif;
238238
u32 channelnum;
239239
int result;
240+
int srcu_idx;
240241

241-
rcu_read_lock();
242+
srcu_idx = srcu_read_lock(&wl->srcu);
242243
vif = wilc_get_wl_to_vif(wl);
243244
if (IS_ERR(vif)) {
244-
rcu_read_unlock();
245+
srcu_read_unlock(&wl->srcu, srcu_idx);
245246
return PTR_ERR(vif);
246247
}
247248

@@ -252,7 +253,7 @@ static int set_channel(struct wiphy *wiphy,
252253
if (result)
253254
netdev_err(vif->ndev, "Error in setting channel\n");
254255

255-
rcu_read_unlock();
256+
srcu_read_unlock(&wl->srcu, srcu_idx);
256257
return result;
257258
}
258259

@@ -805,8 +806,9 @@ static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
805806
struct wilc *wl = wiphy_priv(wiphy);
806807
struct wilc_vif *vif;
807808
struct wilc_priv *priv;
809+
int srcu_idx;
808810

809-
rcu_read_lock();
811+
srcu_idx = srcu_read_lock(&wl->srcu);
810812
vif = wilc_get_wl_to_vif(wl);
811813
if (IS_ERR(vif))
812814
goto out;
@@ -861,7 +863,7 @@ static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
861863
netdev_err(priv->dev, "Error in setting WIPHY PARAMS\n");
862864

863865
out:
864-
rcu_read_unlock();
866+
srcu_read_unlock(&wl->srcu, srcu_idx);
865867
return ret;
866868
}
867869

@@ -1537,32 +1539,33 @@ static struct wireless_dev *add_virtual_intf(struct wiphy *wiphy,
15371539

15381540
if (type == NL80211_IFTYPE_MONITOR) {
15391541
struct net_device *ndev;
1542+
int srcu_idx;
15401543

1541-
rcu_read_lock();
1544+
srcu_idx = srcu_read_lock(&wl->srcu);
15421545
vif = wilc_get_vif_from_type(wl, WILC_AP_MODE);
15431546
if (!vif) {
15441547
vif = wilc_get_vif_from_type(wl, WILC_GO_MODE);
15451548
if (!vif) {
1546-
rcu_read_unlock();
1549+
srcu_read_unlock(&wl->srcu, srcu_idx);
15471550
goto validate_interface;
15481551
}
15491552
}
15501553

15511554
if (vif->monitor_flag) {
1552-
rcu_read_unlock();
1555+
srcu_read_unlock(&wl->srcu, srcu_idx);
15531556
goto validate_interface;
15541557
}
15551558

15561559
ndev = wilc_wfi_init_mon_interface(wl, name, vif->ndev);
15571560
if (ndev) {
15581561
vif->monitor_flag = 1;
15591562
} else {
1560-
rcu_read_unlock();
1563+
srcu_read_unlock(&wl->srcu, srcu_idx);
15611564
return ERR_PTR(-EINVAL);
15621565
}
15631566

15641567
wdev = &vif->priv.wdev;
1565-
rcu_read_unlock();
1568+
srcu_read_unlock(&wl->srcu, srcu_idx);
15661569
return wdev;
15671570
}
15681571

@@ -1610,7 +1613,7 @@ static int del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev)
16101613
list_del_rcu(&vif->list);
16111614
wl->vif_num--;
16121615
mutex_unlock(&wl->vif_mutex);
1613-
synchronize_rcu();
1616+
synchronize_srcu(&wl->srcu);
16141617
return 0;
16151618
}
16161619

@@ -1635,34 +1638,36 @@ static void wilc_set_wakeup(struct wiphy *wiphy, bool enabled)
16351638
{
16361639
struct wilc *wl = wiphy_priv(wiphy);
16371640
struct wilc_vif *vif;
1641+
int srcu_idx;
16381642

1639-
rcu_read_lock();
1643+
srcu_idx = srcu_read_lock(&wl->srcu);
16401644
vif = wilc_get_wl_to_vif(wl);
16411645
if (IS_ERR(vif)) {
1642-
rcu_read_unlock();
1646+
srcu_read_unlock(&wl->srcu, srcu_idx);
16431647
return;
16441648
}
16451649

16461650
netdev_info(vif->ndev, "cfg set wake up = %d\n", enabled);
16471651
wilc_set_wowlan_trigger(vif, enabled);
1648-
rcu_read_unlock();
1652+
srcu_read_unlock(&wl->srcu, srcu_idx);
16491653
}
16501654

16511655
static int set_tx_power(struct wiphy *wiphy, struct wireless_dev *wdev,
16521656
enum nl80211_tx_power_setting type, int mbm)
16531657
{
16541658
int ret;
1659+
int srcu_idx;
16551660
s32 tx_power = MBM_TO_DBM(mbm);
16561661
struct wilc *wl = wiphy_priv(wiphy);
16571662
struct wilc_vif *vif;
16581663

16591664
if (!wl->initialized)
16601665
return -EIO;
16611666

1662-
rcu_read_lock();
1667+
srcu_idx = srcu_read_lock(&wl->srcu);
16631668
vif = wilc_get_wl_to_vif(wl);
16641669
if (IS_ERR(vif)) {
1665-
rcu_read_unlock();
1670+
srcu_read_unlock(&wl->srcu, srcu_idx);
16661671
return -EINVAL;
16671672
}
16681673

@@ -1674,7 +1679,7 @@ static int set_tx_power(struct wiphy *wiphy, struct wireless_dev *wdev,
16741679
ret = wilc_set_tx_power(vif, tx_power);
16751680
if (ret)
16761681
netdev_err(vif->ndev, "Failed to set tx power\n");
1677-
rcu_read_unlock();
1682+
srcu_read_unlock(&wl->srcu, srcu_idx);
16781683

16791684
return ret;
16801685
}
@@ -1757,6 +1762,7 @@ static void wlan_init_locks(struct wilc *wl)
17571762
init_completion(&wl->cfg_event);
17581763
init_completion(&wl->sync_event);
17591764
init_completion(&wl->txq_thread_started);
1765+
init_srcu_struct(&wl->srcu);
17601766
}
17611767

17621768
void wlan_deinit_locks(struct wilc *wilc)
@@ -1767,6 +1773,7 @@ void wlan_deinit_locks(struct wilc *wilc)
17671773
mutex_destroy(&wilc->txq_add_to_head_cs);
17681774
mutex_destroy(&wilc->vif_mutex);
17691775
mutex_destroy(&wilc->deinit_lock);
1776+
cleanup_srcu_struct(&wilc->srcu);
17701777
}
17711778

17721779
int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type,

‎drivers/net/wireless/microchip/wilc1000/hif.c‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,11 +1570,12 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
15701570
struct host_if_drv *hif_drv;
15711571
struct host_if_msg *msg;
15721572
struct wilc_vif *vif;
1573+
int srcu_idx;
15731574
int result;
15741575
int id;
15751576

15761577
id = get_unaligned_le32(&buffer[length - 4]);
1577-
rcu_read_lock();
1578+
srcu_idx = srcu_read_lock(&wilc->srcu);
15781579
vif = wilc_get_vif_from_idx(wilc, id);
15791580
if (!vif)
15801581
goto out;
@@ -1606,21 +1607,22 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
16061607
kfree(msg);
16071608
}
16081609
out:
1609-
rcu_read_unlock();
1610+
srcu_read_unlock(&wilc->srcu, srcu_idx);
16101611
}
16111612

16121613
void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
16131614
{
16141615
struct host_if_drv *hif_drv;
16151616
struct host_if_msg *msg;
16161617
struct wilc_vif *vif;
1618+
int srcu_idx;
16171619
int result;
16181620
int id;
16191621

16201622
mutex_lock(&wilc->deinit_lock);
16211623

16221624
id = get_unaligned_le32(&buffer[length - 4]);
1623-
rcu_read_lock();
1625+
srcu_idx = srcu_read_lock(&wilc->srcu);
16241626
vif = wilc_get_vif_from_idx(wilc, id);
16251627
if (!vif)
16261628
goto out;
@@ -1647,19 +1649,20 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
16471649
kfree(msg);
16481650
}
16491651
out:
1650-
rcu_read_unlock();
1652+
srcu_read_unlock(&wilc->srcu, srcu_idx);
16511653
mutex_unlock(&wilc->deinit_lock);
16521654
}
16531655

16541656
void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
16551657
{
16561658
struct host_if_drv *hif_drv;
16571659
struct wilc_vif *vif;
1660+
int srcu_idx;
16581661
int result;
16591662
int id;
16601663

16611664
id = get_unaligned_le32(&buffer[length - 4]);
1662-
rcu_read_lock();
1665+
srcu_idx = srcu_read_lock(&wilc->srcu);
16631666
vif = wilc_get_vif_from_idx(wilc, id);
16641667
if (!vif)
16651668
goto out;
@@ -1684,7 +1687,7 @@ void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
16841687
}
16851688
}
16861689
out:
1687-
rcu_read_unlock();
1690+
srcu_read_unlock(&wilc->srcu, srcu_idx);
16881691
}
16891692

16901693
int wilc_remain_on_channel(struct wilc_vif *vif, u64 cookie, u16 chan,

‎drivers/net/wireless/microchip/wilc1000/netdev.c‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -127,28 +127,30 @@ void wilc_wlan_set_bssid(struct net_device *wilc_netdev, const u8 *bssid,
127127

128128
int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
129129
{
130+
int srcu_idx;
130131
u8 ret_val = 0;
131132
struct wilc_vif *vif;
132133

133-
rcu_read_lock();
134+
srcu_idx = srcu_read_lock(&wilc->srcu);
134135
wilc_for_each_vif(wilc, vif) {
135136
if (!is_zero_ether_addr(vif->bssid))
136137
ret_val++;
137138
}
138-
rcu_read_unlock();
139+
srcu_read_unlock(&wilc->srcu, srcu_idx);
139140
return ret_val;
140141
}
141142

142143
static void wilc_wake_tx_queues(struct wilc *wl)
143144
{
145+
int srcu_idx;
144146
struct wilc_vif *ifc;
145147

146-
rcu_read_lock();
148+
srcu_idx = srcu_read_lock(&wl->srcu);
147149
wilc_for_each_vif(wl, ifc) {
148150
if (ifc->mac_opened && netif_queue_stopped(ifc->ndev))
149151
netif_wake_queue(ifc->ndev);
150152
}
151-
rcu_read_unlock();
153+
srcu_read_unlock(&wl->srcu, srcu_idx);
152154
}
153155

154156
static int wilc_txq_task(void *vp)
@@ -653,6 +655,7 @@ static int wilc_set_mac_addr(struct net_device *dev, void *p)
653655
struct sockaddr *addr = (struct sockaddr *)p;
654656
unsigned char mac_addr[ETH_ALEN];
655657
struct wilc_vif *tmp_vif;
658+
int srcu_idx;
656659

657660
if (!is_valid_ether_addr(addr->sa_data))
658661
return -EADDRNOTAVAIL;
@@ -664,19 +667,19 @@ static int wilc_set_mac_addr(struct net_device *dev, void *p)
664667

665668
/* Verify MAC Address is not already in use: */
666669

667-
rcu_read_lock();
670+
srcu_idx = srcu_read_lock(&wilc->srcu);
668671
wilc_for_each_vif(wilc, tmp_vif) {
669672
wilc_get_mac_address(tmp_vif, mac_addr);
670673
if (ether_addr_equal(addr->sa_data, mac_addr)) {
671674
if (vif != tmp_vif) {
672-
rcu_read_unlock();
675+
srcu_read_unlock(&wilc->srcu, srcu_idx);
673676
return -EADDRNOTAVAIL;
674677
}
675-
rcu_read_unlock();
678+
srcu_read_unlock(&wilc->srcu, srcu_idx);
676679
return 0;
677680
}
678681
}
679-
rcu_read_unlock();
682+
srcu_read_unlock(&wilc->srcu, srcu_idx);
680683

681684
result = wilc_set_mac_address(vif, (u8 *)addr->sa_data);
682685
if (result)
@@ -764,14 +767,15 @@ netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *ndev)
764767
wilc_tx_complete);
765768

766769
if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
770+
int srcu_idx;
767771
struct wilc_vif *vif;
768772

769-
rcu_read_lock();
773+
srcu_idx = srcu_read_lock(&wilc->srcu);
770774
wilc_for_each_vif(wilc, vif) {
771775
if (vif->mac_opened)
772776
netif_stop_queue(vif->ndev);
773777
}
774-
rcu_read_unlock();
778+
srcu_read_unlock(&wilc->srcu, srcu_idx);
775779
}
776780

777781
return NETDEV_TX_OK;
@@ -815,12 +819,13 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
815819
unsigned int frame_len = 0;
816820
struct wilc_vif *vif;
817821
struct sk_buff *skb;
822+
int srcu_idx;
818823
int stats;
819824

820825
if (!wilc)
821826
return;
822827

823-
rcu_read_lock();
828+
srcu_idx = srcu_read_lock(&wilc->srcu);
824829
wilc_netdev = get_if_handler(wilc, buff);
825830
if (!wilc_netdev)
826831
goto out;
@@ -848,14 +853,15 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
848853
netdev_dbg(wilc_netdev, "netif_rx ret value is: %d\n", stats);
849854
}
850855
out:
851-
rcu_read_unlock();
856+
srcu_read_unlock(&wilc->srcu, srcu_idx);
852857
}
853858

854859
void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)
855860
{
861+
int srcu_idx;
856862
struct wilc_vif *vif;
857863

858-
rcu_read_lock();
864+
srcu_idx = srcu_read_lock(&wilc->srcu);
859865
wilc_for_each_vif(wilc, vif) {
860866
struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)buff;
861867
u16 type = le16_to_cpup((__le16 *)buff);
@@ -876,7 +882,7 @@ void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)
876882
if (vif->monitor_flag)
877883
wilc_wfi_monitor_rx(wilc->monitor_dev, buff, size);
878884
}
879-
rcu_read_unlock();
885+
srcu_read_unlock(&wilc->srcu, srcu_idx);
880886
}
881887

882888
static const struct net_device_ops wilc_netdev_ops = {
@@ -906,7 +912,7 @@ void wilc_netdev_cleanup(struct wilc *wilc)
906912
list_del_rcu(&vif->list);
907913
wilc->vif_num--;
908914
mutex_unlock(&wilc->vif_mutex);
909-
synchronize_rcu();
915+
synchronize_srcu(&wilc->srcu);
910916
if (vif->ndev)
911917
unregister_netdev(vif->ndev);
912918
}
@@ -925,15 +931,16 @@ static u8 wilc_get_available_idx(struct wilc *wl)
925931
{
926932
int idx = 0;
927933
struct wilc_vif *vif;
934+
int srcu_idx;
928935

929-
rcu_read_lock();
936+
srcu_idx = srcu_read_lock(&wl->srcu);
930937
wilc_for_each_vif(wl, vif) {
931938
if (vif->idx == 0)
932939
idx = 1;
933940
else
934941
idx = 0;
935942
}
936-
rcu_read_unlock();
943+
srcu_read_unlock(&wl->srcu, srcu_idx);
937944
return idx;
938945
}
939946

@@ -983,7 +990,7 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
983990
list_add_tail_rcu(&vif->list, &wl->vif_list);
984991
wl->vif_num += 1;
985992
mutex_unlock(&wl->vif_mutex);
986-
synchronize_rcu();
993+
synchronize_srcu(&wl->srcu);
987994

988995
return vif;
989996

0 commit comments

Comments
 (0)