driver_nl80211.c: Implement supported rates / capabilities

[jlopex]

• if (params->supp_rates || !params->set) {
• NLA_PUT(msg, NL80211_ATTR_STA_SUPPORTED_RATES,
• params->supp_rates_len, params->supp_rates);
• wpa_hexdump(MSG_DEBUG, " * supported rates",
• params->supp_rates, params->supp_rates_len);
• }

I don't really remember why this change was there.. we don't have supp_rates / capability settings yet?

[jlopex]

This is a workaround to not include supp_rates when updating the plink status on each station.

I checked the kernel source and the cfg80211_check_station_change function on net/wireless/nl80211.c does a check to NOT allow updating non updatable fields, one of them is supported_rates.

        if (statype != CFG80211_STA_TDLS_PEER_SETUP) {
                /* reject other things that can't change */
                if (params->sta_modify_mask & STATION_PARAM_APPLY_UAPSD)
                        return -EINVAL;
                if (params->sta_modify_mask & STATION_PARAM_APPLY_CAPABILITY)
                        return -EINVAL;
                if (params->supported_rates)
                        return -EINVAL;
                if (params->ext_capab || params->ht_capa || params->vht_capa)
                        return -EINVAL;
        }

If we don't want to have this check on wpa_supplicant, we will need to create a patch for the kernel to ignore properly fields that have not been set on userspace.

[jlopex]

@bcopeland @silverjam

A possible patch for the kernel. If supported_len is 0, then do not update params->supported_rates pointer, by default params->supported_rates point to NULL.

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index c50768a..b1f6bb1 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4000,10 +4000,11 @@ static int nl80211_set_station(struct sk_buff *skb, struct genl_info *info)
        mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);

        if (info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]) {
-               params.supported_rates =
-                       nla_data(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
                params.supported_rates_len =
                        nla_len(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
+               if (params.supported_rates_len > 0)
+                       params.supported_rates =
+                               nla_data(info->attrs[NL80211_ATTR_STA_SUPPORTED_RATES]);
        }

        if (info->attrs[NL80211_ATTR_STA_CAPABILITY]) {

[silverjam]

I don't really understand the rational for anything here. What's wrong with the commit? Why do we add support for the "supported rates" station attr? If something is empty, or isn't there, it seems perfectly reasonable to not include it... especially if it causes issues with code that's already in the kernel.

If we require a kernel patch for certain things in wpa_supp mesh support to work, it limits the impact of the patches. I know the main point is to push them upstream, but it'd be nice if we didn't have to require kernel changes to support userspace code.

In summary: my vote is to just leave this alone.

[ctwitty]

I agree that this is OK to leave. wpa_driver_nl80211_sta_add is used for both adding and updating a station entry. However, the kernel code does not allow us to update the supported rates as Javier references above. IMO, there is no reason to modify the kernel code.

For future reference, this code was added in commit 3b088ac7d6ecc7b96a7bd7a548301c95bac8c01c

I second closing this and leaving it as is.