On 3/25/24 20:51, Johannes Berg wrote:
On Tue, 2024-03-12 at 21:16 +0530, Aditya Kumar Singh wrote:
In such cases, the bss active flag might not provide the exact
status of the MLD links.
That's ... true I guess, but then I'm suspicious, why are you sending
this patch? Does that mean 'active' is managed incorrectly and actually
becomes false when one link is removed, rather than all?
Yes I believe. I would say it is managing the pre-MLO time data
structures. As of today, the active flag is set at assign beacon. But
since it is maintained at sdata level, as soon as first link is assigned
beacon, the flag would be set. Similarly, at stop_ap() it is set to
false, so as soon as first link is brought down, it is set to false.
Hence checking that sdata level flag during MLO scenario might be
misleading.
Can you fix
that too? And if you fix that ... yeah we probably still should have
this patch but ... _without_ this:
Sure let me try to fix that as well. So here's what Im planning -
1. Separate the ether_addr changes into a separate independent patch.
2. Patch series to fix the active flag handling at link level.
+ /* This is consolidated status of the MLD or non ML bss */
+ if (sdata->bss->active)
+ return true;
I'd think?
Yes :)
While at it, when source address is same as the link conf's address and
if userspace requested Tx on a specific link, add changes to use the same
link id if the link bss is matching the requested channel.
Why not separate that? It's really not related much?
Yeah will put this in separate patch. Thanks.
+ if (link_id < 0)
+ return false;
+
+ if (!sdata->vif.valid_links)
+ return false;
+
+ if (!(sdata->vif.valid_links & BIT(link_id)))
+ return false;
The second condition seems useless then?
Yeah. I would say "if (!sdata->vif.valid_links)" this check can be
removed. Will remove it.
But probably better to check *active* links, and then might as well use
ieee80211_vif_link_active()?
Sure, I will see what I can do here. Thanks for the suggestion.
+ link = sdata_dereference(sdata->link[link_id], sdata);
+ if (!link)
+ return false;
That might be a WARN_ON()? After all, if links are valid (or active per
above) that really should be there?
Sure, will do.
int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
struct cfg80211_mgmt_tx_params *params, u64 *cookie)
{
@@ -817,7 +850,7 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
case NL80211_IFTYPE_P2P_GO:
if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
!ieee80211_vif_is_mesh(&sdata->vif) &&
- !sdata->bss->active)
+ !ieee80211_is_link_bss_active(sdata, params->link_id))
need_offchan = true;
rcu_read_lock();
@@ -897,8 +930,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
break;
}
- if (ether_addr_equal(conf->addr, mgmt->sa))
+ if (ether_addr_equal(conf->addr, mgmt->sa)) {
+ /* If userspace requested Tx on a specific link
+ * use the same link id if the link bss is matching
+ * the requested chan.
+ */
+ if (sdata->vif.valid_links &&
+ params->link_id >= 0 && params->link_id == i &&
+ params->chan == chanctx_conf->def.chan)
+ link_id = i;
break;
+ }
chanctx_conf = NULL;
}
base-commit: c2b25092864a16c7865e406badedece5cc25fc2b
