mbox series

[v4,0/2] Add FILS discovery support

Message ID 20200618050427.5891-1-alokad@codeaurora.org
Headers show
Series Add FILS discovery support | expand

Message

Aloka Dixit June 18, 2020, 5:04 a.m. UTC 
This patchset adds support for FILS discovery transmission as per
IEEE Std 802.11ai-2016.

v4: Removed separate flag 'enabled' and data related to beacon
response window and omit replicated probe response features.

Aloka Dixit (2):
  nl80211: Add FILS discovery support
  mac80211: Add FILS discovery support

 include/net/cfg80211.h       | 19 +++++++++++++++
 include/net/mac80211.h       | 27 +++++++++++++++++++++
 include/uapi/linux/nl80211.h | 36 ++++++++++++++++++++++++++++
 net/mac80211/cfg.c           | 41 ++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h   |  7 ++++++
 net/mac80211/tx.c            | 25 ++++++++++++++++++++
 net/wireless/nl80211.c       | 46 ++++++++++++++++++++++++++++++++++++
 7 files changed, 201 insertions(+)

Comments

Johannes Berg July 30, 2020, 2:43 p.m. UTC | #1 
On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:
> + * @NL80211_FILS_DISCOVERY_TMPL: Optional FILS discovery template.

> + *	It has contents of IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery frame

> + *	(Figure 9-687a).


Is that

"It has (contents of ... FILS discovery frame) ..."

or

"It has contents of (... FILS discovery frame) ..."?

I mean, is that with or without headers? The wording doesn't seem
entirely clear to me.

OTOH, if it's with headers, how could it be optional? In fact, either
way, how is it optional?

> +static int nl80211_parse_fils_discovery(struct nlattr *attrs,

> +					struct cfg80211_ap_settings *params)

> +{

> +	struct nlattr *tmpl;

> +	struct nlattr *tb[NL80211_FILS_DISCOVERY_MAX + 1];

> +	int ret;

> +	struct cfg80211_fils_discovery *fd = &params->fils_discovery;

> +

> +	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_MAX, attrs,

> +			       fils_discovery_policy, NULL);

> +	if (ret)

> +		return ret;

> +

> +	if (!tb[NL80211_FILS_DISCOVERY_INT_MIN] ||

> +	    !tb[NL80211_FILS_DISCOVERY_INT_MAX])

> +		return -EINVAL;

> +

> +	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MIN]);

> +	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MAX]);

> +

> +	tmpl = tb[NL80211_FILS_DISCOVERY_TMPL];

