| Message ID | 20240130043225.942202-1-quic_adisi@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: cfg80211/mac80211: add link_id handling in AP channel switch during Multi-Link Operation | expand |
On Tue, 2024-01-30 at 10:02 +0530, Aditya Kumar Singh wrote: > In order to support CSA with MLO, there is a need to handle the functions > ieee80211_set_csa_beacon() and ieee80211_set_after_csa_beacon() on per > link basis. nit: "on a per link" > Add changes for the same. Is that some cultural thing? I always find this phrasing with "for the same" very odd, and would rather say something useful such as "Implement this by passing the correct link data"... but I see this a lot, hence the question. > @@ -3658,7 +3659,7 @@ static int __ieee80211_csa_finalize(struct ieee80211_link_data *link_data) > > sdata->vif.bss_conf.csa_active = false; > > - err = ieee80211_set_after_csa_beacon(sdata, &changed); > + err = ieee80211_set_after_csa_beacon(&sdata->deflink, &changed); weren't you just saying deflink shouldn't be used? > @@ -3928,7 +3930,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, > if (sdata->vif.bss_conf.color_change_active) > ieee80211_color_change_abort(sdata); > > - err = ieee80211_set_csa_beacon(sdata, params, &changed); > + err = ieee80211_set_csa_beacon(&sdata->deflink, params, &changed); dito johannes
On 1/30/24 15:43, Johannes Berg wrote: > On Tue, 2024-01-30 at 10:02 +0530, Aditya Kumar Singh wrote: >> >> + * @link_id: defines the link on which channel switch is expected during >> + * MLO. 0 in case of non-MLO. > > please still use a tab (only) :) > Now I get your point. I was using tab as much as possible and then spaces to align it to ':'. You are suggesting to just use tabs alone right?
On 1/30/24 15:47, Johannes Berg wrote: > On Tue, 2024-01-30 at 10:02 +0530, Aditya Kumar Singh wrote: >> In order to support CSA with MLO, there is a need to handle the functions >> ieee80211_set_csa_beacon() and ieee80211_set_after_csa_beacon() on per >> link basis. > > nit: "on a per link" Will address in next version. Thanks. > >> Add changes for the same. > > Is that some cultural thing? > > I always find this phrasing with "for the same" very odd, and would > rather say something useful such as "Implement this by passing the > correct link data"... but I see this a lot, hence the question. > No idea :). Even I have seen these quite a few times and thought that may be it is fine to use it that way. But I do agree that instead we could put something useful instead. Thanks for pointing it out, I will address this in next version. >> @@ -3658,7 +3659,7 @@ static int __ieee80211_csa_finalize(struct ieee80211_link_data *link_data) >> >> sdata->vif.bss_conf.csa_active = false; >> >> - err = ieee80211_set_after_csa_beacon(sdata, &changed); >> + err = ieee80211_set_after_csa_beacon(&sdata->deflink, &changed); > > weren't you just saying deflink shouldn't be used? > Correct. This patch's aim is to form the base - basically modify the helper function to accept the link data argument. But at this point, CSA is not started on per link basis hence in order to keep CSA working still as it is before this patch, have used deflink here. Functionality wise, this patch is not bringing any change yet. >> @@ -3928,7 +3930,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, >> if (sdata->vif.bss_conf.color_change_active) >> ieee80211_color_change_abort(sdata); >> >> - err = ieee80211_set_csa_beacon(sdata, params, &changed); >> + err = ieee80211_set_csa_beacon(&sdata->deflink, params, &changed); > > dito > Addressed above.
On 1/30/24 15:51, Johannes Berg wrote: > On Tue, 2024-01-30 at 10:02 +0530, Aditya Kumar Singh wrote: >> Currently ieee80211_csa_finish() function finalizes CSA by scheduling a >> finalizing worker using the deflink. With MLO, there is a need to do it >> on a given link basis. >> >> Add changes to pass link ID of the link on which CSA needs to be finalized. > > Why not just directly say > > "Pass the link ID of ..." > > To me at least it seems obvious that a commit makes changes :) > > Anyway .. I'll stop nit-picking too much about your commit messages I > guess, and worst case just rephrase them later. > > (I'm kind of working on this area of CSA too right now, though with a > focus on client.) > >> + /* TODO: MBSSID with MLO changes */ >> if (vif->mbssid_tx_vif == vif) { > > Could you say (even here) more precisely what'd be needed? > We are checking right now "vif->mbssid_tx_vif == vif" for mbssid check. However, with MLO, one single vif can have multiple links and among those 1 or more could be mbssid enabled. Hence, changes are needed to store this mbssid related info on link basis instead of vif directly. >> /* Trigger ieee80211_csa_finish() on the non-transmitting >> * interfaces when channel switch is received on Then later where we are iterating over all interfaces, there also we need to handle it on link basis. Including all these would make this patch a lot bigger. Hence planned to address the whole MBSSID+MLO changes in a separate series overall. >> @@ -3568,7 +3579,7 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif) >> &iter->deflink.csa_finalize_work); > > that still seems to use deflink there, for sure. > Yeah since MBSSID currently would work without MLO, it is safe to use deflink of the non tx vaps while we are iterating.
On 1/30/24 16:13, Aditya Kumar Singh wrote: > On 1/30/24 15:47, Johannes Berg wrote: >> On Tue, 2024-01-30 at 10:02 +0530, Aditya Kumar Singh wrote: >>> In order to support CSA with MLO, there is a need to handle the >>> functions >>> ieee80211_set_csa_beacon() and ieee80211_set_after_csa_beacon() on per >>> link basis. >> >> nit: "on a per link" > > Will address in next version. Thanks. > >> >>> Add changes for the same. >> >> Is that some cultural thing? >> >> I always find this phrasing with "for the same" very odd, and would >> rather say something useful such as "Implement this by passing the >> correct link data"... but I see this a lot, hence the question. >> > > No idea :). Even I have seen these quite a few times and thought that > may be it is fine to use it that way. But I do agree that instead we > could put something useful instead. Thanks for pointing it out, I will > address this in next version. > > >>> @@ -3658,7 +3659,7 @@ static int __ieee80211_csa_finalize(struct >>> ieee80211_link_data *link_data) >>> sdata->vif.bss_conf.csa_active = false; >>> - err = ieee80211_set_after_csa_beacon(sdata, &changed); >>> + err = ieee80211_set_after_csa_beacon(&sdata->deflink, &changed); >> >> weren't you just saying deflink shouldn't be used? >> > > Correct. This patch's aim is to form the base - basically modify the > helper function to accept the link data argument. But at this point, CSA > is not started on per link basis hence in order to keep CSA working > still as it is before this patch, have used deflink here. Functionality > wise, this patch is not bringing any change yet. > > >>> @@ -3928,7 +3930,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, >>> struct net_device *dev, >>> if (sdata->vif.bss_conf.color_change_active) >>> ieee80211_color_change_abort(sdata); >>> - err = ieee80211_set_csa_beacon(sdata, params, &changed); >>> + err = ieee80211_set_csa_beacon(&sdata->deflink, params, &changed); >> >> dito >> > > Addressed above. > Any other comments? So that I can address those and send v8
Currently, during channel switch, deflink (or link_id 0) is always considered. However, with Multi-Link Operation (MLO), there is a need to handle link specific data structures based on the actual operational link_id during channel switch operation. Hence, add support for the same. Non-MLO based operations will use link_id as 0 or deflink member as applicable. While at it, beacon count down now needs to be updated on proper link_id's beacon, do that as well. Aditya Kumar Singh (5): wifi: cfg80211: send link id in channel_switch ops wifi: mac80211: update beacon counters per link basis wifi: mac80211: handle set csa/after_csa beacon on per link basis wifi: mac80211: start and finalize channel switch on link basis wifi: mac80211: add support to call csa_finish on a link --- v7: * fixed proper link id validation in 2/5 and 5/5. (>=) * added comma in trace print in 1/5 v6: * splitted v5 2/3 into v6 3/5, 4/5 and 5/5 * rephrased commit mesage in 1/5 v5: * fixed compilation issue reported by kernel test robot. v4: * fixed compilation issue reported by kernel test robot. * rebased on latest ToT. * moved link_id arguement into csa_params struct in [1/3] v3: * splitted [v2 1/2] into [v3 1/3] and [v3 2/3] having simple cfg80211 changes in 1/3 for easy review. Rest in 2/3 [Johannes] * used wiphy_dereference() instead of sdata_dereference() [Johannes] v2: * reabsed on ToT * removed unwanted locking sequence during cancelling CSA work handler since now locking is moved to wiphy level, that part is uncessary now. --- drivers/net/wireless/ath/ath10k/mac.c | 4 +- drivers/net/wireless/ath/ath10k/wmi.c | 2 +- drivers/net/wireless/ath/ath11k/mac.c | 2 +- drivers/net/wireless/ath/ath11k/wmi.c | 2 +- drivers/net/wireless/ath/ath12k/wmi.c | 2 +- drivers/net/wireless/ath/ath9k/beacon.c | 2 +- .../net/wireless/ath/ath9k/htc_drv_beacon.c | 2 +- .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 6 +- .../wireless/intel/iwlwifi/mvm/time-event.c | 2 +- drivers/net/wireless/mediatek/mt76/mac80211.c | 2 +- .../net/wireless/mediatek/mt76/mt7615/mcu.c | 2 +- .../net/wireless/mediatek/mt76/mt7915/mcu.c | 2 +- .../net/wireless/mediatek/mt76/mt7996/mcu.c | 2 +- .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- drivers/net/wireless/ti/wlcore/event.c | 2 +- drivers/net/wireless/virtual/mac80211_hwsim.c | 2 +- include/net/cfg80211.h | 3 + include/net/mac80211.h | 7 +- net/mac80211/cfg.c | 108 +++++++++++------- net/mac80211/link.c | 2 + net/mac80211/tx.c | 14 ++- net/wireless/nl80211.c | 1 + net/wireless/trace.h | 7 +- 23 files changed, 112 insertions(+), 68 deletions(-) base-commit: 28b3df1fe6ba2cb439ba109f095aa841fef3a54f