Re: [PATCHv3 RESEND 09/11] mac80211: Implement add_nan_func and rm_nan_func
On Tue, 2016-03-29 at 12:35 +0300, Emmanuel Grumbach wrote:
> + * @rm_nan_func: Remove a nan function. The driver must call
> + * ieee80211_nan_func_terminated() with
> + * NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon
> removal.
bad indentation.
Also here: nan -> NAN.
> + /* Only set max_nan_de_entries as available to honor the
> device's
> + * limitations
> + */
> + bitmap_set(sdata->u.nan.func_ids, 1,
> + sdata->local->hw.max_nan_de_entries);
That doesn't make a lot of sense to me. What are you trying to do?
> + inst_id = find_first_bit(sdata->u.nan.func_ids,
> + IEEE80211_MAX_NAN_INSTANCE_ID + 1);
> + if (inst_id == IEEE80211_MAX_NAN_INSTANCE_ID + 1)
> + return -ENOBUFS;
Wouldn't you use max_nan_de_entries here instead of
MAX_NAN_INSTANCE_ID, after validating that the former is actually
smaller than or equal to the latter?
Also, the logic says that the variable should be called
"available_func_ids", although I'd prefer "used_func_ids" after
adjusting the logic according to the comments above.
> + cfg80211_clone_nan_func_members(&func->func, nan_func);
This is quite obviously missing error checking, but see the discussion
in the patch that introduced it.
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + clear_bit(inst_id, sdata->u.nan.func_ids);
> + list_add(&func->list, &sdata->u.nan.functions_list);
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> +
> + ret = drv_add_nan_func(sdata->local, sdata, nan_func);
> + if (ret) {
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + set_bit(inst_id, sdata->u.nan.func_ids);
> + list_del(&func->list);
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> +
> + cfg80211_free_nan_func_members(&func->func);
> + kfree(func);
> + }
> +
> + return ret;
> +}
> +static struct ieee80211_nan_func *
> +ieee80211_find_nan_func(struct ieee80211_sub_if_data *sdata, u8
> instance_id)
> +{
> + struct ieee80211_nan_func *func;
> +
> + lockdep_assert_held(&sdata->u.nan.func_lock);
> +
> + list_for_each_entry(func, &sdata->u.nan.functions_list,
> list) {
> + if (func->func.instance_id == instance_id)
> + return func;
> + }
> +
> + return NULL;
> +}
Arguably though, this whole thing just screams "idr" [1] and then you
don't even need the list_head in the cfg80211 struct.
[1] include/linux/idr.h
> +static struct ieee80211_nan_func *
> +ieee80211_find_nan_func_by_cookie(struct ieee80211_sub_if_data
> *sdata,
> + u64 cookie)
> +{
> + struct ieee80211_nan_func *func;
> +
> + lockdep_assert_held(&sdata->u.nan.func_lock);
> +
> + list_for_each_entry(func, &sdata->u.nan.functions_list,
> list) {
> + if (func->func.cookie == cookie)
> + return func;
> + }
> +
> + return NULL;
> +}
Although this might be more difficult then, but you always
have idr_for_each_entry().
> + case NL80211_IFTYPE_NAN:
> + /* clean all the functions */
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + list_for_each_entry_safe(func, tmp_func,
> + &sdata-
> >u.nan.functions_list, list) {
> + list_del(&func->list);
> + cfg80211_free_nan_func_members(&func->func);
> + kfree(func);
> + }
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> + break;
> case NL80211_IFTYPE_P2P_DEVICE:
> /* relies on synchronize_rcu() below */
> RCU_INIT_POINTER(local->p2p_sdata, NULL);
> /* fall through */
> - case NL80211_IFTYPE_NAN:
any particular reason you're moving the case label?
> default:
> cancel_work_sync(&sdata->work);
> /*
> @@ -1453,9 +1464,15 @@ static void ieee80211_setup_sdata(struct
> ieee80211_sub_if_data *sdata,
> case NL80211_IFTYPE_WDS:
> sdata->vif.bss_conf.bssid = NULL;
> break;
> + case NL80211_IFTYPE_NAN:
> + bitmap_zero(sdata->u.nan.func_ids,
> + IEEE80211_MAX_NAN_INSTANCE_ID + 1);
> + INIT_LIST_HEAD(&sdata->u.nan.functions_list);
> + spin_lock_init(&sdata->u.nan.func_lock);
> + sdata->vif.bss_conf.bssid = sdata->vif.addr;
> + break;
> case NL80211_IFTYPE_AP_VLAN:
> case NL80211_IFTYPE_P2P_DEVICE:
> - case NL80211_IFTYPE_NAN:
also here?
> + if (!local->hw.max_nan_de_entries)
> + local->hw.max_nan_de_entries =
> IEEE80211_MAX_NAN_INSTANCE_ID;
Need a max check also, I guess?
> +/* TODO: record more fields */
...
but isn't cfg80211 recording them anyway?
> +static int ieee80211_reconfig_nan(struct ieee80211_sub_if_data
> *sdata)
> +{
> + struct ieee80211_nan_func *func, *ftmp;
>
[PATCHv3 RESEND 09/11] mac80211: Implement add_nan_func and rm_nan_func
From: Andrei Otcheretianski
Implement add/rm_nan_func functions and handle nan function
termination notifications. Handle instance_id allocation for
nan functions and implement the reconfig flow.
Signed-off-by: Andrei Otcheretianski
Signed-off-by: Emmanuel Grumbach
---
include/net/mac80211.h | 31 ++
net/mac80211/cfg.c | 147 +
net/mac80211/driver-ops.h | 32 ++
net/mac80211/ieee80211_i.h | 15 +
net/mac80211/iface.c | 21 ++-
net/mac80211/main.c| 3 +
net/mac80211/trace.h | 53
net/mac80211/util.c| 55 -
8 files changed, 354 insertions(+), 3 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 011f979..a3cc08f 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2116,6 +2116,8 @@ enum ieee80211_hw_flags {
*
* @txq_ac_max_pending: maximum number of frames per AC pending in all txq
* entries for a vif.
+ * @max_nan_de_entries: maximum number of NAN DE functions supported by the
+ * device.
*/
struct ieee80211_hw {
struct ieee80211_conf conf;
@@ -2146,6 +2148,7 @@ struct ieee80211_hw {
u8 n_cipher_schemes;
const struct ieee80211_cipher_scheme *cipher_schemes;
int txq_ac_max_pending;
+ u8 max_nan_de_entries;
};
static inline bool _ieee80211_hw_check(struct ieee80211_hw *hw,
@@ -3364,6 +3367,12 @@ enum ieee80211_reconfig_type {
* @nan_change_conf: change nan configuration. The data in cfg80211_nan_conf
* contains full new configuration and changes specify which parameters
* are changed with respect to the last nan config.
+ * @add_nan_func: Add a nan function. Returns 0 on success. The data in
+ * cfg80211_nan_func must not be referenced outside the scope of
+ * this call.
+ * @rm_nan_func: Remove a nan function. The driver must call
+ * ieee80211_nan_func_terminated() with
+ * NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon removal.
*/
struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3611,6 +3620,12 @@ struct ieee80211_ops {
int (*nan_change_conf)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct cfg80211_nan_conf *conf, u8 changes);
+ int (*add_nan_func)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ const struct cfg80211_nan_func *nan_func);
+ void (*rm_nan_func)(struct ieee80211_hw *hw,
+ struct ieee80211_vif *vif,
+ u8 instance_id);
};
/**
@@ -5655,4 +5670,20 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw
*hw,
void ieee80211_txq_get_depth(struct ieee80211_txq *txq,
unsigned long *frame_cnt,
unsigned long *byte_cnt);
+
+/*
+ * ieee80211_nan_func_terminated - notify about NAN function termination.
+ *
+ * This function is used to notify mac80211 about nan function termination.
+ *
+ * @vif: &struct ieee80211_vif pointer from the add_interface callback.
+ * @inst_id: the local instance id
+ * @reason: termination reason (one of the NL80211_NAN_FUNC_TERM_REASON_*)
+ * @gfp: allocation flags
+ */
+void ieee80211_nan_func_terminated(struct ieee80211_vif *vif,
+ u8 inst_id,
+ enum nl80211_nan_func_term_reason reason,
+ gfp_t gfp);
+
#endif /* MAC80211_H */
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9215232..67b0d8a 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3,6 +3,7 @@
*
* Copyright 2006-2010 Johannes Berg
* Copyright 2013-2015 Intel Mobile Communications GmbH
+ * Copyright (C) 2015-2016 Intel Deutschland GmbH
*
* This file is GPLv2 as found in COPYING.
*/
@@ -152,6 +153,12 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
memcpy(&sdata->u.nan.nan_conf, conf, sizeof(sdata->u.nan.nan_conf));
+ /* Only set max_nan_de_entries as available to honor the device's
+* limitations
+*/
+ bitmap_set(sdata->u.nan.func_ids, 1,
+ sdata->local->hw.max_nan_de_entries);
+
return ret;
}
@@ -194,6 +201,107 @@ static int ieee80211_nan_change_conf(struct wiphy *wiphy,
return ret;
}
+static int ieee80211_add_nan_func(struct wiphy *wiphy,
+ struct wireless_dev *wdev,
+ struct cfg80211_nan_func *nan_func)
+{
+ struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
+ struct ieee80211_nan_func *func;
+ int ret;
+ int inst_id;
+
+ if (sdata->vif.type != NL80211_IFTYPE_NAN)
+ return -EOPNOTSUPP;
+
+ if (!ieee80211_sdata_running(sdata))
+ return -ENETDOWN;
+
+ inst_id = find_first