> +	if (tmpl) {

> +		fd->tmpl = nla_data(tmpl);

> +		fd->tmpl_len = nla_len(tmpl);


And if it's with headers, it should have some kind of minimum length
too? You've only put a maximum length into the policy.

(Which reminds me I wanted to have an NLA_POLICY_RANGE(NLA_BINARY, min,
max) but haven't done that yet ...)

johannes
Aloka Dixit July 30, 2020, 9:17 p.m. UTC | #2 
On 2020-07-30 07:43, Johannes Berg wrote:
> On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote:

>> + * @NL80211_FILS_DISCOVERY_TMPL: Optional FILS discovery template.

>> + *	It has contents of IEEE Std 802.11ai-2016 9.6.8.36 FILS discovery 

>> frame

>> + *	(Figure 9-687a).

> 

> Is that

> 

> "It has (contents of ... FILS discovery frame) ..."

> 

> or

> 

> "It has contents of (... FILS discovery frame) ..."?

> 

> I mean, is that with or without headers? The wording doesn't seem

> entirely clear to me.

> 

> OTOH, if it's with headers, how could it be optional? In fact, either

> way, how is it optional?

> 


Template has management frame headers as well. Will change the wording 
accordingly.
I made the template optional because FILS discovery may or may not be 
offloaded to FW.
Another way would be to make it mandatory here.

>> +static int nl80211_parse_fils_discovery(struct nlattr *attrs,

>> +					struct cfg80211_ap_settings *params)

>> +{

>> +	struct nlattr *tmpl;

>> +	struct nlattr *tb[NL80211_FILS_DISCOVERY_MAX + 1];

>> +	int ret;

>> +	struct cfg80211_fils_discovery *fd = &params->fils_discovery;

>> +

>> +	ret = nla_parse_nested(tb, NL80211_FILS_DISCOVERY_MAX, attrs,

>> +			       fils_discovery_policy, NULL);

>> +	if (ret)

>> +		return ret;

>> +

>> +	if (!tb[NL80211_FILS_DISCOVERY_INT_MIN] ||

>> +	    !tb[NL80211_FILS_DISCOVERY_INT_MAX])

>> +		return -EINVAL;

>> +

>> +	fd->min_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MIN]);

>> +	fd->max_interval = nla_get_u32(tb[NL80211_FILS_DISCOVERY_INT_MAX]);

>> +

>> +	tmpl = tb[NL80211_FILS_DISCOVERY_TMPL];

>> +	if (tmpl) {

>> +		fd->tmpl = nla_data(tmpl);

>> +		fd->tmpl_len = nla_len(tmpl);

> 

> And if it's with headers, it should have some kind of minimum length

> too? You've only put a maximum length into the policy.

> 

> (Which reminds me I wanted to have an NLA_POLICY_RANGE(NLA_BINARY, min,

> max) but haven't done that yet ...)

> 


Yeah, I looked through existing examples for NLA_BINARY, those provide 
only the higher bound for length.
But I can modify it to range once that is added.

> johannes
Johannes Berg July 30, 2020, 9:22 p.m. UTC | #3 
On Thu, 2020-07-30 at 14:17 -0700, Aloka Dixit wrote:

> > OTOH, if it's with headers, how could it be optional? In fact, either

> > way, how is it optional?

> > 

> 

> Template has management frame headers as well. Will change the wording 

> accordingly.


OK.

> I made the template optional because FILS discovery may or may not be 

> offloaded to FW.


But how would anyone know? Try without it, and then try again if that
fails? Would it fail? I mean, you also said it was required at least for
6 GHz, so wouldn't userspace be better off always giving it - and then
we should probably make it mandatory so it doesn't fall into the trap?

However - and here that's my ignorance speaking - can it really be
offloaded? I mean, is everything in there completely determined by the
beacon already, and so you have no choice in how to build it? Or how
does that work?

> Yeah, I looked through existing examples for NLA_BINARY, those provide 

> only the higher bound for length.


Yeah, no way to do anything else right now. But you should have a lower
bound in the code, I think.

> But I can modify it to range once that is added.


Later maybe :)

johannes
Aloka Dixit July 30, 2020, 9:53 p.m. UTC | #4 
On 2020-07-30 14:22, Johannes Berg wrote:
> On Thu, 2020-07-30 at 14:17 -0700, Aloka Dixit wrote:

> 

>> > OTOH, if it's with headers, how could it be optional? In fact, either

>> > way, how is it optional?

>> >

>> 

>> Template has management frame headers as well. Will change the wording

>> accordingly.

> 

> OK.

> 

>> I made the template optional because FILS discovery may or may not be

>> offloaded to FW.

> 

> But how would anyone know? Try without it, and then try again if that

> fails? Would it fail? I mean, you also said it was required at least 

> for

> 6 GHz, so wouldn't userspace be better off always giving it - and then

> we should probably make it mandatory so it doesn't fall into the trap?

> 


If the template is not provided, FW keeps sending event to get it.
But as my ath11k driver code is limited to 6GHz, it already throws error 
if template not provided.
Yes, in general it will be better to make it mandatory, I will do it in 
next version.

> However - and here that's my ignorance speaking - can it really be

> offloaded? I mean, is everything in there completely determined by the

> beacon already, and so you have no choice in how to build it? Or how

> does that work?

> 


Yes, the frame parameters are fixed except for the timestamp which FW is 
expected to fill.

>> Yeah, I looked through existing examples for NLA_BINARY, those provide

>> only the higher bound for length.

> 

> Yeah, no way to do anything else right now. But you should have a lower

> bound in the code, I think.

> 


Okay.

>> But I can modify it to range once that is added.

> 

> Later maybe :)

> 

> johannes