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

[Pablo MARTIN-GOMEZ <pmartin-gomez@freebox.fr>]

On 27/02/2026 12:45, Johannes Berg wrote:
> 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.
>
Reading the full function, and give that the 24 value is already
hardcoded in the above code, I guess you are right.

>>>  	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?
A copy-paste error, I wanted to say `IEEE80211_MIN_ACTION_SIZE(mesh_action)`
> 
>>> +	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.
Fair enough. I'll send the extra patches. Otherwise the change looks
good to me.
> 
> johannes

Pablo MG