| Message ID | 20240626045216.3754013-10-quic_adisi@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: cfg80211/mac80211: add DFS support for MLO | expand |
On Wed, 2024-06-26 at 10:22 +0530, Aditya Kumar Singh wrote: > If CAC is started and the interface is brought down, currently, CAC > started on link 0 is aborted in function ieee80211_do_stop(). Technically, > by the time execution reaches the above function, all links are already > teared down and hence link 0 (or deflink) alone is handled. Also, since nit: torn down > links are teared down, information on other links is also not available. same here > Hence there is a need to handle this in function ieee80211_free_links(). Not sure I buy that argument though, it feels wrong. Clearly you should be able to stop this from even cfg80211, it tracks whether radar detection is running, so whenever the link is removed, you can also stop it there? Doing it on free seems much too late. johannes
On 6/26/24 18:07, Johannes Berg wrote: > On Wed, 2024-06-26 at 10:22 +0530, Aditya Kumar Singh wrote: >> If CAC is started and the interface is brought down, currently, CAC >> started on link 0 is aborted in function ieee80211_do_stop(). Technically, >> by the time execution reaches the above function, all links are already >> teared down and hence link 0 (or deflink) alone is handled. Also, since > > nit: torn down Okay. > >> links are teared down, information on other links is also not available. > > same here Sure. > >> Hence there is a need to handle this in function ieee80211_free_links(). > > Not sure I buy that argument though, it feels wrong. Clearly you should > be able to stop this from even cfg80211, it tracks whether radar > detection is running, so whenever the link is removed, you can also stop > it there? Doing it on free seems much too late. > Actually if cfg80211_remove_link() is called, then it is aborted during stop_ap handler. So that path is fine only. This was done in situations where properly interface is not brought down or abruptly brought down. For example AP is started and then directly iw wlanX del is issued. Ideally the normal deinit flow should have taken care but just in case if still somehow by passes it then ultimately while freeing the links it will be aborted. May be should I rephrase the commit message?
On Thu, 2024-06-27 at 09:47 +0530, Aditya Kumar Singh wrote: > > Not sure I buy that argument though, it feels wrong. Clearly you should > > be able to stop this from even cfg80211, it tracks whether radar > > detection is running, so whenever the link is removed, you can also stop > > it there? Doing it on free seems much too late. > > > > Actually if cfg80211_remove_link() is called, then it is aborted during > stop_ap handler. So that path is fine only. This was done in situations > where properly interface is not brought down or abruptly brought down. > For example AP is started and then directly iw wlanX del is issued. Still cfg80211 should clean up the link and call stop_ap() etc. If that's broken we need to fix it. > Ideally the normal deinit flow should have taken care but just in case > if still somehow by passes it then ultimately while freeing the links it > will be aborted. May be should I rephrase the commit message? I think we need to fix the cases where it doesn't clean up properly instead. johannes
On 6/28/24 14:30, Johannes Berg wrote: > On Thu, 2024-06-27 at 09:47 +0530, Aditya Kumar Singh wrote: > >>> Not sure I buy that argument though, it feels wrong. Clearly you should >>> be able to stop this from even cfg80211, it tracks whether radar >>> detection is running, so whenever the link is removed, you can also stop >>> it there? Doing it on free seems much too late. >>> >> >> Actually if cfg80211_remove_link() is called, then it is aborted during >> stop_ap handler. So that path is fine only. This was done in situations >> where properly interface is not brought down or abruptly brought down. >> For example AP is started and then directly iw wlanX del is issued. > > Still cfg80211 should clean up the link and call stop_ap() etc. If > that's broken we need to fix it. Got it. > >> Ideally the normal deinit flow should have taken care but just in case >> if still somehow by passes it then ultimately while freeing the links it >> will be aborted. May be should I rephrase the commit message? > > I think we need to fix the cases where it doesn't clean up properly > instead. > Okay. Let me have a look again properly and comeback. - Aditya
diff --git a/net/mac80211/link.c b/net/mac80211/link.c index 8871cc1a0454..6c68887f051c 100644 --- a/net/mac80211/link.c +++ b/net/mac80211/link.c @@ -111,10 +111,32 @@ static void ieee80211_tear_down_links(struct ieee80211_sub_if_data *sdata, static void ieee80211_free_links(struct ieee80211_sub_if_data *sdata, struct link_container **links) { + struct ieee80211_bss_conf *link_conf; + struct ieee80211_link_data *link_data; + struct cfg80211_chan_def chandef; unsigned int link_id; - for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) + for (link_id = 0; link_id < IEEE80211_MLD_MAX_NUM_LINKS; link_id++) { + link_data = &links[link_id]->data; + link_conf = &links[link_id]->conf; + + if (!link_data || !link_conf) + continue; + + wiphy_delayed_work_cancel(sdata->local->hw.wiphy, + &link_data->dfs_cac_timer_work); + + if (sdata->wdev.links[link_id].cac_started) { + chandef = link_conf->chanreq.oper; + ieee80211_link_release_channel(link_data); + cfg80211_cac_event(sdata->dev, + &chandef, + NL80211_RADAR_CAC_ABORTED, + GFP_KERNEL, link_id); + } + kfree(links[link_id]); + } } static int ieee80211_check_dup_link_addrs(struct ieee80211_sub_if_data *sdata)
If CAC is started and the interface is brought down, currently, CAC started on link 0 is aborted in function ieee80211_do_stop(). Technically, by the time execution reaches the above function, all links are already teared down and hence link 0 (or deflink) alone is handled. Also, since links are teared down, information on other links is also not available. Hence there is a need to handle this in function ieee80211_free_links(). Add changes in ieee80211_free_links() function which cancels any scheduled dfs_cac_timer_work on the link and if CAC is started, aborts it. Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- net/mac80211/link.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)