diff mbox series

Report guard interval and dual carrier modulation.

Message ID 20230107072917.3838409-1-druth@chromium.org
State Accepted
Headers show
Series Report guard interval and dual carrier modulation. | expand

Commit Message

David Ruth Jan. 7, 2023, 7:29 a.m. UTC 
Allows collecting and exposing more information about the station's
current connection from the kernel to the connection manager.

* Add an enum to represent guard interval settings to driver.h.
* Add fields for storing guard interval and dual carrier modulation
  information into the hostap_sta_driver_data struct.
* Add bitmask values indicating the presence of fields.
  * STA_DRV_DATA_TX_HE_DCM
  * STA_DRV_DATA_RX_HE_DCM
  * STA_DRV_DATA_TX_HE_GI
  * STA_DRV_DATA_RX_HE_GI
* Retrieve NL80211_RATE_INFO_HE_GI and NL80211_RATE_INFO_HE_DCM in
  get_sta_handler, and set appropriate flags.
* Propagate the values over D-Bus.

Signed-off-by: David Ruth <druth@chromium.org>
---

 src/drivers/driver.h                   | 13 +++++++
 src/drivers/driver_nl80211.c           | 48 ++++++++++++++++++++++++--
 wpa_supplicant/dbus/dbus_new_helpers.c | 12 +++++++
 3 files changed, 71 insertions(+), 2 deletions(-)

Comments

Jouni Malinen Feb. 21, 2023, 12:16 p.m. UTC | #1 
On Sat, Jan 07, 2023 at 07:29:17AM +0000, David Ruth wrote:
> Allows collecting and exposing more information about the station's
> current connection from the kernel to the connection manager.
> 
> * Add an enum to represent guard interval settings to driver.h.
> * Add fields for storing guard interval and dual carrier modulation
>   information into the hostap_sta_driver_data struct.
> * Add bitmask values indicating the presence of fields.
>   * STA_DRV_DATA_TX_HE_DCM
>   * STA_DRV_DATA_RX_HE_DCM
>   * STA_DRV_DATA_TX_HE_GI
>   * STA_DRV_DATA_RX_HE_GI
> * Retrieve NL80211_RATE_INFO_HE_GI and NL80211_RATE_INFO_HE_DCM in
>   get_sta_handler, and set appropriate flags.

Thanks, I applied the nl80211 related parts now, but as far as the D-Bus
interface is concerned:

> * Propagate the values over D-Bus.

I have some open questions on this below:

> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> +enum guard_interval {
> +	GUARD_INTERVAL_0_4 = 1,
> +	GUARD_INTERVAL_0_8 = 2,
> +	GUARD_INTERVAL_1_6 = 3,
> +	GUARD_INTERVAL_3_2 = 4,
> +};

These feels like an implementation internal definition where those
values might change.. At minimum, this would need a comment if there is
requirement for the values to be maintained as-is.

> diff --git a/wpa_supplicant/dbus/dbus_new_helpers.c b/wpa_supplicant/dbus/dbus_new_helpers.c
> @@ -1146,6 +1146,18 @@ int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
> +	     !wpa_dbus_dict_append_uint32(&iter_dict, "rx-guard-interval",
> +					  si->data.rx_guard_interval)) ||
> +	    (si->data.tx_guard_interval &&
> +	     !wpa_dbus_dict_append_uint32(&iter_dict, "tx-guard-interval",
> +					  si->data.tx_guard_interval)) ||

This would expose those enum guard_interval values as-is over the D-Bus
interface. Is that really appropriate? How would the application at the
other end know what these values 1-4 mean? The actual values 0.4, 0.8,
1.6, 3.2 might make more sense there or at least this mapping would need
to be clearly documented somewhere.

> +	    (si->data.rx_dcm &&
> +	     !wpa_dbus_dict_append_uint32(&iter_dict, "rx-dcm",
> +					  si->data.rx_dcm)) ||
> +	    (si->data.tx_dcm &&
> +	     !wpa_dbus_dict_append_uint32(&iter_dict, "tx-dcm",
> +					  si->data.tx_dcm)) ||

