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

[Johannes Berg]

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.

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

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

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

johannes