| Message ID | 20201103092244.1925158-1-john@phrozen.org |
|---|---|
| Headers | show |
| Series | mac80211: add BSS color change support | expand |
> + > +/* > + * cfg80211_bss_color_notify - notify about bss color event These all need /** I was going to fix that (and some below things), but it got a bit complex, and there's a question ... and patch 2 needs significant work anyway. > + NL80211_CMD_OBSS_COLOR_COLLISION, > + > + NL80211_CMD_COLOR_CHANGE, > + NL80211_CMD_COLOR_CHANGE_ANNOUNCEMENT_STARTED, > + NL80211_CMD_COLOR_CHANGE_ANNOUNCEMENT_ABORTED, > + NL80211_CMD_COLOR_CHANGE_ANNOUNCEMENT_COMPLETED, Might make sense to multiplex that into a single NL80211_CMD_COLOR_CHANGE event? Not sure, it doesn't matter too much now, but ... we do have limited commands eventually :-) > + * @NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_IES: Nested set of attributes containing the IE > + * information for the time while performing a color switch. _ELEMS perhaps? Should probably also be a bit more specific on what's used from there. > + * @NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_C_OFF_BEACON: An array of offsets (u16) to the color > + * switch counters in the beacons tail (%NL80211_ATTR_BEACON_TAIL). Is this inside or outside, for example? I think outside, but should it be? I guess should be just like CSA though. > + * @NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_C_OFF_PRESP: An array of offsets (u16) to the color > + * switch counters in the probe response (%NL80211_ATTR_PROBE_RESP). This is unused? > * @NUM_NL80211_ATTR: total number of nl80211_attrs available > * @NL80211_ATTR_MAX: highest attribute number currently defined > * @__NL80211_ATTR_AFTER_LAST: internal use > @@ -3035,6 +3071,12 @@ enum nl80211_attrs { > NL80211_ATTR_MULTIPLE_BSSID_IES, > NL80211_ATTR_MULTIPLE_BSSID_EMA, > > + NL80211_ATTR_OBSS_COLOR_BITMAP, > + > + NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COUNT, > + NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COLOR, > + NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_IES, and in fact not defined? > +static int nl80211_color_change(struct sk_buff *skb, struct genl_info *info) > +{ > + struct cfg80211_registered_device *rdev = info->user_ptr[0]; > + struct net_device *dev = info->user_ptr[1]; > + struct wireless_dev *wdev = dev->ieee80211_ptr; > + struct cfg80211_color_change_settings params; > + static struct nlattr *color_change_attrs[NL80211_ATTR_MAX + 1]; I'd prefer you didn't. I'm working on getting rid of the RTNL in most places, and that just adds more complexity to that ... please dynamically allocate it. Also, it just reserves space in the binary for a very rarely used code path. > + int err, len; > + > + if (!rdev->ops->color_change || > + !(wiphy_ext_feature_isset(&rdev->wiphy, NL80211_EXT_FEATURE_BSS_COLOR))) some line wrapping in some places would be nice - not sure if this one is affected, but still, you have a few that wouldn't be hard to wrap IMHO. > + err = nla_parse_nested(color_change_attrs, NL80211_ATTR_MAX, > + info->attrs[NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_IES], > + nl80211_policy, NULL); Please pass info->extack through to the last argument. > + if (err) > + return err; > + > + err = nl80211_parse_beacon(rdev, color_change_attrs, ¶ms.beacon_color_change); (here for example, re wrapping) > + if (!info->attrs[NL80211_ATTR_CNTDWN_OFFS_BEACON]) > + return -EINVAL; > + > + len = nla_len(info->attrs[NL80211_ATTR_CNTDWN_OFFS_BEACON]); > + if (len != sizeof(u16)) > + return -EINVAL; > + > + memcpy(¶ms.counter_offset_beacon, > + nla_data(info->attrs[NL80211_ATTR_CNTDWN_OFFS_BEACON]), > + sizeof(u16)); In the CSA case, this comes from the *inner* attributes. In your case, it comes from the *outer*. IMHO that's super confusing. > + if (params.counter_offset_beacon >= params.beacon_color_change.tail_len) > + return -EINVAL; > + > + if (params.beacon_color_change.tail[params.counter_offset_beacon] != params.count) > + return -EINVAL; > + > + if (info->attrs[NL80211_ATTR_CNTDWN_OFFS_PRESP]) { > + params.counter_offset_presp = > + nla_get_u16(info->attrs[NL80211_ATTR_CNTDWN_OFFS_PRESP]); same here. In CSA this is also allowed to be an array - the parsing code probably needs to be different because we currently don't validate the size/type of this to be NLA_U16, just like you did above with the NL80211_ATTR_CNTDWN_OFFS_BEACON. johannes
On Tue, 2020-11-03 at 10:22 +0100, John Crispin wrote: > > @@ -6805,4 +6822,15 @@ struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw, > struct sk_buff * > ieee80211_get_unsol_bcast_probe_resp_tmpl(struct ieee80211_hw *hw, > struct ieee80211_vif *vif); > +/** > + * ieeee80211_obss_color_collision_notify notify userland about a BSS color > + * collision. missing a "-" here > +static void ieee80211_color_change_bss_config_notify(struct ieee80211_sub_if_data *sdata, > + u8 color, int enable, u32 changed) > +{ > + sdata->vif.bss_conf.he_bss_color.color = color; > + sdata->vif.bss_conf.he_bss_color.enabled = enable; > + changed |= BSS_CHANGED_HE_BSS_COLOR; > + > + ieee80211_bss_info_change_notify(sdata, changed); > + > + if (ieee80211_hw_check(&sdata->local->hw, SUPPORTS_MULTI_BSSID_AP) && > + !sdata->vif.multiple_bssid.parent) { > + struct ieee80211_sub_if_data *child; > + > + rcu_read_lock(); > + list_for_each_entry_rcu(child, &sdata->local->interfaces, list) { > + if (child->vif.multiple_bssid.parent != &sdata->vif) > + continue; > + sdata_lock(child); Err, you can't lock a mutex in an atomic context opened by rcu_read_lock(). I guess you can just take the iflist_mtx or something, but this certainly doesn't seem right. Also, since it's being called under sdata_lock(parent), it'll need some lockdep annotations to be able to do this. > + child->vif.bss_conf.he_bss_color.color = color; > + child->vif.bss_conf.he_bss_color.enabled = enable; > + ieee80211_bss_info_change_notify(child, BSS_CHANGED_HE_BSS_COLOR); And the driver is also allowed to sleep in this IIRC, so you can't call it under rcu_read_lock(). johannes