diff mbox series

[RFC,3/6] wifi: mac80211: notify driver about per-radio monitor enabled state

Message ID b75992cb05270eb72fc9eaac3313ac3236701657.1722885720.git-series.nbd@nbd.name
State New
Headers show
Series wifi: cfg80211/mac80211: improve support for multiple radios | expand

Commit Message

Felix Fietkau Aug. 5, 2024, 7:23 p.m. UTC
This allows monitoring on one or more radios while minimizing performance
impact on the others.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 include/net/mac80211.h     |  3 +++
 net/mac80211/ieee80211_i.h |  6 ++++++
 net/mac80211/iface.c       | 26 +++++++++++++++++++++++++-
 net/mac80211/main.c        | 12 ++++++++++++
 4 files changed, 46 insertions(+), 1 deletion(-)

Comments

Johannes Berg Aug. 23, 2024, 10:16 a.m. UTC | #1
On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote:
> This allows monitoring on one or more radios while minimizing performance
> impact on the others.

But why are you doing it this way? You could already solve this entirely
with the driver by setting WANT_MONITOR_VIF and dealing with that, I'd
think? At least after this series.

I generally don't like hw->conf, it just hasn't really matched reality
for years with all kinds of new concurrency capabilities. At the very
least you'd have to write more text here to convince me that we want to
add something to it ... :)

johannes
Johannes Berg Sept. 17, 2024, 8:13 a.m. UTC | #2
On Fri, 2024-08-23 at 13:26 +0200, Felix Fietkau wrote:
> 
> > On 23. Aug 2024, at 12:16, Johannes Berg <johannes@sipsolutions.net> wrote:
> > 
> > On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote:
> > > This allows monitoring on one or more radios while minimizing performance
> > > impact on the others.
> > 
> > But why are you doing it this way? You could already solve this entirely
> > with the driver by setting WANT_MONITOR_VIF and dealing with that, I'd
> > think? At least after this series.
> > 
> > I generally don't like hw->conf, it just hasn't really matched reality
> > for years with all kinds of new concurrency capabilities. At the very
> > least you'd have to write more text here to convince me that we want to
> > add something to it ... :)
> 
> I really don’t see how WANT_MONITOR_VIF helps. It seems completely unrelated to me, since it only creates a single driver visible vif, if there are no non-monitor vifs on the phy.

Well, it's true that it only creates one towards the driver, but that
one vif can also only be bound to a single channel context, and
therefore a single radio.

If we actually want(ed) to support monitoring on different radios
simultaneously we'd have to change mac80211 quite a bit, and probably
introduce multiple virtual monitor interfaces. Internally, we _always_
have it now, to be able to bind a channel context, so we'd actually need
multiple - one for each possible parallel channel.

So that's why I think having WANT_MONITOR_VIF helps - you can assume
today that only one chanctx can be used for monitoring, and once you
have the monitor vif in hand, you know which one it is. Therefore you
know which radio it is, and can adjust your offloads/etc. accordingly.

> I want to be able to control, which radios I want to capture on, regardless of which vifs are already active on the same phy.

Sure.

> A global monitor enable/disable status means that I can’t prevent monitor-incompatible offloads from being disabled on radios that I’m not monitoring on.
> 

Yeah I'd just say don't use that state, but the presence of the monitor
vif, and you can figure out which radio it's present on.

johannes
Ben Greear Sept. 17, 2024, 3:26 p.m. UTC | #3
On 9/17/24 01:13, Johannes Berg wrote:
> On Fri, 2024-08-23 at 13:26 +0200, Felix Fietkau wrote:
>>
>>> On 23. Aug 2024, at 12:16, Johannes Berg <johannes@sipsolutions.net> wrote:
>>>
>>> On Mon, 2024-08-05 at 21:23 +0200, Felix Fietkau wrote:
>>>> This allows monitoring on one or more radios while minimizing performance
>>>> impact on the others.
>>>
>>> But why are you doing it this way? You could already solve this entirely
>>> with the driver by setting WANT_MONITOR_VIF and dealing with that, I'd
>>> think? At least after this series.
>>>
>>> I generally don't like hw->conf, it just hasn't really matched reality
>>> for years with all kinds of new concurrency capabilities. At the very
>>> least you'd have to write more text here to convince me that we want to
>>> add something to it ... :)
>>
>> I really don’t see how WANT_MONITOR_VIF helps. It seems completely unrelated to me, since it only creates a single driver visible vif, if there are no non-monitor vifs on the phy.
> 
> Well, it's true that it only creates one towards the driver, but that
> one vif can also only be bound to a single channel context, and
> therefore a single radio.
> 
> If we actually want(ed) to support monitoring on different radios
> simultaneously we'd have to change mac80211 quite a bit, and probably
> introduce multiple virtual monitor interfaces. Internally, we _always_
> have it now, to be able to bind a channel context, so we'd actually need
> multiple - one for each possible parallel channel.

I definitely want to be able to sniff on the individual underlying channels,
and not have the traffic from the different underlying radios mixed (unless
specifically requesting that feature somehow).

 From user-space perspective, having multiple monitor vifs seems the way
