On 11/15/24 13:44, Johannes Berg wrote:
On Wed, 2024-11-13 at 21:50 +0530, Aditya Kumar Singh wrote:
Because link ID is cleared from the bitmap well before link stop is
called. As mentioned in commit message, this is the flow -
nl80211_remove_link
> cfg80211_remove_link -> link ID gets updated here
> ieee80211_del_intf_link
> ieee80211_vif_set_links
> ieee80211_vif_update_links
> ieee80211_link_stop -> this ultimately tries to stop
CAC if it is ongoing.
OK, but why does it have to be that way? It's all under wiphy mutex, so
perhaps we could just clear it later?
Yeah. I tried below diff, hwsim test cases shows no regression.
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -5046,10 +5046,11 @@ static void ieee80211_del_intf_link(struct wiphy
*wiphy,
unsigned int link_id)
{
struct ieee80211_sub_if_data *sdata =
IEEE80211_WDEV_TO_SUB_IF(wdev);
+ u16 new_links = wdev->valid_links & ~BIT(link_id);
lockdep_assert_wiphy(sdata->local->hw.wiphy);
- ieee80211_vif_set_links(sdata, wdev->valid_links, 0);
+ ieee80211_vif_set_links(sdata, new_links, 0);
}
static int
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 040d62051eb9..65c8e47246b7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -2843,10 +2843,9 @@ void cfg80211_remove_link(struct wireless_dev
*wdev, unsigned int link_id)
break;
}
- wdev->valid_links &= ~BIT(link_id);
-
rdev_del_intf_link(rdev, wdev, link_id);
+ wdev->valid_links &= ~BIT(link_id);
eth_zero_addr(wdev->links[link_id].addr);
}
So I will submit this as patch then?
There's necessarily going to be some temporary inconsistency here, I'm
not sure it matters too much if it isn't very visible?
Any particular case you suspect and want me to test?
Alternatively, could do something like
wdev->valid_links &= ~BIT(link_id);
wdev->removing_link = link_id;
...
wdev->removing_link = -1;
and accept the wdev->removing_link in these APIs like CAC?
Umm.. will work for CAC stopping from user space. But if radar is
detected, in this flow (driver -> mac80211 -> cfg80211), valid_link
bitmap is still valid and hence valid_bitmap check works and there will
be no removing_links set.
So then may be both needs to be checked? Like either the link_id should
be present in valid_link or it should be the removing_link?
if (WARN_ON(wdev->removing_link != link_id &&
wdev->valid_links && !(wdev->valid_links & BIT(link_id))))
return;
I'm more inclining towards the first suggestion you gave - clearing the
bitmap later. What's your suggestion?
--
Aditya
