| Message ID | 20240130043225.942202-4-quic_adisi@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| 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: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 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
On Tue, 2024-01-30 at 19:20 +0530, Aditya Kumar Singh wrote: > > > > 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. > > Any other comments? So that I can address those and send v8 > Ah no, I didn't respond sorry, but that explanation makes sense. johannes
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index dd237081dbe3..a5d510932cbe 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -3586,20 +3586,21 @@ void ieee80211_channel_switch_disconnect(struct ieee80211_vif *vif, bool block_t } EXPORT_SYMBOL(ieee80211_channel_switch_disconnect); -static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata, +static int ieee80211_set_after_csa_beacon(struct ieee80211_link_data *link_data, u64 *changed) { + struct ieee80211_sub_if_data *sdata = link_data->sdata; int err; switch (sdata->vif.type) { case NL80211_IFTYPE_AP: - if (!sdata->deflink.u.ap.next_beacon) + if (!link_data->u.ap.next_beacon) return -EINVAL; - err = ieee80211_assign_beacon(sdata, &sdata->deflink, - sdata->deflink.u.ap.next_beacon, + err = ieee80211_assign_beacon(sdata, link_data, + link_data->u.ap.next_beacon, NULL, NULL, changed); - ieee80211_free_next_beacon(&sdata->deflink); + ieee80211_free_next_beacon(link_data); if (err < 0) return err; @@ -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); if (err) return err; @@ -3717,18 +3718,19 @@ void ieee80211_csa_finalize_work(struct wiphy *wiphy, struct wiphy_work *work) ieee80211_csa_finalize(link); } -static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata, +static int ieee80211_set_csa_beacon(struct ieee80211_link_data *link_data, struct cfg80211_csa_settings *params, u64 *changed) { + struct ieee80211_sub_if_data *sdata = link_data->sdata; struct ieee80211_csa_settings csa = {}; int err; switch (sdata->vif.type) { case NL80211_IFTYPE_AP: - sdata->deflink.u.ap.next_beacon = + link_data->u.ap.next_beacon = cfg80211_beacon_dup(¶ms->beacon_after); - if (!sdata->deflink.u.ap.next_beacon) + if (!link_data->u.ap.next_beacon) return -ENOMEM; /* @@ -3754,7 +3756,7 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata, IEEE80211_MAX_CNTDWN_COUNTERS_NUM) || (params->n_counter_offsets_presp > IEEE80211_MAX_CNTDWN_COUNTERS_NUM)) { - ieee80211_free_next_beacon(&sdata->deflink); + ieee80211_free_next_beacon(link_data); return -EINVAL; } @@ -3764,11 +3766,11 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata, csa.n_counter_offsets_presp = params->n_counter_offsets_presp; csa.count = params->count; - err = ieee80211_assign_beacon(sdata, &sdata->deflink, + err = ieee80211_assign_beacon(sdata, link_data, ¶ms->beacon_csa, &csa, NULL, changed); if (err < 0) { - ieee80211_free_next_beacon(&sdata->deflink); + ieee80211_free_next_beacon(link_data); return err; } @@ -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); if (err) { ieee80211_link_unreserve_chanctx(&sdata->deflink); goto out;
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. Add changes for the same. Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- net/mac80211/cfg.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)