Re: [PATCH v2] wifi: move action code from per-type frame structs

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2026-02-27 at 12:22 +0100, Pablo MARTIN-GOMEZ wrote:
> 
> > +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
> > @@ -755,16 +755,12 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
> >  	switch (action_code) {
> >  	case WLAN_PUB_ACTION_TDLS_DISCOVER_RES:
> >  		/* See the layout of 'struct ieee80211_mgmt'. */
> > -		extra = sizeof(mgmt->u.action.u.tdls_discover_resp) +
> > -			sizeof(mgmt->u.action.category);
> > +		extra = IEEE80211_MIN_ACTION_SIZE(tdls_discover_resp) - 24;
> Not sure that is clearer than:
> ```
> extra = sizeof(mgmt->u.action.u.tdls_discover_resp) +
>         sizeof(mgmt->u.action.category) +
>         sizeof(mgmt->u.action.action_code);
> ```

That's a bit of a corner case, but given the above code that uses 24
explicitly, I tend to think it _is_ clearer, and it's definitely "safer
to understand" - you know that you didn't miss anything...

If someone wrote here

	extra = sizeof(mgmt->u.action.u.tdls_discover_resp);

would you really see that it's wrong? I think it's very easy to miss.
It's correct now, but the "- 24" definitely flags this as something
special to check the context.

> >  	addba_ext_data =
> >  		ieee80211_retrieve_addba_ext_data(sta,
> > -						  mgmt->u.action.u.addba_req.variable,
> > +						  mgmt->u.action.addba_req.variable,
> >  						  len -
> >  						  offsetof(typeof(*mgmt),
> > -							   u.action.u.addba_req.variable),
> > +							   u.action.addba_req.variable),
> `offsetof(typeof(*mgmt), u.action.u.type.variable` is equivalent to
> `IEEE80211_MIN_ACTION_SIZE(type)`, isn't clearer to use it?

I guess we could. I was mostly getting it to compile at some point :)

> > @@ -949,8 +945,8 @@ void mesh_rx_path_sel_frame(struct ieee80211_sub_if_data *sdata,
> >  	}
> >  	rcu_read_unlock();
> >  
> > -	baselen = (u8 *) mgmt->u.action.u.mesh_action.variable - (u8 *) mgmt;
> > -	elems = ieee802_11_parse_elems(mgmt->u.action.u.mesh_action.variable,
> > +	baselen = mgmt->u.action.mesh_action.variable - (u8 *)mgmt;
> Another equivalent to `IEEE80211_MIN_ACTION_SIZE(type)`

type?

> > +	elems = ieee802_11_parse_elems(mgmt->u.action.mesh_action.variable,
> >  				       len - baselen,

> > -	dialog_token = mgmt->u.action.u.ttlm_req.dialog_token;
> > -	ies_len  = len - offsetof(struct ieee80211_mgmt,
> > -				  u.action.u.ttlm_req.variable);
> > -	elems = ieee802_11_parse_elems(mgmt->u.action.u.ttlm_req.variable,
> > +	dialog_token = mgmt->u.action.ttlm_req.dialog_token;
> > +	ies_len  = len - IEEE80211_MIN_ACTION_SIZE(ttlm_req);
> Here you did it :)

Guess it didn't really matter that much. Or I stumbled over this one at
a different time, or something :)

I don't think any of this is worth changing right now. Feel free to send
extra patches.

johannes