[PATCH v2] add new macsec offload modes to abstract device-level details

[Jouni Malinen]

On Mon, Feb 12, 2024 at 10:03:05AM +0100, Sabrina Dubroca wrote:
> Currently, users have to guess which offload mode their hardware
> provides, as the kernel doesn't expose this information.
> 
> Add an "on" mode which requires offload (either to mac or phy) and a
> "prefer" mode which allows fallback to SW when offload (either to mac
> or phy) is unavailable.
> 
> Also rename the existing modes to string values to make the config
> file a bit clearer.

Wouldn't that renaming break all existing users? At minimum, there would
need to be backwards compatibility aliases to allow the previously used
integers to be used as well as the new names.

> diff --git a/hostapd/config_file.c b/hostapd/config_file.c
> +#ifdef CONFIG_MACSEC
> +const char *macsec_offload_strings[NUM_CONFIG_MACSEC_OFFLOAD_MODES] = {
> +	"off",
> +	"on",
> +	"mac",
> +	"phy",
> +	"prefer",
> +};

Having this array here as a globally available array and also in
wpa_supplicant/config.c feels strange. Maybe a single helper function
that maps a string to an enum macsec_offload_modes value would seem
cleaner. That helper function could also include mapping of the old
values (0..2) to their appropriate new enum values.

> diff --git a/src/common/ieee802_1x_defs.h b/src/common/ieee802_1x_defs.h
> index e7acff108eb3..ea964e1dd338 100644
> --- a/src/common/ieee802_1x_defs.h
> +++ b/src/common/ieee802_1x_defs.h
> +enum macsec_offload_modes {
> +	CONFIG_MACSEC_OFFLOAD_OFF,
> +	CONFIG_MACSEC_OFFLOAD_ON,
> +	CONFIG_MACSEC_OFFLOAD_MAC,
> +	CONFIG_MACSEC_OFFLOAD_PHY,
> +	CONFIG_MACSEC_OFFLOAD_PREFER,
> +	NUM_CONFIG_MACSEC_OFFLOAD_MODES,
> +};
> +extern const char *macsec_offload_strings[NUM_CONFIG_MACSEC_OFFLOAD_MODES];

This file looks a bit strange place for implementation internal values.
I would expect to see values defined in the IEEE standard here..
Something under src/pae might be more appropriate for this (and the
shared helper function for string->enum value mapping).

> diff --git a/wpa_supplicant/config_file.c b/wpa_supplicant/config_file.c
> +static void write_macsec_offload(FILE *f, struct wpa_ssid *ssid)
> +{
> +	if (ssid->macsec_offload < ARRAY_SIZE(macsec_offload_strings))
> +		fprintf(f, "\tmacsecoffload=%s\n", macsec_offload_strings[ssid->macsec_offload]);

That should have been macsec_offload as the field name (this would
result in writing a configuration file that would be rejected).

-- 
Jouni Malinen                                            PGP id EFC895FA