[PATCH wireless-next v2 1/2] wifi: mac80211: Add eMLSR/eMLMR action frame parsing support

[Johannes Berg]

On Sun, 2026-01-25 at 11:51 +0100, Lorenzo Bianconi wrote:
> 
> +static inline u8 ieee80211_get_emlsr_pad_delay_update(u8 param)
> +{
> +	u8 pad_delay = FIELD_GET(IEEE80211_EML_EMLSR_PAD_DELAY, param);

I generally prefer the typed versions and mac80211 (mostly?) uses those,
i.e. u8_get_bits() and friends, since they also cover endian conversions
where needed. Is there any reason you use FIELD_* versions here?

> +	if (pad_delay > IEEE80211_EML_CAP_EMLSR_PADDING_DELAY_256US)
> +		pad_delay = 0;

Seems that should use a constant, rather than =0?

Also, is that really the right thing to do (also below) to just silently
cap it? Maybe that should be up to the caller? In some(/most/all)? cases
we should probably even just _reject_ frames that carry an invalid
value, which you can't do with this helper?

> +static inline u32 ieee80211_get_emlsr_trans_delay_update(u8 param)
> +{
> +	u16 trans_delay = FIELD_GET(IEEE80211_EML_EMLSR_TRANS_DELAY, param);
> +
> +	if (trans_delay > IEEE80211_EML_CAP_EMLSR_TRANSITION_DELAY_256US)
> +		trans_delay = 0;
> +
> +	return trans_delay;

why does that use _three_ different types? Wouldn't u8 be sufficient?

> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1902,6 +1902,21 @@ enum ieee80211_offload_flags {
>  	IEEE80211_OFFLOAD_DECAP_ENABLED		= BIT(2),
>  };
>  
> +struct ieee80211_eml_params {
> +	u8 control;
> +	u16 link_bitmap;
> +	union {
> +		struct {
> +			u16 emlsr_pad_delay;
> +			u16 emlsr_trans_delay;
> +		};
> +		struct {
> +			u8 mcs_map_count;
> +			u8 mcs_map_bw[9];
> +		};
> +	};
> +};

Maybe add kernel-doc? Also not sure the union really is worth it? It's a
tiny thing. Especially since you don't even label it - maybe if the
parts were labled emlsr and emlmr, and then you had emlsr.pad_delay?

(I'd label them anyway, of course, even if not a union.)

Also now the emlsr pad/trans delay are duplicated in the station info,
is that worth doing? You have to and do track them there too, anyway, as
we discussed on IRC, could just have the driver use them from there?

Per spec I'm also not sure what the MCS map should be when it's not
included in the frame?

> + * @set_eml_op_mode: Configure eMLSR/eMLMR operation mode in the underlay
> + *	driver according to the parameter received in the EML Operating mode
> + *	notification frame.

Maybe describe the link_id here, or move it to the params and describe
it in kernel-doc there?

> +static inline int drv_set_eml_op_mode(struct ieee80211_sub_if_data *sdata,
> +				      struct ieee80211_sta *sta,
> +				      unsigned int link_id,
> +				      struct ieee80211_eml_params *eml_params)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	int ret = 0;

Shouldn't that be -EOPNOTSUPP?

> +static void
> +ieee80211_send_eml_op_mode_notif(struct ieee80211_sub_if_data *sdata,
> +				 struct ieee80211_mgmt *req, u8 act_len)
> +{
> +	int hdr_len = offsetof(struct ieee80211_mgmt, u.action.u.eml_omn);
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_mgmt *mgmt;
> +	struct sk_buff *skb;
> +
> +	skb = dev_alloc_skb(local->tx_headroom + hdr_len + act_len);
> +	if (!skb)
> +		return;
> +
> +	skb_reserve(skb, local->tx_headroom);
> +	mgmt = skb_put_zero(skb, hdr_len);
> +	mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
> +					  IEEE80211_STYPE_ACTION);
> +	memcpy(mgmt->da, req->sa, ETH_ALEN);
> +	memcpy(mgmt->sa, sdata->vif.addr, ETH_ALEN);
> +	memcpy(mgmt->bssid, sdata->vif.addr, ETH_ALEN);
> +
> +	mgmt->u.action.category = WLAN_CATEGORY_PROTECTED_EHT;
> +	memcpy(&mgmt->u.action.u.eml_omn, &req->u.action.u.eml_omn, act_len);
> +	mgmt->u.action.u.eml_omn.control &=
> +		~(IEEE80211_EML_CTRL_EMLSR_PARAM_UPDATE |
> +		  IEEE80211_EML_CTRL_INDEV_COEX_ACT);
> +	ieee80211_tx_skb(sdata, skb);

It seems to me that it'd be better to not copy the request, but rather
build the response. It's not _that_ much data, and from the spec it
seems to me that e.g. the MCS map should be included in the response,
but you do that now, I think?

> +void ieee80211_rx_eml_op_mode_notif(struct ieee80211_sub_if_data *sdata,
> +				    struct sk_buff *skb)
> +{
> +	int hdr_len = offsetof(struct ieee80211_mgmt, u.action.u.eml_omn);
> +	enum nl80211_iftype type = ieee80211_vif_type_p2p(&sdata->vif);
> +	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
> +	const struct wiphy_iftype_ext_capab *ift_ext_capa;
> +	struct ieee80211_mgmt *mgmt = (void *)skb->data;
> +	struct ieee80211_local *local = sdata->local;
> +	u8 control = mgmt->u.action.u.eml_omn.control;
> +	u8 *ptr = mgmt->u.action.u.eml_omn.variable;
> +	struct ieee80211_eml_params eml_params = {};
> +	struct sta_info *sta;
> +	u8 act_len = 3; /* action_code + dialog_token + control */
> +
> +	if (!ieee80211_vif_is_mld(&sdata->vif))
> +		return;
> +
> +	/* eMLSR and eMLMR can't be enabled at the same time */
> +	if ((control & IEEE80211_EML_CTRL_EMLSR_MODE) &&
> +	    (control & IEEE80211_EML_CTRL_EMLMR_MODE))
> +		return;
> +
> +	if ((control & IEEE80211_EML_CTRL_EMLMR_MODE) &&
> +	    (control & IEEE80211_EML_CTRL_EMLSR_PARAM_UPDATE))
> +		return;
> +
> +	ift_ext_capa = cfg80211_get_iftype_ext_capa(local->hw.wiphy, type);
> +	if (!ift_ext_capa)
> +		return;
> +
> +	if (!status->link_valid)
> +		return;
> +
> +	sta = sta_info_get_bss(sdata, mgmt->sa);
> +	if (!sta)
> +		return;
> +
> +	if (control & IEEE80211_EML_CTRL_EMLSR_MODE) {
> +		if (!(ift_ext_capa->eml_capabilities &
> +		      IEEE80211_EML_CAP_EMLSR_SUPP))
> +			return;
> +
> +		if (control & IEEE80211_EML_CTRL_EMLSR_PARAM_UPDATE) {
> +			u16 eml_cap = sta->sta.eml_cap;
> +			u8 pad_delay, trans_delay;
> +
> +			/* Update sta padding and transition delay */
> +			pad_delay =
> +				ieee80211_get_emlsr_pad_delay_update(ptr[3]);
> +			trans_delay =
> +				ieee80211_get_emlsr_pad_delay_update(ptr[3]);

It seems to me you're missing a bunch of input validation?

> +
> +			eml_cap &= ~(IEEE80211_EML_CAP_EMLSR_PADDING_DELAY |
> +				     IEEE80211_EML_CAP_EMLSR_TRANSITION_DELAY);
> +			eml_cap |= FIELD_PREP(IEEE80211_EML_EMLSR_PAD_DELAY,
> +					      pad_delay) |
> +				   FIELD_PREP(IEEE80211_EML_EMLSR_TRANS_DELAY,
> +					      trans_delay);
> +			sta->sta.eml_cap = eml_cap;

Same comment about typed bitfield accessors, and u8_replace_bits() would
even shorten that quite a bit.

> +	if (skb->len < hdr_len + act_len)
> +		return;

bit late that :)

johannes