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

[Lorenzo Bianconi]

> 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?

ack, I will fix it in v3.

> 
> > +	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?

I guess we can just move this code in the caller and reject the frame if the
values are not valid. I will fix it in v3.

> 
> > +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.)

ack, I will add kernel-doc in v3.

> 
> 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?

I agree, I guess we can drop emlsr_pad_delay and emlsr_trans_delay in
ieee80211_eml_params struct and just rely on sta values. I will fix it in v3.

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

IIUC the mcs map value are supposed to be in Operation mode notification frame
just for eMLMR. I think the driver should check if the bit is set in
ieee80211_eml_params control field to verify if mcs_map_bw values are valid.

> 
> > + * @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?

ack, I will fix it in v3.

> 
> > +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?

ack, I will fix it in v3.

> 
> > +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?

ack for avoiding memcpy().
Reading the standard, it is not clear to me if mcs map values are supposed to be
added in the Notification frame sent by the AP. What do you 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?

ack, I will fix it in v3.

> 
> > +
> > +			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.

ack, I will fix it in v3.

> 
> > +	if (skb->len < hdr_len + act_len)
> > +		return;
> 
> bit late that :)

ack, I will fix it in v3.

Regards,
Lorenzo

> 
> johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20260126/29ce1364/attachment.sig>