On 8/28/24 16:03, Johannes Berg wrote:
On Thu, 2024-07-11 at 09:21 +0530, Aditya Kumar Singh wrote:
* In the worker, go over all the contexts again and for all such context
which is marked with radar detected, add it to a local linked list.
Why is the local list needed first?
As per the current design and hwsim test cases,
NL80211_RADAR_CAC_ABORTED event is expected first and then
NL80211_RADAR_DETECTED event. Now, in the worker, while iterating over
the contexts and upon finding radar marked contexts,
cfg80211_radar_event() can not be called immediately. First
ieee80211_dfs_cac_cancel() needs to be called which in turn will send
CAC_ABORTED event. Now calling this will delete the channel context,
hence a local copy is taken and since we could have multiple contexts
marked with radar, a linked list is used.
+++ b/include/net/mac80211.h
@@ -257,6 +257,7 @@ struct ieee80211_chan_req {
* after RTS/CTS handshake to receive SMPS MIMO transmissions;
* this will always be >= @rx_chains_static.
* @radar_enabled: whether radar detection is enabled on this channel.
+ * @radar_detected: whether radar got detected on this channel.
* @drv_priv: data area for driver use, will always be aligned to
* sizeof(void *), size is determined in hw information.
*/
@@ -269,6 +270,7 @@ struct ieee80211_chanctx_conf {
u8 rx_chains_static, rx_chains_dynamic;
bool radar_enabled;
+ bool radar_detected;
I'm not sure why you're adding this to the driver visible part of the
chanctx, I don't think that really makes sense since setting it must be
done by mac80211 through the API function to trigger all the work?
So you want to add this member in struct ieee80211_chanctx? Yeah we can
move to there as well.
+++ b/net/mac80211/ieee80211_i.h
@@ -1329,6 +1329,11 @@ enum mac80211_scan_state {
DECLARE_STATIC_KEY_FALSE(aql_disable);
+struct radar_info {
+ struct list_head list;
+ struct cfg80211_chan_def chandef;
+};
If it _really_ is needed this can be local to the C file.
Okay sure.
+ INIT_LIST_HEAD(&radar_info_list);
but really, why?
I've answered this above. Please let me know if it is still not clear.
list_for_each_entry(ctx, &local->chanctx_list, list) {
if (ctx->replace_state == IEEE80211_CHANCTX_REPLACES_OTHER)
continue;
- num_chanctx++;
- chandef = ctx->conf.def;
+ if (ctx->conf.radar_detected) {
+ ctx->conf.radar_detected = false;
+ num_chanctx++;
+
+ radar_info = kzalloc(sizeof(*radar_info), GFP_KERNEL);
+ if (WARN_ON(!radar_info))
+ continue;
that clearly shouldn't be a WARN_ON,
If node can not be added, NL event will not be sent to user sapce and
apt action can not be taken. Will that be fine missing out it without
any warning?
+
+ INIT_LIST_HEAD(&radar_info->list);
+ radar_info->chandef = ctx->conf.def;
+ list_add_tail(&radar_info->list, &radar_info_list);
but I also don't really see why you couldn't just call
cfg80211_radar_event() here.
I've answered this as well above. Please let me know if it is still not
clear.
+ if (num_chanctx > 1) {
+ /* XXX: multi-channel is not supported yet in case of non-MLO */
+ if (WARN_ON(!(wiphy->flags & WIPHY_FLAG_SUPPORTS_MLO)))
+ trigger_event = false;
+ }
I don't see how that'd happen in the first place?
This part is keeping the same old code with the new handling with MLO.
Prior to MLO, if num_chanctx is more than 1, it would throw a warn_on so
that's why it will throw now as well if MLO is not supported and
simultaneously two channel contexts are marked as radar detected.
johannes
--
Aditya
