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@xxxxxxxxxxx>
> ---
> 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
