[PATCH] Report guard interval and dual carrier modulation.

[Jouni Malinen]

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.
 
-- 
Jouni Malinen                                            PGP id EFC895FA