Skip to content

Commit

Permalink
Revert "wifi: wilc1000: convert list management to RCU"
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Alexis Lothoré authored and Kalle Valo committed Jun 1, 2024
1 parent 10bc855 commit ebfb5e8
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 45 deletions.
41 changes: 24 additions & 17 deletions drivers/net/wireless/microchip/wilc1000/cfg80211.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,12 @@ static int set_channel(struct wiphy *wiphy,
struct wilc_vif *vif;
u32 channelnum;
int result;
int srcu_idx;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
vif = wilc_get_wl_to_vif(wl);
if (IS_ERR(vif)) {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return PTR_ERR(vif);
}

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

rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return result;
}

Expand Down Expand Up @@ -805,8 +806,9 @@ static int set_wiphy_params(struct wiphy *wiphy, u32 changed)
struct wilc *wl = wiphy_priv(wiphy);
struct wilc_vif *vif;
struct wilc_priv *priv;
int srcu_idx;

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

out:
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return ret;
}

Expand Down Expand Up @@ -1537,32 +1539,33 @@ static struct wireless_dev *add_virtual_intf(struct wiphy *wiphy,

if (type == NL80211_IFTYPE_MONITOR) {
struct net_device *ndev;
int srcu_idx;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
vif = wilc_get_vif_from_type(wl, WILC_AP_MODE);
if (!vif) {
vif = wilc_get_vif_from_type(wl, WILC_GO_MODE);
if (!vif) {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
goto validate_interface;
}
}

if (vif->monitor_flag) {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
goto validate_interface;
}

ndev = wilc_wfi_init_mon_interface(wl, name, vif->ndev);
if (ndev) {
vif->monitor_flag = 1;
} else {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return ERR_PTR(-EINVAL);
}

wdev = &vif->priv.wdev;
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return wdev;
}

Expand Down Expand Up @@ -1610,7 +1613,7 @@ static int del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev)
list_del_rcu(&vif->list);
wl->vif_num--;
mutex_unlock(&wl->vif_mutex);
synchronize_rcu();
synchronize_srcu(&wl->srcu);
return 0;
}

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

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
vif = wilc_get_wl_to_vif(wl);
if (IS_ERR(vif)) {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return;
}

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

static int set_tx_power(struct wiphy *wiphy, struct wireless_dev *wdev,
enum nl80211_tx_power_setting type, int mbm)
{
int ret;
int srcu_idx;
s32 tx_power = MBM_TO_DBM(mbm);
struct wilc *wl = wiphy_priv(wiphy);
struct wilc_vif *vif;

if (!wl->initialized)
return -EIO;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
vif = wilc_get_wl_to_vif(wl);
if (IS_ERR(vif)) {
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return -EINVAL;
}

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

return ret;
}
Expand Down Expand Up @@ -1757,6 +1762,7 @@ static void wlan_init_locks(struct wilc *wl)
init_completion(&wl->cfg_event);
init_completion(&wl->sync_event);
init_completion(&wl->txq_thread_started);
init_srcu_struct(&wl->srcu);
}

void wlan_deinit_locks(struct wilc *wilc)
Expand All @@ -1767,6 +1773,7 @@ void wlan_deinit_locks(struct wilc *wilc)
mutex_destroy(&wilc->txq_add_to_head_cs);
mutex_destroy(&wilc->vif_mutex);
mutex_destroy(&wilc->deinit_lock);
cleanup_srcu_struct(&wilc->srcu);
}

int wilc_cfg80211_init(struct wilc **wilc, struct device *dev, int io_type,
Expand Down
15 changes: 9 additions & 6 deletions drivers/net/wireless/microchip/wilc1000/hif.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1570,11 +1570,12 @@ 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]);
rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
vif = wilc_get_vif_from_idx(wilc, id);
if (!vif)
goto out;
Expand Down Expand Up @@ -1606,21 +1607,22 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
kfree(msg);
}
out:
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
}

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]);
rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
vif = wilc_get_vif_from_idx(wilc, id);
if (!vif)
goto out;
Expand All @@ -1647,19 +1649,20 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length)
kfree(msg);
}
out:
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
mutex_unlock(&wilc->deinit_lock);
}

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]);
rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
vif = wilc_get_vif_from_idx(wilc, id);
if (!vif)
goto out;
Expand All @@ -1684,7 +1687,7 @@ void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length)
}
}
out:
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
}

