From: Johannes Berg <johannes@sipsolutions.net> To: John Crispin <john@phrozen.org> Cc: linux-wireless@vger.kernel.org, ath11k@lists.infradead.org Subject: Re: [PATCH V3 1/9] nl80211: add basic multiple bssid support Date: Thu, 27 Aug 2020 14:58:52 +0200 [thread overview] Message-ID: <6a4fa0a60089bb1565def2bca187b9c0afc36022.camel@sipsolutions.net> (raw) In-Reply-To: <20200812150050.2683396-2-john@phrozen.org> On Wed, 2020-08-12 at 17:00 +0200, John Crispin wrote: > This patch adds support for passing the multiple bssid config to the > kernel when adding an interface. If the BSS is non-transmitting it needs > to be indicated. A non-transmitting BSSID will have a parent interface, > which needs to be transmitting. The multiple bssid elements are passed > as an array. > > Signed-off-by: John Crispin <john@phrozen.org> > --- > include/net/cfg80211.h | 35 +++++++++++++++++++++++++++++++ > include/uapi/linux/nl80211.h | 22 ++++++++++++++++++++ > net/wireless/nl80211.c | 40 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 97 insertions(+) > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h > index 541dea0fd571..0b0c730dc8d2 100644 > --- a/include/net/cfg80211.h > +++ b/include/net/cfg80211.h > @@ -470,6 +470,21 @@ struct ieee80211_supported_band { > const struct ieee80211_sband_iftype_data *iftype_data; > }; > > +/** > + * struct ieee80211_multiple_bssid - AP settings for multi bssid > + * > + * @index: the index of this AP in the multi bssid group. > + * @count: the total number of multi bssid peer APs. > + * @parent: a non-transmitted bssid has a transmitted parent It obviously has a parent, but what does this u32 value mean? > +/** > + * struct cfg80211_multiple_bssid_data - multiple_bssid data > + * @ies: array of extra information element(s) to add into Beacon frames for multiple > + * bssid or %NULL > + * @len: array of lengths of multiple_bssid.ies in octets > + * @cnt: number of entries in multiple_bssid.ies > + */ > +struct cfg80211_multiple_bssid_data { > + u8 *ies[NL80211_MULTIPLE_BSSID_IES_MAX]; > + size_t len[NL80211_MULTIPLE_BSSID_IES_MAX]; This is all pretty much dynamic - why have the hard-coded limitation? Wouldn't it be only marginally harder to do struct ... { int cnt; struct { const u8 *ies; size_t len; } data[]; }; and size it dynamically? And have the driver advertise some kind of limit, I guess. In fact, even with the patch as is, what if a driver only can do 4 not 8... Or what if your driver can do 8, but the next driver comes along and has to bump it to 16, then your current driver will be a mess. > + * @NL80211_ATTR_MULTIPLE_BSSID_PARENT: If this is a Non-Transmitted BSSID, define > + * the parent (transmitting) interface. By what? interface index? > @@ -662,6 +662,11 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COUNT] = { .type = NLA_U8 }, > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COLOR] = { .type = NLA_U8 }, > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_IES] = NLA_POLICY_NESTED(nl80211_policy), > + [NL80211_ATTR_MULTIPLE_BSSID_NON_TRANSMITTING] = { .type = NLA_FLAG }, > + [NL80211_ATTR_MULTIPLE_BSSID_PARENT] = { .type = NLA_U32 }, > + [NL80211_ATTR_MULTIPLE_BSSID_INDEX] = { .type = NLA_U8 }, > + [NL80211_ATTR_MULTIPLE_BSSID_COUNT] = { .type = NLA_U8 }, > + [NL80211_ATTR_MULTIPLE_BSSID_IES] = { .type = NLA_NESTED }, Maybe those should all go into a single top-level attribute with their own nested policy? Or maybe not, I'm certainly willing to entertain arguments eitherway. What's the point of BSSID_COUNT by the way, you can count based on the number of elements in the BSSID_IES array, no? Actually, I don't understand this API at all. This is being set on a single virtual interface. Clearly, you need a new interface for every BSSID, transmitting or not. So why can you have many things set on a single interface? And why do you need a count, if you know the number of interfaces you have? > + if (info->attrs[NL80211_ATTR_MULTIPLE_BSSID_PARENT]) > + params.multiple_bssid.parent = > + nla_get_u8(info->attrs[NL80211_ATTR_MULTIPLE_BSSID_PARENT]); As Aloka also pointed out, this must be nla_get_u32(). But I don't really like passing that down - if it's an interface index then resolve it here and pass a pointer. If it's something else, please document it clearly. Thanks, johannes
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Berg <johannes@sipsolutions.net> To: John Crispin <john@phrozen.org> Cc: linux-wireless@vger.kernel.org, ath11k@lists.infradead.org Subject: Re: [PATCH V3 1/9] nl80211: add basic multiple bssid support Date: Thu, 27 Aug 2020 14:58:52 +0200 [thread overview] Message-ID: <6a4fa0a60089bb1565def2bca187b9c0afc36022.camel@sipsolutions.net> (raw) In-Reply-To: <20200812150050.2683396-2-john@phrozen.org> On Wed, 2020-08-12 at 17:00 +0200, John Crispin wrote: > This patch adds support for passing the multiple bssid config to the > kernel when adding an interface. If the BSS is non-transmitting it needs > to be indicated. A non-transmitting BSSID will have a parent interface, > which needs to be transmitting. The multiple bssid elements are passed > as an array. > > Signed-off-by: John Crispin <john@phrozen.org> > --- > include/net/cfg80211.h | 35 +++++++++++++++++++++++++++++++ > include/uapi/linux/nl80211.h | 22 ++++++++++++++++++++ > net/wireless/nl80211.c | 40 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 97 insertions(+) > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h > index 541dea0fd571..0b0c730dc8d2 100644 > --- a/include/net/cfg80211.h > +++ b/include/net/cfg80211.h > @@ -470,6 +470,21 @@ struct ieee80211_supported_band { > const struct ieee80211_sband_iftype_data *iftype_data; > }; > > +/** > + * struct ieee80211_multiple_bssid - AP settings for multi bssid > + * > + * @index: the index of this AP in the multi bssid group. > + * @count: the total number of multi bssid peer APs. > + * @parent: a non-transmitted bssid has a transmitted parent It obviously has a parent, but what does this u32 value mean? > +/** > + * struct cfg80211_multiple_bssid_data - multiple_bssid data > + * @ies: array of extra information element(s) to add into Beacon frames for multiple > + * bssid or %NULL > + * @len: array of lengths of multiple_bssid.ies in octets > + * @cnt: number of entries in multiple_bssid.ies > + */ > +struct cfg80211_multiple_bssid_data { > + u8 *ies[NL80211_MULTIPLE_BSSID_IES_MAX]; > + size_t len[NL80211_MULTIPLE_BSSID_IES_MAX]; This is all pretty much dynamic - why have the hard-coded limitation? Wouldn't it be only marginally harder to do struct ... { int cnt; struct { const u8 *ies; size_t len; } data[]; }; and size it dynamically? And have the driver advertise some kind of limit, I guess. In fact, even with the patch as is, what if a driver only can do 4 not 8... Or what if your driver can do 8, but the next driver comes along and has to bump it to 16, then your current driver will be a mess. > + * @NL80211_ATTR_MULTIPLE_BSSID_PARENT: If this is a Non-Transmitted BSSID, define > + * the parent (transmitting) interface. By what? interface index? > @@ -662,6 +662,11 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = { > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COUNT] = { .type = NLA_U8 }, > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_COLOR] = { .type = NLA_U8 }, > [NL80211_ATTR_COLOR_CHANGE_ANNOUNCEMENT_IES] = NLA_POLICY_NESTED(nl80211_policy), > + [NL80211_ATTR_MULTIPLE_BSSID_NON_TRANSMITTING] = { .type = NLA_FLAG }, > + [NL80211_ATTR_MULTIPLE_BSSID_PARENT] = { .type = NLA_U32 }, > + [NL80211_ATTR_MULTIPLE_BSSID_INDEX] = { .type = NLA_U8 }, > + [NL80211_ATTR_MULTIPLE_BSSID_COUNT] = { .type = NLA_U8 }, > + [NL80211_ATTR_MULTIPLE_BSSID_IES] = { .type = NLA_NESTED }, Maybe those should all go into a single top-level attribute with their own nested policy? Or maybe not, I'm certainly willing to entertain arguments eitherway. What's the point of BSSID_COUNT by the way, you can count based on the number of elements in the BSSID_IES array, no? Actually, I don't understand this API at all. This is being set on a single virtual interface. Clearly, you need a new interface for every BSSID, transmitting or not. So why can you have many things set on a single interface? And why do you need a count, if you know the number of interfaces you have? > + if (info->attrs[NL80211_ATTR_MULTIPLE_BSSID_PARENT]) > + params.multiple_bssid.parent = > + nla_get_u8(info->attrs[NL80211_ATTR_MULTIPLE_BSSID_PARENT]); As Aloka also pointed out, this must be nla_get_u32(). But I don't really like passing that down - if it's an interface index then resolve it here and pass a pointer. If it's something else, please document it clearly. Thanks, johannes -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k
next prev parent reply other threads:[~2020-08-27 12:59 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-12 15:00 [PATCH V3 0/9] mac80211: add multiple bssid / EMA John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 1/9] nl80211: add basic multiple bssid support John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-19 2:48 ` Aloka Dixit 2020-08-19 2:48 ` Aloka Dixit 2020-08-19 4:32 ` John Crispin 2020-08-27 12:58 ` Johannes Berg [this message] 2020-08-27 12:58 ` Johannes Berg 2020-08-12 15:00 ` [PATCH V3 2/9] mac80211: add multiple bssid support to interface handling John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-27 13:03 ` Johannes Berg 2020-08-27 13:03 ` Johannes Berg 2020-08-27 13:08 ` Johannes Berg 2020-08-27 13:08 ` Johannes Berg 2020-10-08 0:33 ` Pradeep Kumar Chitrapu 2020-10-08 0:33 ` Pradeep Kumar Chitrapu 2020-10-08 8:06 ` John Crispin 2020-10-08 8:06 ` John Crispin 2020-10-08 17:21 ` Pradeep Kumar Chitrapu 2020-10-08 17:21 ` Pradeep Kumar Chitrapu 2020-10-08 19:42 ` John Crispin 2020-10-08 19:42 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 3/9] mac80211: add multiple bssid support to beacon handling John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 4/9] mac80211: add multiple bssid/ema support to bcn templating John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-27 13:10 ` Johannes Berg 2020-08-27 13:10 ` Johannes Berg 2020-08-28 3:20 ` Aloka Dixit 2020-08-28 3:20 ` Aloka Dixit 2020-08-12 15:00 ` [PATCH V3 5/9] ath11k: add a struct to pass parameters into ath11k_wmi_vdev_up John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 6/9] ath11k: add the multiple bssid WMI commands John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 7/9] ath11k: add multiple bssid support to device creation John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 8/9] ath11k: add EMA beacon support John Crispin 2020-08-12 15:00 ` John Crispin 2020-08-12 15:00 ` [PATCH V3 9/9] ath11k: set the multiple bssid hw flags and capabilities John Crispin 2020-08-12 15:00 ` John Crispin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6a4fa0a60089bb1565def2bca187b9c0afc36022.camel@sipsolutions.net \ --to=johannes@sipsolutions.net \ --cc=ath11k@lists.infradead.org \ --cc=john@phrozen.org \ --cc=linux-wireless@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.