These might be fine as-is, but I'll note that the values from the kernel
are 0 or 1, so there might be cleaner encoding options for these than
uint32.
diff mbox series

Patch

diff --git a/src/drivers/driver.h b/src/drivers/driver.h
index cb27282aa..12e2e8c3a 100644
--- a/src/drivers/driver.h
+++ b/src/drivers/driver.h
@@ -2317,6 +2317,13 @@  struct wpa_driver_capa {
 
 struct hostapd_data;
 
+enum guard_interval {
+	GUARD_INTERVAL_0_4 = 1,
+	GUARD_INTERVAL_0_8 = 2,
+	GUARD_INTERVAL_1_6 = 3,
+	GUARD_INTERVAL_3_2 = 4,
+};
+
 #define STA_DRV_DATA_TX_MCS BIT(0)
 #define STA_DRV_DATA_RX_MCS BIT(1)
 #define STA_DRV_DATA_TX_VHT_MCS BIT(2)
@@ -2331,6 +2338,10 @@  struct hostapd_data;
 #define STA_DRV_DATA_RX_HE_MCS BIT(11)
 #define STA_DRV_DATA_TX_HE_NSS BIT(12)
 #define STA_DRV_DATA_RX_HE_NSS BIT(13)
+#define STA_DRV_DATA_TX_HE_DCM BIT(14)
+#define STA_DRV_DATA_RX_HE_DCM BIT(15)
+#define STA_DRV_DATA_TX_HE_GI BIT(16)
+#define STA_DRV_DATA_RX_HE_GI BIT(17)
 
 struct hostap_sta_driver_data {
 	unsigned long rx_packets, tx_packets;
@@ -2368,6 +2379,8 @@  struct hostap_sta_driver_data {
 	s8 avg_signal; /* dBm */
 	s8 avg_beacon_signal; /* dBm */
 	s8 avg_ack_signal; /* dBm */
+	enum guard_interval rx_guard_interval, tx_guard_interval;
+	u8 rx_dcm, tx_dcm;
 };
 
 struct hostapd_sta_add_params {
diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 3f6c05c09..35c10cbb8 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -7380,6 +7380,8 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 		[NL80211_RATE_INFO_VHT_NSS] = { .type = NLA_U8 },
 		[NL80211_RATE_INFO_HE_MCS] = { .type = NLA_U8 },
 		[NL80211_RATE_INFO_HE_NSS] = { .type = NLA_U8 },
+		[NL80211_RATE_INFO_HE_GI] = { .type = NLA_U8 },
+		[NL80211_RATE_INFO_HE_DCM] = { .type = NLA_U8 },
 	};
 
 	nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
@@ -7503,8 +7505,10 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 				nla_get_u8(rate[NL80211_RATE_INFO_VHT_MCS]);
 			data->flags |= STA_DRV_DATA_TX_VHT_MCS;
 		}
-		if (rate[NL80211_RATE_INFO_SHORT_GI])
+		if (rate[NL80211_RATE_INFO_SHORT_GI]) {
+			data->tx_guard_interval = GUARD_INTERVAL_0_4;
 			data->flags |= STA_DRV_DATA_TX_SHORT_GI;
+		}
 		if (rate[NL80211_RATE_INFO_VHT_NSS]) {
 			data->tx_vht_nss =
 				nla_get_u8(rate[NL80211_RATE_INFO_VHT_NSS]);
@@ -7520,6 +7524,25 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 				nla_get_u8(rate[NL80211_RATE_INFO_HE_NSS]);
 			data->flags |= STA_DRV_DATA_TX_HE_NSS;
 		}
+		if (rate[NL80211_RATE_INFO_HE_GI]) {
+			switch (nla_get_u8(rate[NL80211_RATE_INFO_HE_GI])) {
+				case NL80211_RATE_INFO_HE_GI_0_8:
+					data->tx_guard_interval = GUARD_INTERVAL_0_8;
+					break;
+				case NL80211_RATE_INFO_HE_GI_1_6:
+					data->tx_guard_interval = GUARD_INTERVAL_1_6;
+					break;
+				case NL80211_RATE_INFO_HE_GI_3_2:
+					data->tx_guard_interval = GUARD_INTERVAL_3_2;
+					break;
+			}
+			data->flags |= STA_DRV_DATA_TX_HE_GI;
+		}
+		if (rate[NL80211_RATE_INFO_HE_DCM]) {
+			data->tx_dcm =
+				nla_get_u8(rate[NL80211_RATE_INFO_HE_DCM]);
+			data->flags |= STA_DRV_DATA_TX_HE_DCM;
+		}
 	}
 
 	if (stats[NL80211_STA_INFO_RX_BITRATE] &&
@@ -7546,8 +7569,10 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 				nla_get_u8(rate[NL80211_RATE_INFO_VHT_MCS]);
 			data->flags |= STA_DRV_DATA_RX_VHT_MCS;
 		}