int wilc_remain_on_channel(struct wilc_vif *vif, u64 cookie, u16 chan,
Expand Down
43 changes: 25 additions & 18 deletions drivers/net/wireless/microchip/wilc1000/netdev.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,28 +127,30 @@ void wilc_wlan_set_bssid(struct net_device *wilc_netdev, const u8 *bssid,

int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
{
int srcu_idx;
u8 ret_val = 0;
struct wilc_vif *vif;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
wilc_for_each_vif(wilc, vif) {
if (!is_zero_ether_addr(vif->bssid))
ret_val++;
}
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
return ret_val;
}

static void wilc_wake_tx_queues(struct wilc *wl)
{
int srcu_idx;
struct wilc_vif *ifc;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
wilc_for_each_vif(wl, ifc) {
if (ifc->mac_opened && netif_queue_stopped(ifc->ndev))
netif_wake_queue(ifc->ndev);
}
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
}

static int wilc_txq_task(void *vp)
Expand Down Expand Up @@ -653,6 +655,7 @@ static int wilc_set_mac_addr(struct net_device *dev, void *p)
struct sockaddr *addr = (struct sockaddr *)p;
unsigned char mac_addr[ETH_ALEN];
struct wilc_vif *tmp_vif;
int srcu_idx;

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

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

rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
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) {
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
return -EADDRNOTAVAIL;
}
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
return 0;
}
}
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);

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

if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
int srcu_idx;
struct wilc_vif *vif;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
wilc_for_each_vif(wilc, vif) {
if (vif->mac_opened)
netif_stop_queue(vif->ndev);
}
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
}

return NETDEV_TX_OK;
Expand Down Expand Up @@ -815,12 +819,13 @@ 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;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
wilc_netdev = get_if_handler(wilc, buff);
if (!wilc_netdev)
goto out;
Expand Down Expand Up @@ -848,14 +853,15 @@ void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size,
netdev_dbg(wilc_netdev, "netif_rx ret value is: %d\n", stats);
}
out:
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
}

void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)
{
int srcu_idx;
struct wilc_vif *vif;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wilc->srcu);
wilc_for_each_vif(wilc, vif) {
struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)buff;
u16 type = le16_to_cpup((__le16 *)buff);
Expand All @@ -876,7 +882,7 @@ void wilc_wfi_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size, bool is_auth)
if (vif->monitor_flag)
wilc_wfi_monitor_rx(wilc->monitor_dev, buff, size);
}
rcu_read_unlock();
srcu_read_unlock(&wilc->srcu, srcu_idx);
}

static const struct net_device_ops wilc_netdev_ops = {
Expand Down Expand Up @@ -906,7 +912,7 @@ void wilc_netdev_cleanup(struct wilc *wilc)
list_del_rcu(&vif->list);
wilc->vif_num--;
mutex_unlock(&wilc->vif_mutex);
synchronize_rcu();
synchronize_srcu(&wilc->srcu);
if (vif->ndev)
unregister_netdev(vif->ndev);
}
Expand All @@ -925,15 +931,16 @@ static u8 wilc_get_available_idx(struct wilc *wl)
{
int idx = 0;
struct wilc_vif *vif;
int srcu_idx;

rcu_read_lock();
srcu_idx = srcu_read_lock(&wl->srcu);
wilc_for_each_vif(wl, vif) {
if (vif->idx == 0)
idx = 1;
else
idx = 0;
}
rcu_read_unlock();
srcu_read_unlock(&wl->srcu, srcu_idx);
return idx;
}

Expand Down Expand Up @@ -983,7 +990,7 @@ struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
list_add_tail_rcu(&vif->list, &wl->vif_list);
wl->vif_num += 1;
mutex_unlock(&wl->vif_mutex);
synchronize_rcu();
synchronize_srcu(&wl->srcu);

return vif;

Expand Down
Loading

0 comments on commit ebfb5e8

Please sign in to comment.