to do this, as that is how it works now.

Thanks,
Ben

> So that's why I think having WANT_MONITOR_VIF helps - you can assume
> today that only one chanctx can be used for monitoring, and once you
> have the monitor vif in hand, you know which one it is. Therefore you
> know which radio it is, and can adjust your offloads/etc. accordingly.
> 
>> I want to be able to control, which radios I want to capture on, regardless of which vifs are already active on the same phy.
> 
> Sure.
> 
>> A global monitor enable/disable status means that I can’t prevent monitor-incompatible offloads from being disabled on radios that I’m not monitoring on.
>>
> 
> Yeah I'd just say don't use that state, but the presence of the monitor
> vif, and you can figure out which radio it's present on.
> 
> johannes
>
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9618c82262e3..7bee2d912efe 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1748,6 +1748,8 @@  enum ieee80211_smps_mode {
  *
  * @flags: configuration flags defined above
  *
+ * @monitor_radios: bitmask of radios with monitor mode enabled.
+ *
  * @listen_interval: listen interval in units of beacon interval
  * @ps_dtim_period: The DTIM period of the AP we're connected to, for use
  *	in power saving. Power saving will not be enabled until a beacon
@@ -1777,6 +1779,7 @@  enum ieee80211_smps_mode {
  */
 struct ieee80211_conf {
 	u32 flags;
+	u32 monitor_radios;
 	int power_level, dynamic_ps_timeout;
 
 	u16 listen_interval;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 304cce0b771d..3be9f8149117 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1330,6 +1330,10 @@  enum mac80211_scan_state {
 
 DECLARE_STATIC_KEY_FALSE(aql_disable);
 
+struct ieee80211_radio_data {
+	u32 monitors;
+};
+
 struct ieee80211_local {
 	/* embed the driver visible part.
 	 * don't cast (use the static inlines below), but we keep
@@ -1613,6 +1617,8 @@  struct ieee80211_local {
 	u8 ext_capa[8];
 
 	bool wbrf_supported;
+
+	struct ieee80211_radio_data *radio_data;
 };
 
 static inline struct ieee80211_sub_if_data *
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 56fed4f69427..4db867ae97f0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -601,6 +601,18 @@  static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata, bool going_do
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
+		for (i = 0; i < local->hw.wiphy->n_radio; i++) {
+			if (!(sdata->wdev.radio_mask & BIT(i)))
+				continue;
+
+			local->radio_data[i].monitors--;
+			if (local->radio_data[i].monitors)
+				continue;
+
+			local->hw.conf.monitor_radios &= ~BIT(i);
+			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
+		}
+
 		ieee80211_adjust_monitor_flags(sdata, -1);
 		break;
 	case NL80211_IFTYPE_NAN:
@@ -1214,7 +1226,7 @@  int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
 	struct net_device *dev = wdev->netdev;
 	struct ieee80211_local *local = sdata->local;
 	u64 changed = 0;
-	int res;
+	int i, res;
 	u32 hw_reconf_flags = 0;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
@@ -1330,6 +1342,18 @@  int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
 			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
 		}
 
+		for (i = 0; i < local->hw.wiphy->n_radio; i++) {
+			if (!(sdata->wdev.radio_mask & BIT(i)))
+				continue;
+
+			local->radio_data[i].monitors++;
+			if (local->radio_data[i].monitors > 1)
+				continue;
+
+			local->hw.conf.monitor_radios |= BIT(i);
+			hw_reconf_flags |= IEEE80211_CONF_CHANGE_MONITOR;
+		}
+
 		ieee80211_adjust_monitor_flags(sdata, 1);
 		ieee80211_configure_filter(local);
 		ieee80211_recalc_offload(local);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 6abf85a58133..2ce771744ea8 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1348,6 +1348,16 @@  int ieee80211_register_hw(struct ieee80211_hw *hw)
 	if (!local->int_scan_req)
 		return -ENOMEM;
 
+	if (hw->wiphy->n_radio) {
+		local->radio_data = kcalloc(hw->wiphy->n_radio,
+					    sizeof(*local->radio_data),
+					    GFP_KERNEL);
+		if (!local->radio_data) {
+			result = -ENOMEM;
+			goto fail_workqueue;
+		}
+	}
+
 	eth_broadcast_addr(local->int_scan_req->bssid);
 
 	for (band = 0; band < NUM_NL80211_BANDS; band++) {
@@ -1642,6 +1652,7 @@  int ieee80211_register_hw(struct ieee80211_hw *hw)
 		local->wiphy_ciphers_allocated = false;
 	}
 	kfree(local->int_scan_req);
+	kfree(local->radio_data);
 	return result;
 }
 EXPORT_SYMBOL(ieee80211_register_hw);
@@ -1694,6 +1705,7 @@  void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 	destroy_workqueue(local->workqueue);
 	ieee80211_led_exit(local);
 	kfree(local->int_scan_req);
+	kfree(local->radio_data);
 }
 EXPORT_SYMBOL(ieee80211_unregister_hw);