Re: [PATCH wireless-next v4 08/21] wifi: nl80211/cfg80211: pack SMD dynamic context along with frame

[Pooventhiran G <pooventhiran.g@oss.qualcomm.com>]

On 11/11/2026 12:21 am, Johannes Berg wrote:
> On Fri, 2026-11-06 at 13:28 +0530, Pooventhiran G wrote:
>>
>> +static size_t _nl80211_smd_ctx_nl_ba_param_size(u8 n_tids, bool dl)
>> +{
>> +	size_t n = 0;
>> +
>> +	n += nla_total_size(0); /* ba_params nest header */
>> +	n += n_tids * (nla_total_size(0) + /* tid nest header */
>> +		       nla_total_size(sizeof(u16)) + /* buff_size */
>> +		       nla_total_size(sizeof(u8)) + /* policy */
>> +		       nla_total_size(0) + /* amsdu_support */
>> +		       nla_total_size(sizeof(u16)) + /* timeout */
>> +		       nla_total_size(0) + /* ext_no_frag */
>> +		       nla_total_size(sizeof(u8)) + /* ext_frag_lvl */
>> +		       nla_total_size(sizeof(u16))); /* ext_buff_size */
>> +
>> +	if (dl)
>> +		n += n_tids * nla_total_size(sizeof(u16)); /* winstarto */
>> +
>> +	return n;
>> +}
>> +
>> +static size_t
>> +nl80211_smd_ctx_nl_size(struct cfg80211_smd_transition_info *st_info)
>> +{
>> +	const struct ieee80211_smd_ctx *ctx = st_info->ctx;
>> +	int n_dl_tids = 0, n_ul_tids = 0;
>> +	u8 pn_len = ctx->pn_len;
>> +	size_t n = 0;
>> +
>> +	/* count valid TIDs */
>> +	n_dl_tids = hweight8(ctx->dl.valid_tid_bmap);
>> +	n_ul_tids = hweight8(ctx->ul.valid_tid_bmap);
>> +
>> +	n += nla_total_size(sizeof(u8)); /* ST type */
>> +
>> +	/* outer NL80211_ATTR_SMD_CTX nest */
>> +	n += nla_total_size(0); /* nest header */
>> +
>> +	/* DL nest */
>> +	n += nla_total_size(0); /* DL nest header */
>> +
>> +	n += nla_total_size(sizeof(u8)); /* valid_tid_bitmap */
>> +
>> +	if (ctx->dl_sn_valid) {
>> +		n += nla_total_size(0); /* DL SN nest header */
>> +		n += n_dl_tids * nla_total_size(sizeof(u16)); /* DL SN */
>> +		if (ctx->mgmt_tid_valid)
>> +			n += nla_total_size(sizeof(u16)); /* DL mgmt_sn */
>> +	}
>> +
>> +	if (ctx->pn_valid)
>> +		n += nla_total_size(pn_len); /* DL PN */
>> +
>> +	if (ctx->ba_params_valid)
>> +		n += _nl80211_smd_ctx_nl_ba_param_size(n_dl_tids, true); /* DL BA */
>> +
>> +	/* UL nest */
>> +	n += nla_total_size(0); /* UL nest header */
>> +
>> +	n += nla_total_size(sizeof(u8)); /* valid_tid_bitmap */
>> +
>> +	if (ctx->ul_sn_valid) {
>> +		n += nla_total_size(0); /* UL SN nest header */
>> +		n += n_ul_tids * nla_total_size(sizeof(u16)); /* UL SN */
>> +		if (ctx->mgmt_tid_valid)
>> +			n += nla_total_size(sizeof(u16)); /* UL mgmt_sn */
>> +	}
>> +
>> +	if (ctx->pn_valid) {
>> +		n += nla_total_size(0); /* UL PN nest header */
>> +		n += n_ul_tids * nla_total_size(pn_len); /* UL PN */
>> +		if (ctx->mgmt_tid_valid)
>> +			n += nla_total_size(pn_len); /* UL mgmt_pn */
>> +	}
>> +
>> +	if (ctx->ba_params_valid)
>> +		n += _nl80211_smd_ctx_nl_ba_param_size(n_ul_tids, false); /* UL BA */
>> +
>> +	return n;
>> +}
> 
> This kind of code is just crazy LLM output ... we can just have a sane
> upper bound and make that a single line of code? Nobody will really care
> about the precise size of the allocation.
> 

