Re: [PATCH wireless-next v4 09/21] wifi: nl80211/cfg80211: add support for SMD context programming

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

On 11/11/2026 12:25 am, Johannes Berg wrote:
> On Fri, 2026-11-06 at 13:28 +0530, Pooventhiran G wrote:
>>
>> +static int
>> +nl80211_parse_smd_ctx_ba_params(struct nlattr *tb,
>> +				struct ieee80211_smd_ctx_ba *ba_tids)
>> +{
>> +	struct nlattr *ba_tb[NL80211_SMD_CTX_BA_ATTR_MAX + 1];
>> +	struct ieee80211_smd_ctx_ba *ba;
>> +	struct nlattr *ba_attr;
>> +	int n_tids = 0;
>> +	int rem, tid;
>> +
>> +	nla_for_each_nested(ba_attr, tb, rem) {
>> +		tid = nla_type(ba_attr) - 1;
>> +		if (tid < 0 || tid >= IEEE80211_SMD_CTX_NUM_TIDS)
>> +			return -EINVAL;
>> +
>> +		if (nla_parse_nested(ba_tb, NL80211_SMD_CTX_BA_ATTR_MAX,
>> +				     ba_attr, nl80211_smd_ctx_ba_policy,
>> +				     NULL))
> 
> if you set up the policies correctly (and you should, but I didn't go
> back to check now) then you don't need the policy here
> 

Sure, I will remove them from the parser API.

>> +		ba->amsdu_supported =
>> +			!!ba_tb[NL80211_SMD_CTX_BA_ATTR_AMSDU_SUPPORT];
> 
> it's not super useful to use !! to assign to a bool, and if you wanted
> to be fancy you'd use nla_flag(), right? ;)
> 

:)

I will clean these up.

>> +		if (nla_parse_nested(dl_tb, NL80211_SMD_CTX_DL_ATTR_MAX,
>> +				     tb[NL80211_SMD_CTX_ATTR_DL],
>> +				     nl80211_smd_ctx_dl_policy, NULL)) {
> 
> same here, and then it kind of cannot fail, I think
> 
>> +			nla_for_each_nested(sn_attr,
>> +					    dl_tb[NL80211_SMD_CTX_DL_ATTR_SN],
>> +					    rem) {
>> +				tid = nla_type(sn_attr) - 1;
>> +				if (tid < 0 ||
>> +				    tid >= IEEE80211_SMD_CTX_NUM_TIDS) {
>> +					GENL_SET_ERR_MSG(info,
>> +							 "invalid DL SN tid");
>> +					goto err_free_ctx;
>> +				}
>> +
>> +				if (nla_len(sn_attr) != sizeof(u16)) {
>> +					GENL_SET_ERR_MSG(info,
>> +							 "invalid DL SN len");
>> +					goto err_free_ctx;
>> +				}
> 
> You've got to be _kidding_ me, why do I even have to get see such
> obviously redundant code?
> 
> Please read your own code.
> 

With respect to the attributes and policies defined in
https://lore.kernel.org/all/20261106-smd-v4-5-62e39487f51a@oss.qualcomm.com/,
DL SN (similarly, UL SN and UL PN) is a nested attribute where tid iterator is
used as the inner attribute to pack the scalar per-TID SN and there was no way
to add policies for that (other than having to define something like [1] and
use NLA_POLICY_NESTED).

There were similar changes done for array of values for
NL80211_ATTR_CNTDWN_OFFS_BEACON, NL80211_ATTR_SCAN_FREQUENCIES, etc., So, this
approach was done here to make sure that each SN in the array was u16 as
expected. If there _is_ a way to add policies for the iterator-based nesting
of such values that you could point me to, I will change accordingly.

[1] =>

struct nla_policy sn_policy[8] = {
	[0] = { .type = NLA_U16 },
	[1] = { .type = NLA_U16 },
	...
	[7] = { .type = NLA_U16 },
}

> johannes