Hi Johannes, >Also, if it's possible, perhaps we should consider checking that nobody >globally advertises any capabilities they don't advertise on all possible >interface types?
>I'm assuming that not listing an interface type would mean that the global >defaults apply, but old userspace will also not know about the per-interface, >so you shouldn't list anything there. >So I'm thinking something like >supported_on_all = iftype_ext_capab[0] >for i in 1..num_iftype_ext_capab-1: > supported_on_all &= iftype_ext_capab[i] WARN_ON(wiphy->ext_capa_mask & > ~supported_on_all) We were thinking whether this is an appropriate validation or not since we cannot be sure how the Extended Capabilities are defined. They need not necessarily be all positive capabilities, they could couple both the positive and negative capabilities as well. Please let us know if this change is really needed. Thanks Vidyullatha -----Original Message----- From: Johannes Berg [mailto:johan...@sipsolutions.net] Sent: Sunday, April 17, 2016 3:42 AM To: Kanchanapally, Vidyullatha <vkanc...@qti.qualcomm.com> Cc: linux-wireless@vger.kernel.org; Malinen, Jouni <jo...@qca.qualcomm.com>; Hullur Subramanyam, Amarnath <amarn...@qca.qualcomm.com>; Undekari, Sunil Dutt <usd...@qti.qualcomm.com> Subject: Re: [PATCH] cfg80211: Advertise extended capabilities per interface type to userspace On Fri, 2016-04-15 at 16:57 +0530, Kanchanapally, Vidyullatha wrote: > > +struct wiphy_iftype_ext_capab { > + enum nl80211_iftype iftype; > + const u8 *ext_capab; > + const u8 *ext_capab_mask; > + u8 ext_capab_len; I think you should reuse the struct member names that we used before - that will make grepping for them easier. > + struct wiphy_iftype_ext_capab *iftype_ext_capab; const > + * @NL80211_ATTR_IFTYPE_EXT_CAPA: Nested attribute of the following > attributes: > + * %NL80211_ATTR_IFTYPE, %NL80211_ATTR_EXT_CAPA, > + * %NL80211_ATTR_EXT_CAPA_MASK, to specify the extended > capabilities per > + * interface type. That's a bit awkward to parse, since you have to parse the big attribute list again, but I guess it still makes more sense than having entirely different attributes. > + state->split_start++; > + break; > + case 13: > + if (rdev->wiphy.num_iftype_ext_capab && > + rdev->wiphy.iftype_ext_capab) { > + struct nlattr *nested_ext_capab, *nested; > + > + nested = nla_nest_start(msg, > + NL80211_ATTR_IFTYPE_ > EXT_CAPA); > + if (!nested) > + goto nla_put_failure; > + > + for (i = 0; i < rdev- > >wiphy.num_iftype_ext_capab; i++) { > + struct wiphy_iftype_ext_capab > *capab; > + > + capab = &rdev- > >wiphy.iftype_ext_capab[i]; > + nested_ext_capab = > nla_nest_start(msg, i); > + if (!nested_ext_capab || > + nla_put_u32(msg, > NL80211_ATTR_IFTYPE, > + capab->iftype) || > + nla_put(msg, > NL80211_ATTR_EXT_CAPA, > + capab->ext_capab_len, > + capab->ext_capab) || > + nla_put(msg, > NL80211_ATTR_EXT_CAPA_MASK, > + capab->ext_capab_len, > + capab->ext_capab_mask)) > + goto nla_put_failure; > + > + nla_nest_end(msg, nested_ext_capab); > + } > + nla_nest_end(msg, nested); > + } I think we should consider allowing this to be split multiple messages (for different interface types), since this can potentially get rather long (interface types x 2 x capa len). OTOH, we'd have to get a LOT of interface types x capabilities to hit the limit of 4k, not sure. But it should just require a few lines of code to do this. Also, if it's possible, perhaps we should consider checking that nobody globally advertises any capabilities they don't advertise on all possible interface types? I'm assuming that not listing an interface type would mean that the global defaults apply, but old userspace will also not know about the per-interface, so you shouldn't list anything there. So I'm thinking something like supported_on_all = iftype_ext_capab[0] for i in 1..num_iftype_ext_capab-1: supported_on_all &= iftype_ext_capab[i] WARN_ON(wiphy->ext_capa_mask & ~supported_on_all) (which is obviously some kind of pseudo-code.) johannes
