| Message ID | 20200618050427.5891-3-alokad@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Add FILS discovery support | expand |
On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote: > +++ b/net/mac80211/cfg.c > @@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, > return 0; > } > > +static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata, > + struct cfg80211_fils_discovery *params) > +{ > + struct fils_discovery_data *new, *old = NULL; > + struct ieee80211_fils_discovery *fd; > + > + fd = &sdata->vif.bss_conf.fils_discovery; > + fd->min_interval = params->min_interval; > + fd->max_interval = params->max_interval; > + > + if (!params->tmpl || !params->tmpl_len) /* Optional template */ > + return 0; Now I'm even more confused. If the template is optional, then if it's not given it doesn't mean *everything* should be ignored, does it? What would be the point of that? OTOH, if the template isn't there, what would you do? But it still doesn't make sense - if no template means you shouldn't do anything then that doesn't mean the template should be optional, that just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY attribute when it doesn't want anything to be done? So it seems to me that something doesn't match. Either the template is truly optional and then this shouldn't just return success, or the template isn't actually optional? > + err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery); > + if (err < 0) { > + ieee80211_vif_release_channel(sdata); > + return err; Is there no goto label for this error case? > +struct fils_discovery_data { > + struct rcu_head rcu_head; > + int len; > + u8 data[0]; please use [] not [0]. > +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif) > +{ > + struct sk_buff *skb = NULL; > + struct fils_discovery_data *tmpl = NULL; > + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); > + > + if (sdata->vif.type != NL80211_IFTYPE_AP) > + return NULL; > + > + rcu_read_lock(); > + tmpl = rcu_dereference(sdata->u.ap.fils_discovery); > + if (!tmpl) { > + rcu_read_unlock(); > + return NULL; > + } > + > + skb = dev_alloc_skb(tmpl->len); > + if (skb) > + skb_put_data(skb, tmpl->data, tmpl->len); You should consider the headroom that the driver may have requested. johannes
On 2020-07-30 07:47, Johannes Berg wrote: > On Wed, 2020-06-17 at 22:04 -0700, Aloka Dixit wrote: > >> +++ b/net/mac80211/cfg.c >> @@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct >> ieee80211_sub_if_data *sdata, >> return 0; >> } >> >> +static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data >> *sdata, >> + struct cfg80211_fils_discovery *params) >> +{ >> + struct fils_discovery_data *new, *old = NULL; >> + struct ieee80211_fils_discovery *fd; >> + >> + fd = &sdata->vif.bss_conf.fils_discovery; >> + fd->min_interval = params->min_interval; >> + fd->max_interval = params->max_interval; >> + >> + if (!params->tmpl || !params->tmpl_len) /* Optional template */ >> + return 0; > > Now I'm even more confused. If the template is optional, then if it's > not given it doesn't mean *everything* should be ignored, does it? > > What would be the point of that? OTOH, if the template isn't there, > what > would you do? > > But it still doesn't make sense - if no template means you shouldn't do > anything then that doesn't mean the template should be optional, that > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY > attribute when it doesn't want anything to be done? > > So it seems to me that something doesn't match. Either the template is > truly optional and then this shouldn't just return success, or the > template isn't actually optional? > Everything is not ignored, I set the minimum and maximum interval values before checking for the template so that those are accepted even if template isn't present. For 6GHz, template is required, at least for ath11k driver. But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded to FW. We can make the template mandatory instead and then the respective drivers will choose the handling. Please suggest. >> + err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery); >> + if (err < 0) { >> + ieee80211_vif_release_channel(sdata); >> + return err; > > Is there no goto label for this error case? > Existing function doesn't use goto labels for error cases, only return. >> +struct fils_discovery_data { >> + struct rcu_head rcu_head; >> + int len; >> + u8 data[0]; > > please use [] not [0]. > Will fix in next version. >> +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw >> *hw, >> + struct ieee80211_vif *vif) >> +{ >> + struct sk_buff *skb = NULL; >> + struct fils_discovery_data *tmpl = NULL; >> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); >> + >> + if (sdata->vif.type != NL80211_IFTYPE_AP) >> + return NULL; >> + >> + rcu_read_lock(); >> + tmpl = rcu_dereference(sdata->u.ap.fils_discovery); >> + if (!tmpl) { >> + rcu_read_unlock(); >> + return NULL; >> + } >> + >> + skb = dev_alloc_skb(tmpl->len); >> + if (skb) >> + skb_put_data(skb, tmpl->data, tmpl->len); > > You should consider the headroom that the driver may have requested. > I didn't understand this point, what would the driver request headroom for? Thanks > johannes
On Thu, 2020-07-30 at 14:00 -0700, Aloka Dixit wrote: > > > + fd = &sdata->vif.bss_conf.fils_discovery; > > > + fd->min_interval = params->min_interval; > > > + fd->max_interval = params->max_interval; > > > + > > > + if (!params->tmpl || !params->tmpl_len) /* Optional template */ > > > + return 0; > > > > Now I'm even more confused. If the template is optional, then if it's > > not given it doesn't mean *everything* should be ignored, does it? > > > > What would be the point of that? OTOH, if the template isn't there, > > what > > would you do? > > > > But it still doesn't make sense - if no template means you shouldn't do > > anything then that doesn't mean the template should be optional, that > > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY > > attribute when it doesn't want anything to be done? > > > > So it seems to me that something doesn't match. Either the template is > > truly optional and then this shouldn't just return success, or the > > template isn't actually optional? > > > > Everything is not ignored, I set the minimum and maximum interval values > before checking for the template so that those are accepted even if > template isn't present. Right, oops, missed that. > For 6GHz, template is required, at least for ath11k driver. > But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded to > FW. But ... now I'm still confused. If you *don't* offload it, how will it work? Will it all bubble up to hostapd and that will send the response? Does that work without any other changes? But then what would you need the min/max for? I guess I still don't understand it... I thought this was a periodic frame anyway like beacon, so how could you _not_ offload it? > We can make the template mandatory instead and then the respective > drivers will choose the handling. > Please suggest. I have no idea ... still trying to understand it. > > > + err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery); > > > + if (err < 0) { > > > + ieee80211_vif_release_channel(sdata); > > > + return err; > > > > Is there no goto label for this error case? > > > > Existing function doesn't use goto labels for error cases, only return. Maybe add one? Surely the release_channel() must alraedy exist there somewhere. > > > + skb = dev_alloc_skb(tmpl->len); > > > + if (skb) > > > + skb_put_data(skb, tmpl->data, tmpl->len); > > > > You should consider the headroom that the driver may have requested. > > > > I didn't understand this point, what would the driver request headroom > for? Whatever it wants for ... drivers are allowed request extra headroom (hw->extra_tx_headroom) and we generally honour that for every skb we build for the driver. johannes
On 2020-07-30 14:26, Johannes Berg wrote: > On Thu, 2020-07-30 at 14:00 -0700, Aloka Dixit wrote: > >> > > + fd = &sdata->vif.bss_conf.fils_discovery; >> > > + fd->min_interval = params->min_interval; >> > > + fd->max_interval = params->max_interval; >> > > + >> > > + if (!params->tmpl || !params->tmpl_len) /* Optional template */ >> > > + return 0; >> > >> > Now I'm even more confused. If the template is optional, then if it's >> > not given it doesn't mean *everything* should be ignored, does it? >> > >> > What would be the point of that? OTOH, if the template isn't there, >> > what >> > would you do? >> > >> > But it still doesn't make sense - if no template means you shouldn't do >> > anything then that doesn't mean the template should be optional, that >> > just means userspace shouldn't even put the NL80211_ATTR_FILS_DISCOVERY >> > attribute when it doesn't want anything to be done? >> > >> > So it seems to me that something doesn't match. Either the template is >> > truly optional and then this shouldn't just return success, or the >> > template isn't actually optional? >> > >> >> Everything is not ignored, I set the minimum and maximum interval >> values >> before checking for the template so that those are accepted even if >> template isn't present. > > Right, oops, missed that. > >> For 6GHz, template is required, at least for ath11k driver. >> But for 2.4GHz and 5GHz FILS discovery transmission is not offloaded >> to >> FW. > > But ... now I'm still confused. > > If you *don't* offload it, how will it work? Will it all bubble up to > hostapd and that will send the response? Does that work without any > other changes? > > But then what would you need the min/max for? I guess I still don't > understand it... I thought this was a periodic frame anyway like > beacon, > so how could you _not_ offload it? > Min and max intervals are used to decide if a FILS discovery frame should be sent at all when respective timers expires. Depending on how close that time is to the next beacons, the device may just send the beacon instead. In lower bands, for non-offloaded case, FW will send events asking for the frame until it gets one. Whether that should go all the way to hostapd or should the driver itself handle it remains to be seen. My current focus is only 6GHz, but didn't want to restrict kernel implementation so moved 6GHz related checks to the driver instead. All in all, making the template mandatory will be safer so that the driver will always have one if required. >> We can make the template mandatory instead and then the respective >> drivers will choose the handling. >> Please suggest. > > I have no idea ... still trying to understand it. > >> > > + err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery); >> > > + if (err < 0) { >> > > + ieee80211_vif_release_channel(sdata); >> > > + return err; >> > >> > Is there no goto label for this error case? >> > >> >> Existing function doesn't use goto labels for error cases, only >> return. > > Maybe add one? Surely the release_channel() must alraedy exist there > somewhere. > Okay. >> > > + skb = dev_alloc_skb(tmpl->len); >> > > + if (skb) >> > > + skb_put_data(skb, tmpl->data, tmpl->len); >> > >> > You should consider the headroom that the driver may have requested. >> > >> >> I didn't understand this point, what would the driver request headroom >> for? > > Whatever it wants for ... drivers are allowed request extra headroom > (hw->extra_tx_headroom) and we generally honour that for every skb we > build for the driver. > I will look into other instances to understand this requirement. > johannes
On Thu, 2020-07-30 at 15:08 -0700, Aloka Dixit wrote: > Min and max intervals are used to decide if a FILS discovery frame > should be sent at all when respective timers expires. > Depending on how close that time is to the next beacons, the device may > just send the beacon instead. Right, OK. > In lower bands, for non-offloaded case, FW will send events asking for > the frame until it gets one. Aha. > Whether that should go all the way to hostapd or should the driver > itself handle it remains to be seen. I don't see why it should go up - the driver can have the template and answer that? I mean, we already push the template down. > My current focus is only 6GHz, but didn't want to restrict kernel > implementation so moved 6GHz related checks to the driver instead. It might still make sense to have a bitmap of where FILS discovery is supported, if it's different for different bands? Unless of course you think that a given device will always support FILS discovery on all bands, you just haven't implemented it yet for all bands - but will complete it before really enabling it in the driver? > All in all, making the template mandatory will be safer so that the > driver will always have one if required. Agree. Thanks! johannes
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 11d5610d2ad5..87d9570497fe 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -317,6 +317,7 @@ struct ieee80211_vif_chanctx_switch { * @BSS_CHANGED_TWT: TWT status changed * @BSS_CHANGED_HE_OBSS_PD: OBSS Packet Detection status changed. * @BSS_CHANGED_HE_BSS_COLOR: BSS Color has changed + * @BSS_CHANGED_FILS_DISCOVERY: FILS discovery status changed. * */ enum ieee80211_bss_change { @@ -350,6 +351,7 @@ enum ieee80211_bss_change { BSS_CHANGED_TWT = 1<<27, BSS_CHANGED_HE_OBSS_PD = 1<<28, BSS_CHANGED_HE_BSS_COLOR = 1<<29, + BSS_CHANGED_FILS_DISCOVERY = 1<<30, /* when adding here, make sure to change ieee80211_reconfig */ }; @@ -490,6 +492,18 @@ struct ieee80211_ftm_responder_params { size_t civicloc_len; }; +/** + * struct ieee80211_fils_discovery - FILS discovery parameters from + * IEEE Std 802.11ai-2016, Annex C.3 MIB detail. + * + * @min_interval: Minimum packet interval in TUs (0 - 10000) + * @max_interval: Maximum packet interval in TUs (0 - 10000) + */ +struct ieee80211_fils_discovery { + u32 min_interval; + u32 max_interval; +}; + /** * struct ieee80211_bss_conf - holds the BSS's changing parameters * @@ -607,6 +621,7 @@ struct ieee80211_ftm_responder_params { * @he_oper: HE operation information of the AP we are connected to * @he_obss_pd: OBSS Packet Detection parameters. * @he_bss_color: BSS coloring settings, if BSS supports HE + * @fils_discovery: FILS discovery configuration */ struct ieee80211_bss_conf { const u8 *bssid; @@ -674,6 +689,7 @@ struct ieee80211_bss_conf { } he_oper; struct ieee80211_he_obss_pd he_obss_pd; struct cfg80211_he_bss_color he_bss_color; + struct ieee80211_fils_discovery fils_discovery; }; /** @@ -6558,4 +6574,15 @@ u32 ieee80211_calc_tx_airtime(struct ieee80211_hw *hw, */ bool ieee80211_set_hw_80211_encap(struct ieee80211_vif *vif, bool enable); +/** + * ieee80211_get_fils_discovery_tmpl - Get FILS discovery template. + * @hw: pointer obtained from ieee80211_alloc_hw(). + * @vif: &struct ieee80211_vif pointer from the add_interface callback. + * + * The driver is responsible for freeing the returned skb. + * + * Return: FILS discovery template. %NULL on error. + */ +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw, + struct ieee80211_vif *vif); #endif /* MAC80211_H */ diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 9b360544ad6f..dae054b769b5 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -837,6 +837,33 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, return 0; } +static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata, + struct cfg80211_fils_discovery *params) +{ + struct fils_discovery_data *new, *old = NULL; + struct ieee80211_fils_discovery *fd; + + fd = &sdata->vif.bss_conf.fils_discovery; + fd->min_interval = params->min_interval; + fd->max_interval = params->max_interval; + + if (!params->tmpl || !params->tmpl_len) /* Optional template */ + return 0; + + old = sdata_dereference(sdata->u.ap.fils_discovery, sdata); + new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL); + if (!new) + return -ENOMEM; + new->len = params->tmpl_len; + memcpy(new->data, params->tmpl, params->tmpl_len); + rcu_assign_pointer(sdata->u.ap.fils_discovery, new); + + if (old) + kfree_rcu(old, rcu_head); + + return 0; +} + static int ieee80211_set_ftm_responder_params( struct ieee80211_sub_if_data *sdata, const u8 *lci, size_t lci_len, @@ -1103,6 +1130,14 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, } changed |= err; + err = ieee80211_set_fils_discovery(sdata, ¶ms->fils_discovery); + if (err < 0) { + ieee80211_vif_release_channel(sdata); + return err; + } else if (err == 0) { + changed |= BSS_CHANGED_FILS_DISCOVERY; + } + err = drv_start_ap(sdata->local, sdata); if (err) { old = sdata_dereference(sdata->u.ap.beacon, sdata); @@ -1158,6 +1193,7 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev) struct ieee80211_local *local = sdata->local; struct beacon_data *old_beacon; struct probe_resp *old_probe_resp; + struct fils_discovery_data *old_fils_discovery; struct cfg80211_chan_def chandef; sdata_assert_lock(sdata); @@ -1166,6 +1202,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev) if (!old_beacon) return -ENOENT; old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata); + old_fils_discovery = sdata_dereference(sdata->u.ap.fils_discovery, + sdata); /* abort any running channel switch */ mutex_lock(&local->mtx); @@ -1189,9 +1227,12 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev) /* remove beacon and probe response */ RCU_INIT_POINTER(sdata->u.ap.beacon, NULL); RCU_INIT_POINTER(sdata->u.ap.probe_resp, NULL); + RCU_INIT_POINTER(sdata->u.ap.fils_discovery, NULL); kfree_rcu(old_beacon, rcu_head); if (old_probe_resp) kfree_rcu(old_probe_resp, rcu_head); + if (old_fils_discovery) + kfree_rcu(old_fils_discovery, rcu_head); kfree(sdata->vif.bss_conf.ftmr_params); sdata->vif.bss_conf.ftmr_params = NULL; diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index ec1a71ac65f2..479c1691630b 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -272,6 +272,12 @@ struct probe_resp { u8 data[]; }; +struct fils_discovery_data { + struct rcu_head rcu_head; + int len; + u8 data[0]; +}; + struct ps_data { /* yes, this looks ugly, but guarantees that we can later use * bitmap_empty :) @@ -287,6 +293,7 @@ struct ps_data { struct ieee80211_if_ap { struct beacon_data __rcu *beacon; struct probe_resp __rcu *probe_resp; + struct fils_discovery_data __rcu *fils_discovery; /* to be used after channel switch. */ struct cfg80211_beacon_data *next_beacon; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index e9ce658141f5..9c1044782561 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -4998,6 +4998,31 @@ struct sk_buff *ieee80211_proberesp_get(struct ieee80211_hw *hw, } EXPORT_SYMBOL(ieee80211_proberesp_get); +struct sk_buff *ieee80211_get_fils_discovery_tmpl(struct ieee80211_hw *hw, + struct ieee80211_vif *vif) +{ + struct sk_buff *skb = NULL; + struct fils_discovery_data *tmpl = NULL; + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); + + if (sdata->vif.type != NL80211_IFTYPE_AP) + return NULL; + + rcu_read_lock(); + tmpl = rcu_dereference(sdata->u.ap.fils_discovery); + if (!tmpl) { + rcu_read_unlock(); + return NULL; + } + + skb = dev_alloc_skb(tmpl->len); + if (skb) + skb_put_data(skb, tmpl->data, tmpl->len); + rcu_read_unlock(); + return skb; +} +EXPORT_SYMBOL(ieee80211_get_fils_discovery_tmpl); + struct sk_buff *ieee80211_pspoll_get(struct ieee80211_hw *hw, struct ieee80211_vif *vif) {
This patch adds mac80211 support to configure FILS discovery transmission. Changes include functions to store and retrieve FILS discovery template, minimum and maximum packet intervals. Signed-off-by: Aloka Dixit <alokad@codeaurora.org> --- v4: Removed attributes unrelated to FILS discovery - bcn_resp_win, omit_replicate_probe_resp and member named enabled in struct ieee80211_fils_discovery. include/net/mac80211.h | 27 +++++++++++++++++++++++++ net/mac80211/cfg.c | 41 ++++++++++++++++++++++++++++++++++++++ net/mac80211/ieee80211_i.h | 7 +++++++ net/mac80211/tx.c | 25 +++++++++++++++++++++++ 4 files changed, 100 insertions(+)