| Message ID | d42ef01b-996b-a645-d59e-f3dec5a974a9@candelatech.com |
|---|---|
| State | New |
| Headers | show |
| Series | HACK/RFC: Fix link_sta->rx_nss == 0 in iwlwifi upon eMLSR link change. | expand |
On Tue, 2024-07-16 at 16:25 -0700, Ben Greear wrote: > While poking around at some instability and poor performance seen in download > direction, I noticed that the rate-ctrl was probably set incorrectly in > the iwlwifi driver due to link_sta->rx_nss being zero when changing active link > to the secondary link (the one we didn't originally associate with). > > After debugging, I found that the hack below will make this problem > go away. I sincerely doubt this is the correct approach, but I'm not > sure how it is all supposed to work in the first place. Andrei came up with this, which does seem better, but probably wouldn't address the AP side: diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index d624c51d0bd1..8d32adf7502d 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -5744,6 +5744,7 @@ static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata, } if (link_id != assoc_data->assoc_link_id) { + ieee80211_sta_init_nss(link_sta); err = ieee80211_sta_activate_link(sta, link_id); if (err) goto out_err; Care to test it? In general I think we should probably remove the call to ieee80211_sta_init_nss() from rate_control_rate_init() and call it explicitly wherever needed, since with MLO we require offloaded rate control and rate_control_rate_init() doesn't really do anything (except this for the deflink, which is then questionable) johannes
On 8/27/24 08:51, Johannes Berg wrote: > On Tue, 2024-07-16 at 16:25 -0700, Ben Greear wrote: >> While poking around at some instability and poor performance seen in download >> direction, I noticed that the rate-ctrl was probably set incorrectly in >> the iwlwifi driver due to link_sta->rx_nss being zero when changing active link >> to the secondary link (the one we didn't originally associate with). >> >> After debugging, I found that the hack below will make this problem >> go away. I sincerely doubt this is the correct approach, but I'm not >> sure how it is all supposed to work in the first place. > > Andrei came up with this, which does seem better, but probably wouldn't > address the AP side: > > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > index d624c51d0bd1..8d32adf7502d 100644 > --- a/net/mac80211/mlme.c > +++ b/net/mac80211/mlme.c > @@ -5744,6 +5744,7 @@ static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata, > } > > if (link_id != assoc_data->assoc_link_id) { > + ieee80211_sta_init_nss(link_sta); > err = ieee80211_sta_activate_link(sta, link_id); > if (err) > goto out_err; > > Care to test it? > > In general I think we should probably remove the call to > ieee80211_sta_init_nss() from rate_control_rate_init() and call it > explicitly wherever needed, since with MLO we require offloaded rate > control and rate_control_rate_init() doesn't really do anything (except > this for the deflink, which is then questionable) Yes, we can test this. Thanks, Ben
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 4dc1def69548..b69d0eb250d6 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -39,6 +39,27 @@ void rate_control_rate_init(struct sta_info *sta) ieee80211_sta_init_nss(&sta->deflink); + pr_info("rate: sta-init-nss called from rate-control-rate-init, nss: %d\n", + sta->deflink.pub->rx_nss); + + { + int z; + + for (z = 0; z<IEEE80211_MLD_MAX_NUM_LINKS; z++) { + struct link_sta_info *ls = + rcu_dereference_protected(sta->link[z], + lockdep_is_held(&local->hw.wiphy->mtx)); + if (!ls) + continue; + if (ls == &sta->deflink) + continue; + + pr_info("rate: rate-control-rate-init, setting other link rx_nss from: %d to %d link-id: %d\n", + ls->pub->rx_nss, sta->deflink.pub->rx_nss, z); + ls->pub->rx_nss = sta->deflink.pub->rx_nss; + } + } + if (!ref) return;