Re: [PATCH V2 1/3] nl80211: add support for setting fixed HE rate/gi/ltf

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

On Tue, 2020-02-04 at 11:35 +0100, John Crispin wrote:
> 
> +	[NL80211_TXRATE_HE] = {
> +		.type = NLA_EXACT_LEN,
> +		.len = sizeof(struct nl80211_txrate_he),
> +	},
> +	[NL80211_TXRATE_HE_GI] =  NLA_POLICY_RANGE(NLA_U8,
> +						   NL80211_RATE_INFO_HE_GI_0_8,
> +						   NL80211_RATE_INFO_HE_GI_3_2),
> +	[NL80211_TXRATE_HE_LTF] = NLA_POLICY_RANGE(NLA_U8,
> +						   NL80211_RATE_INFO_HE_1XLTF,
> +						   NL80211_RATE_INFO_HE_4XLTF),
>  };

Thanks for this :-)

> +		if (tb[NL80211_TXRATE_HE]) {
> +			if (!he_set_mcs_mask(info, sband, nla_data(tb[NL80211_TXRATE_HE]),
> +					     mask->control[band].he_mcs))
> +				return -EINVAL;

Maybe unify these into a single condition?

if (tb[...] &&
    he_set_...
	return -EINVAL;

Seems nicer to me. Especially with the lines already being so long.

> +		}
> +		if (tb[NL80211_TXRATE_HE_GI]) {
> +			mask->control[band].he_gi =
> +				nla_get_u8(tb[NL80211_TXRATE_HE_GI]);
> +			if (mask->control[band].he_gi > NL80211_RATE_INFO_HE_GI_3_2)
> +				return -EINVAL;

This is not needed with the policy, is it?

> +		}
> +		if (tb[NL80211_TXRATE_HE_LTF]) {
> +			mask->control[band].he_ltf =
> +				nla_get_u8(tb[NL80211_TXRATE_HE_LTF]);
> +			if (mask->control[band].he_ltf > NL80211_RATE_INFO_HE_4XLTF)
> +				return -EINVAL;

Same here.

>  			if (!(rdev->wiphy.bands[band]->ht_cap.ht_supported ||
> -			      rdev->wiphy.bands[band]->vht_cap.vht_supported))
> +			      rdev->wiphy.bands[band]->vht_cap.vht_supported ||
> +			      (rdev->wiphy.bands[band]->iftype_data &&
> +			       rdev->wiphy.bands[band]->iftype_data->he_cap.has_he)))
>  				return -EINVAL;

And now we get to why I replied at all and didn't just fix it up ;-)

That can't be right, iftype_data is an array of pointers, you're
basically always taking the first one?

johannes