| Message ID | 20250514-mlo-dfs-acs-v1-2-74e42a5583c6@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: cfg80211/mac80211: Handle simultaneous scan and DFS operations on different radios | expand |
On Wed, 2025-05-14 at 16:58 +0530, Raj Kumar Bhagat wrote: > Currently, in multi-radio wiphy cases, if one radio is operating on a DFS > channel, -EBUSY is returned even when a scan is requested on a different > radio. Because of this, an MLD AP with one radio (link) on a DFS channel > and Automatic Channel Selection (ACS) on another radio (link) cannot be > brought up. > > In multi-radio wiphy cases, multiple radios are grouped under a single > wiphy. Hence, if a radio is operating on a DFS channel and a scan is > requested on a different radio of the same wiphy, the scan can be allowed > simultaneously without impacting the DFS operations. > > Add logic to check the underlying radio used for the requested scan. If the > radio on which DFS is already running is not being used, allow the scan > operation; otherwise, return -EBUSY. So while I agree in principle, I think this needs to be more carefully constructed because it relies on an unstated (?) assumption that each radio is going to ever be used for scanning on a certain band. That seems to make sense, and a radio will certainly only ever be able to _operate_ on the frequencies listed for it (due to chanctx etc.), but is it really true that it will never be able to operate at all on other frequencies? I'm not sure I find the notion of e.g. having a 5 and 6 GHz radio that are used for operating on those bands, but being able to scan 5 GHz channels using the 6 GHz radio completely unimaginable? Maybe it is though and I'm just overly paranoid? We could also just leave that up to the drivers, of course, but then I think we should _state_ this assumption somewhere in the docs/header file(s)? > +bool ieee80211_is_radio_idx_in_scan_req(struct wiphy *wiphy, > + struct cfg80211_scan_request *scan_req, > + int radio_idx) > +{ > + struct ieee80211_channel *chan; > + int i, chan_radio_idx; > + > + if (!scan_req) > + return false; That seems overly paranoid, or maybe it should be WARN_ON()? I mean, asking something about a scan request and then not giving one is just the wrong thing to do in the first place, no? And if you're going to be paranoid then this probably shouldn't be called with an invalid/negative radio_idx either :) > + for (i = 0; i < scan_req->n_channels; i++) { > + chan = scan_req->channels[i]; > + chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); > + /* > + * Skip channels with an invalid radio index and continue > + * checking. If any channel in the scan request matches the > + * given radio index, return true. > + */ > + if (chan_radio_idx < 0) > + continue; This seems ... wrong? If there's a channel in the scan request that didn't map to _any_ radio then how are we even scanning there? And the comment seems even stranger, why would we _want_ to ignore it (which it conveniently doesn't answer)? johannes
On 5/16/2025 1:31 PM, Johannes Berg wrote: > On Wed, 2025-05-14 at 16:58 +0530, Raj Kumar Bhagat wrote: >> Currently, in multi-radio wiphy cases, if one radio is operating on a DFS >> channel, -EBUSY is returned even when a scan is requested on a different >> radio. Because of this, an MLD AP with one radio (link) on a DFS channel >> and Automatic Channel Selection (ACS) on another radio (link) cannot be >> brought up. >> >> In multi-radio wiphy cases, multiple radios are grouped under a single >> wiphy. Hence, if a radio is operating on a DFS channel and a scan is >> requested on a different radio of the same wiphy, the scan can be allowed >> simultaneously without impacting the DFS operations. >> >> Add logic to check the underlying radio used for the requested scan. If the >> radio on which DFS is already running is not being used, allow the scan >> operation; otherwise, return -EBUSY. > > So while I agree in principle, I think this needs to be more carefully > constructed because it relies on an unstated (?) assumption that each > radio is going to ever be used for scanning on a certain band. That > seems to make sense, and a radio will certainly only ever be able to > _operate_ on the frequencies listed for it (due to chanctx etc.), but is > it really true that it will never be able to operate at all on other > frequencies? > I'm not sure If I fully understood the this comment. This patch assumes that multiple radios are grouped under a single wiphy. Each radio has its own list of frequencies it can scan, and there is no overlap in frequencies between any two radios within the same wiphy. If this assumption holds, then if one radio is operating on a DFS channel and a new scan request does not include any frequencies from that radio's list, the scan should be allowed—since the DFS radio wouldn't be involved in handling that scan request. > I'm not sure I find the notion of e.g. having a 5 and 6 GHz radio that > are used for operating on those bands, but being able to scan 5 GHz > channels using the 6 GHz radio completely unimaginable? Maybe it is > though and I'm just overly paranoid? > > We could also just leave that up to the drivers, of course, but then I > think we should _state_ this assumption somewhere in the docs/header > file(s)? > >> +bool ieee80211_is_radio_idx_in_scan_req(struct wiphy *wiphy, >> + struct cfg80211_scan_request *scan_req, >> + int radio_idx) >> +{ >> + struct ieee80211_channel *chan; >> + int i, chan_radio_idx; >> + >> + if (!scan_req) >> + return false; > > That seems overly paranoid, or maybe it should be WARN_ON()? I mean, > asking something about a scan request and then not giving one is just > the wrong thing to do in the first place, no? > > And if you're going to be paranoid then this probably shouldn't be > called with an invalid/negative radio_idx either :) > sure, this function should not be called with NULL scan_req and invalid radio_idx. Better will remove the check: if (!scan_req). > >> + for (i = 0; i < scan_req->n_channels; i++) { >> + chan = scan_req->channels[i]; >> + chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); >> + /* >> + * Skip channels with an invalid radio index and continue >> + * checking. If any channel in the scan request matches the >> + * given radio index, return true. >> + */ >> + if (chan_radio_idx < 0) >> + continue; > > This seems ... wrong? If there's a channel in the scan request that > didn't map to _any_ radio then how are we even scanning there? And the > comment seems even stranger, why would we _want_ to ignore it (which it > conveniently doesn't answer)? > It seems, (chan_radio_idx < 0) should never be true because the chan is taken from the valid scan request. I should remove this check in next version?
On Thu, 2025-05-22 at 16:10 +0530, Raj Kumar Bhagat wrote: > > I'm not sure If I fully understood the this comment. > > This patch assumes that multiple radios are grouped under a single wiphy. > Each radio has its own list of frequencies it can scan, and there is no overlap > in frequencies between any two radios within the same wiphy. Yeah I guess I'm just overly paranoid due to lack of familiarity with all the multi-radio things. > If this assumption holds, then if one radio is operating on a DFS channel and a > new scan request does not include any frequencies from that radio's list, the > scan should be allowed—since the DFS radio wouldn't be involved in handling that > scan request. Agree. > > > + for (i = 0; i < scan_req->n_channels; i++) { > > > + chan = scan_req->channels[i]; > > > + chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); > > > + /* > > > + * Skip channels with an invalid radio index and continue > > > + * checking. If any channel in the scan request matches the > > > + * given radio index, return true. > > > + */ > > > + if (chan_radio_idx < 0) > > > + continue; > > > > This seems ... wrong? If there's a channel in the scan request that > > didn't map to _any_ radio then how are we even scanning there? And the > > comment seems even stranger, why would we _want_ to ignore it (which it > > conveniently doesn't answer)? > > > > It seems, (chan_radio_idx < 0) should never be true because the chan is > taken from the valid scan request. I should remove this check in next version? I'm not sure, why did you add it? Maybe it should be a WARN_ON and abort the whole function? It just doesn't seem right to _ignore_. johannes
On 5/22/2025 4:53 PM, Johannes Berg wrote: >>>> + for (i = 0; i < scan_req->n_channels; i++) { >>>> + chan = scan_req->channels[i]; >>>> + chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); >>>> + /* >>>> + * Skip channels with an invalid radio index and continue >>>> + * checking. If any channel in the scan request matches the >>>> + * given radio index, return true. >>>> + */ >>>> + if (chan_radio_idx < 0) >>>> + continue; >>> This seems ... wrong? If there's a channel in the scan request that >>> didn't map to _any_ radio then how are we even scanning there? And the >>> comment seems even stranger, why would we _want_ to ignore it (which it >>> conveniently doesn't answer)? >>> >> It seems, (chan_radio_idx < 0) should never be true because the chan is >> taken from the valid scan request. I should remove this check in next version? > I'm not sure, why did you add it? Maybe it should be a WARN_ON and abort > the whole function? It just doesn't seem right to _ignore_. I initially added the check as a precautionary measure. In the next version, I’ll replace it with a WARN_ON() to flag the unexpected condition. As a conservative approach, will return true in that case, assuming the scan request might use the specified radio_idx to safely abort the function.
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index 3aaf5abf1acc13008a0472672c826d495c80407c..94c83f58e23d0666af881e5e0c981cca17231bfd 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -644,15 +644,39 @@ ieee80211_find_chanctx(struct ieee80211_local *local, return NULL; } -bool ieee80211_is_radar_required(struct ieee80211_local *local) +bool ieee80211_is_radar_required(struct ieee80211_local *local, + struct cfg80211_scan_request *req) { + struct wiphy *wiphy = local->hw.wiphy; struct ieee80211_link_data *link; + struct ieee80211_channel *chan; + int radio_idx; lockdep_assert_wiphy(local->hw.wiphy); + if (!req) + return false; + for_each_sdata_link(local, link) { - if (link->radar_required) - return true; + if (link->radar_required) { + if (wiphy->n_radio < 2) + return true; + + chan = link->conf->chanreq.oper.chan; + radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); + /* + * The radio index (radio_idx) is expected to be valid, + * as it's derived from a channel tied to a link. If + * it's invalid (i.e., negative), return true to avoid + * potential issues with radar-sensitive operations. + */ + if (radio_idx < 0) + return true; + + if (ieee80211_is_radio_idx_in_scan_req(wiphy, req, + radio_idx)) + return true; + } } return false; diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 30809f0b35f73e77b05a6f802011a64900e1532f..a5050e3025b795a1c2a24f1a370ef7e05c2fde6a 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -2712,8 +2712,12 @@ void ieee80211_recalc_chanctx_min_def(struct ieee80211_local *local, struct ieee80211_chanctx *ctx, struct ieee80211_link_data *rsvd_for, bool check_reserved); -bool ieee80211_is_radar_required(struct ieee80211_local *local); +bool ieee80211_is_radar_required(struct ieee80211_local *local, + struct cfg80211_scan_request *req); +bool ieee80211_is_radio_idx_in_scan_req(struct wiphy *wiphy, + struct cfg80211_scan_request *scan_req, + int radio_idx); void ieee80211_dfs_cac_timer_work(struct wiphy *wiphy, struct wiphy_work *work); void ieee80211_dfs_cac_cancel(struct ieee80211_local *local, struct ieee80211_chanctx *chanctx); diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c index 2b9abc27462eb5a41f47523d1653c7f27137588d..686d9f6e9b527acaa3500e8d99d5ca513bb1025e 100644 --- a/net/mac80211/offchannel.c +++ b/net/mac80211/offchannel.c @@ -567,6 +567,7 @@ static int ieee80211_start_roc_work(struct ieee80211_local *local, { struct ieee80211_roc_work *roc, *tmp; bool queued = false, combine_started = true; + struct cfg80211_scan_request *req; int ret; lockdep_assert_wiphy(local->hw.wiphy); @@ -612,9 +613,11 @@ static int ieee80211_start_roc_work(struct ieee80211_local *local, roc->mgmt_tx_cookie = *cookie; } + req = wiphy_dereference(local->hw.wiphy, local->scan_req); + /* if there's no need to queue, handle it immediately */ if (list_empty(&local->roc_list) && - !local->scanning && !ieee80211_is_radar_required(local)) { + !local->scanning && !ieee80211_is_radar_required(local, req)) { /* if not HW assist, just queue & schedule work */ if (!local->ops->remain_on_channel) { list_add_tail(&roc->list, &local->roc_list); diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index cb707907188585d6874bf290874bdb0ca33bb399..6947a7ede51c990af97ed875017c3e4d351f1cd9 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -571,7 +571,8 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local, return 0; } -static bool __ieee80211_can_leave_ch(struct ieee80211_sub_if_data *sdata) +static bool __ieee80211_can_leave_ch(struct ieee80211_sub_if_data *sdata, + struct cfg80211_scan_request *req) { struct ieee80211_local *local = sdata->local; struct ieee80211_sub_if_data *sdata_iter; @@ -579,7 +580,7 @@ static bool __ieee80211_can_leave_ch(struct ieee80211_sub_if_data *sdata) lockdep_assert_wiphy(local->hw.wiphy); - if (!ieee80211_is_radar_required(local)) + if (!ieee80211_is_radar_required(local, req)) return true; if (!regulatory_pre_cac_allowed(local->hw.wiphy)) @@ -595,9 +596,10 @@ static bool __ieee80211_can_leave_ch(struct ieee80211_sub_if_data *sdata) } static bool ieee80211_can_scan(struct ieee80211_local *local, - struct ieee80211_sub_if_data *sdata) + struct ieee80211_sub_if_data *sdata, + struct cfg80211_scan_request *req) { - if (!__ieee80211_can_leave_ch(sdata)) + if (!__ieee80211_can_leave_ch(sdata, req)) return false; if (!list_empty(&local->roc_list)) @@ -612,15 +614,19 @@ static bool ieee80211_can_scan(struct ieee80211_local *local, void ieee80211_run_deferred_scan(struct ieee80211_local *local) { + struct cfg80211_scan_request *req; + lockdep_assert_wiphy(local->hw.wiphy); if (!local->scan_req || local->scanning) return; + req = wiphy_dereference(local->hw.wiphy, local->scan_req); if (!ieee80211_can_scan(local, rcu_dereference_protected( local->scan_sdata, - lockdep_is_held(&local->hw.wiphy->mtx)))) + lockdep_is_held(&local->hw.wiphy->mtx)), + req)) return; wiphy_delayed_work_queue(local->hw.wiphy, &local->scan_work, @@ -717,10 +723,10 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, !(sdata->vif.active_links & BIT(req->tsf_report_link_id))) return -EINVAL; - if (!__ieee80211_can_leave_ch(sdata)) + if (!__ieee80211_can_leave_ch(sdata, req)) return -EBUSY; - if (!ieee80211_can_scan(local, sdata)) { + if (!ieee80211_can_scan(local, sdata, req)) { /* wait for the work to finish/time out */ rcu_assign_pointer(local->scan_req, req); rcu_assign_pointer(local->scan_sdata, sdata); diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 27d414efa3fd4bda40d2a37b14da6a4aa6bf0a02..0c43216fb7a1b0941349be760c331045d2cf9101 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -3953,6 +3953,34 @@ static u8 ieee80211_chanctx_radar_detect(struct ieee80211_local *local, return radar_detect; } +bool ieee80211_is_radio_idx_in_scan_req(struct wiphy *wiphy, + struct cfg80211_scan_request *scan_req, + int radio_idx) +{ + struct ieee80211_channel *chan; + int i, chan_radio_idx; + + if (!scan_req) + return false; + + for (i = 0; i < scan_req->n_channels; i++) { + chan = scan_req->channels[i]; + chan_radio_idx = cfg80211_get_radio_idx_by_chan(wiphy, chan); + /* + * Skip channels with an invalid radio index and continue + * checking. If any channel in the scan request matches the + * given radio index, return true. + */ + if (chan_radio_idx < 0) + continue; + + if (chan_radio_idx == radio_idx) + return true; + } + + return false; +} + static u32 __ieee80211_get_radio_mask(struct ieee80211_sub_if_data *sdata) {
Currently, in multi-radio wiphy cases, if one radio is operating on a DFS channel, -EBUSY is returned even when a scan is requested on a different radio. Because of this, an MLD AP with one radio (link) on a DFS channel and Automatic Channel Selection (ACS) on another radio (link) cannot be brought up. In multi-radio wiphy cases, multiple radios are grouped under a single wiphy. Hence, if a radio is operating on a DFS channel and a scan is requested on a different radio of the same wiphy, the scan can be allowed simultaneously without impacting the DFS operations. Add logic to check the underlying radio used for the requested scan. If the radio on which DFS is already running is not being used, allow the scan operation; otherwise, return -EBUSY. Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com> --- net/mac80211/chan.c | 30 +++++++++++++++++++++++++++--- net/mac80211/ieee80211_i.h | 6 +++++- net/mac80211/offchannel.c | 5 ++++- net/mac80211/scan.c | 20 +++++++++++++------- net/mac80211/util.c | 28 ++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 12 deletions(-)