We moved to computing the size instead of having a fixed value because we
transfer an almost dense context in one phase but just a very sparse one in
another and thought this would save space on the nlmsg skb. If we transfer
data as D2.0 says, we will transfer about 330 bytes in Prep while only about
70 bytes in Exec. This assumes all 8 active data TIDs and without nlhdr
overhead. Is it okay to define the max size (~400 bytes + nlhdr overhead)
nevertheless to avoid this size computation?

>> +static int
>> +nl80211_put_smd_ctx_ba_params(struct sk_buff *msg,
>> +			      const unsigned long *tid_bmap,
>> +			      const struct ieee80211_smd_ctx_ba *ba_tids,
>> +			      bool dl)
>> +{
>> +	u32 ba_params_attr = NL80211_SMD_CTX_DL_ATTR_BA_PARAMS;
>> +	struct nlattr *dl_ul_ba = NULL, *tid_ba = NULL;
> 
> nope.
> 
>> +	u8 tid;
>> +
>> +	if (!dl)
>> +		ba_params_attr = NL80211_SMD_CTX_UL_ATTR_BA_PARAMS;
>> +
>> +	dl_ul_ba = nla_nest_start(msg, ba_params_attr);
>> +	if (!dl_ul_ba)
>> +		return -ENOBUFS;
>> +
>> +	for_each_set_bit(tid, tid_bmap, IEEE80211_SMD_CTX_NUM_TIDS) {
>> +		const struct ieee80211_smd_ctx_ba *ba = &ba_tids[tid];
>> +
>> +		tid_ba = nla_nest_start(msg, tid + 1);
> 
> that can also be declared in the loop
> >> +static int nl80211_put_smd_ctx(struct sk_buff *msg,
>> +			       struct cfg80211_smd_transition_info *st_info)
>> +{
>> +	struct nlattr *dl_sn = NULL, *ul_sn = NULL, *ul_pn = NULL;
>> +	struct nlattr *smd_ctx = NULL, *dl = NULL, *ul = NULL;
>> +	const struct ieee80211_smd_ctx *ctx;
>> +	unsigned long dl_tid_bmap = 0, ul_tid_bmap = 0;
> 
> same here, what's with all the pointless =0/=NULL?
> 
> and move variables into the inner code blocks too
> 

Will remove the initializations and move the variables to the inner blocks.

>> +	pn_valid = (ctx->pn_len && ctx->pn_len <= IEEE80211_SMD_CTX_MAX_PN_LEN &&
>> +		    ctx->pn_valid);
> 
> useless parentheses, but anyway you don't need pn_len or the max, or the
> pn_valid even since it's always transferred ...
> 

We may not transfer PN always. In case we do (as being discussed in
https://lore.kernel.org/all/b6cda6b5-adb0-403d-a2b4-d64939a967e7@oss.qualcomm.com/),
we might still need the length bound check to make sure driver does not give
incorrect length.

>> +nla_fail_ul:
>> +	nla_nest_cancel(msg, ul);
>> +
>> +nla_fail_dl:
>> +	nla_nest_cancel(msg, dl);
>> +
>> +nla_fail:
>> +	nla_nest_cancel(msg, smd_ctx);
>> +	return -EINVAL;
> 
> crazy LLMs again? You obviously don't need to have nested cancel to get
> rid of the whole thing ... probably don't need it at all because you're
> just going to free the skb.
> 

Ahh, I think I misreferred some examples (like cfg80211_net_detect_results())
to cancel the inner nests but are pointless to an skb that is going to be
freed. I will remove them.

> johannes