Hi,
Sorry for the long delay reviewing this here...
> +/*
> + * cfg80211_ul_bitrate_mask - masks for MU uplink bitrate control
Can you add kernel-doc?
Also maybe we should think about EHT already - I guess we can use the
same structure here for EHT in the future?
> /**
> * struct cfg80211_tid_cfg - TID specific configuration
> @@ -4233,6 +4245,11 @@ struct cfg80211_ops {
> const u8 *peer,
> const struct cfg80211_bitrate_mask *mask);
>
> + int (*set_ul_bitrate_mask)(struct wiphy *wiphy,
> + struct net_device *dev,
> + const u8 *peer,
> + const struct cfg80211_ul_bitrate_mask *mask);
Missing documentation.
> NL80211_CMD_SET_FILS_AAD,
>
> + NL80211_CMD_SET_UL_BITRATE_MASK,
> /* add new commands above here */
Please keep a newline after.
> NL80211_ATTR_RADAR_OFFCHAN,
>
> + NL80211_ATTR_UL_RATES,
> /* add attributes here, update the policy in nl80211.c */
same here
> /**
> + * enum nl80211_ul_rate_attributes - MU uplink rate set attributes
> + * @NL80211_UL_RATE_HE: HE MU UL MCS rates for MU uplink traffic,
> + * see &struct nl80211_ul_rate_he
Generally, this should be called e.g. NL80211_UL_RATE_ATTR_*, i.e. have
"ATTR" somewhere.
Given my earlier comment about EHT I _think_ it should be
NL80211_UL_RATE_ATTR_HE_GI
so we can add
NL80211_UL_RATE_ATTR_EHT_GI
if needed (though I think in that particuilar it's really not needed and
we can use the same GI attribute for both since they both have the same
values.
> +static const struct nla_policy nl80211_ul_rate_attr_policy[NL80211_UL_RATE_MAX + 1] = {
> + [NL80211_UL_RATE_HE] = NLA_POLICY_EXACT_LEN(sizeof(struct nl80211_ul_rate_he)),
> + [NL80211_UL_RATE_HE_GI] = NLA_POLICY_RANGE(NLA_U8,
> + NL80211_RATE_INFO_HE_GI_0_8,
> + NL80211_RATE_INFO_HE_GI_3_2),
> + [NL80211_UL_RATE_HE_LTF] = NLA_POLICY_RANGE(NLA_U8,
> + NL80211_RATE_INFO_HE_1XLTF,
> + NL80211_RATE_INFO_HE_4XLTF),
> + [NL80211_UL_RATE_HE_LDPC] = { .type = NLA_FLAG },
> + [NL80211_UL_RATE_HE_STBC] = { .type = NLA_FLAG },
> +};
Nice!
But please link it into the overall policy:
+ [NL80211_ATTR_UL_RATES] =
+ NLA_POLICY_NESTED_ARRAY(nl80211_ul_rate_attr_policy),
I think? The index is the band enum value, but we can still validate it
already?
> + /* The nested attribute uses enum nl80211_band as the index. This maps
> + * directly to the enum nl80211_band values used in cfg80211.
> + */
> + nla_for_each_nested(tx_rates, attrs[attr], rem) {
> + enum nl80211_band band = nla_type(tx_rates);
> + int err;
> +
> + if (band < 0 || band >= NUM_NL80211_BANDS)
> + return -EINVAL;
NUM_NL80211_BANDS can change, maybe we should just ignore additional
bands?
> + sband = rdev->wiphy.bands[band];
> + if (!sband)
> + return -EINVAL;
> + err = nla_parse_nested_deprecated(tb, NL80211_UL_RATE_MAX,
> + tx_rates,
> + nl80211_ul_rate_attr_policy,
> + info->extack);
Please don't use _deprecated() in new code. And if you link the policy
properly, you don't need it here.
> + if (tb[NL80211_UL_RATE_HE_LDPC])
> + mask->control[band].he_ul_ldpc =
> + !nla_get_flag(tb[NL80211_UL_RATE_HE_LDPC]);
> + if (tb[NL80211_UL_RATE_HE_STBC])
> + mask->control[band].he_ul_stbc =
> + !nla_get_flag(tb[NL80211_UL_RATE_HE_STBC]);
This doesn't make sense - see how nla_get_flag() works and then remove
the if statements :)
> +TRACE_EVENT(rdev_set_ul_bitrate_mask,
> + TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
> + const u8 *peer, const struct cfg80211_ul_bitrate_mask *mask),
> + TP_ARGS(wiphy, netdev, peer, mask),
> + TP_STRUCT__entry(
> + WIPHY_ENTRY
> + NETDEV_ENTRY
> + MAC_ENTRY(peer)
> + ),
> + TP_fast_assign(
> + WIPHY_ASSIGN;
> + NETDEV_ASSIGN;
> + MAC_ASSIGN(peer, peer);
> + ),
> + TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", peer: " MAC_PR_FMT,
> + WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(peer))
> +);
>
A bit more information would be useful, IMHO.
johannes