-		if (rate[NL80211_RATE_INFO_SHORT_GI])
+		if (rate[NL80211_RATE_INFO_SHORT_GI]) {
+			data->rx_guard_interval = GUARD_INTERVAL_0_4;
 			data->flags |= STA_DRV_DATA_RX_SHORT_GI;
+		}
 		if (rate[NL80211_RATE_INFO_VHT_NSS]) {
 			data->rx_vht_nss =
 				nla_get_u8(rate[NL80211_RATE_INFO_VHT_NSS]);
@@ -7563,6 +7588,25 @@  static int get_sta_handler(struct nl_msg *msg, void *arg)
 				nla_get_u8(rate[NL80211_RATE_INFO_HE_NSS]);
 			data->flags |= STA_DRV_DATA_RX_HE_NSS;
 		}
+		if (rate[NL80211_RATE_INFO_HE_GI]) {
+			switch (nla_get_u8(rate[NL80211_RATE_INFO_HE_GI])) {
+				case NL80211_RATE_INFO_HE_GI_0_8:
+					data->rx_guard_interval = GUARD_INTERVAL_0_8;
+					break;
+				case NL80211_RATE_INFO_HE_GI_1_6:
+					data->rx_guard_interval = GUARD_INTERVAL_1_6;
+					break;
+				case NL80211_RATE_INFO_HE_GI_3_2:
+					data->rx_guard_interval = GUARD_INTERVAL_3_2;
+					break;
+			}
+			data->flags |= STA_DRV_DATA_RX_HE_GI;
+		}
+		if (rate[NL80211_RATE_INFO_HE_DCM]) {
+			data->rx_dcm =
+				nla_get_u8(rate[NL80211_RATE_INFO_HE_DCM]);
+			data->flags |= STA_DRV_DATA_RX_HE_DCM;
+		}
 	}
 
 	if (stats[NL80211_STA_INFO_TID_STATS])
diff --git a/wpa_supplicant/dbus/dbus_new_helpers.c b/wpa_supplicant/dbus/dbus_new_helpers.c
index e21f9129f..b058115fa 100644
--- a/wpa_supplicant/dbus/dbus_new_helpers.c
+++ b/wpa_supplicant/dbus/dbus_new_helpers.c
@@ -1146,6 +1146,18 @@  int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
 	    (si->data.avg_ack_signal &&
 	     !wpa_dbus_dict_append_int32(&iter_dict, "avg-ack-rssi",
 					 si->data.avg_ack_signal)) ||
+	    (si->data.rx_guard_interval &&
+	     !wpa_dbus_dict_append_uint32(&iter_dict, "rx-guard-interval",
+					  si->data.rx_guard_interval)) ||
+	    (si->data.tx_guard_interval &&
+	     !wpa_dbus_dict_append_uint32(&iter_dict, "tx-guard-interval",
+					  si->data.tx_guard_interval)) ||
+	    (si->data.rx_dcm &&
+	     !wpa_dbus_dict_append_uint32(&iter_dict, "rx-dcm",
+					  si->data.rx_dcm)) ||
+	    (si->data.tx_dcm &&
+	     !wpa_dbus_dict_append_uint32(&iter_dict, "tx-dcm",
+					  si->data.tx_dcm)) ||
 	    !wpa_dbus_dict_close_write(&variant_iter, &iter_dict) ||
 	    !dbus_message_iter_close_container(iter, &variant_iter))
 		return -ENOMEM;