Re: [PATCHv3 RESEND 03/11] cfg80211: add add_nan_func / rm_nan_func

[Johannes Berg]

> + NL80211_ATTR_NAN_FUNC_INST_ID,

I don't see why this has to be a top-level attribute.

> +enum nl80211_nan_func_attributes {
> + __NL80211_NAN_FUNC_INVALID,
> + NL80211_NAN_FUNC_TYPE,

You can easily move it into here, and 

> + /* propagate the instance id and cookie to userspace  */
> + if (WARN_ON(nla_put_u8(msg, NL80211_ATTR_NAN_FUNC_INST_ID,
> +    func.instance_id) ||
> + nla_put_u64(msg, NL80211_ATTR_COOKIE,
> func.cookie))) {

use the appropriate nesting here.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 RESEND 03/11] cfg80211: add add_nan_func / rm_nan_func

[Johannes Berg]

On Tue, 2016-03-29 at 12:35 +0300, Emmanuel Grumbach wrote:

> + * @cookie: user defined cookie (will be returned with
> notifications)

Didn't we change it to not be user defined?

> + * @NL80211_NAN_FUNC_TTL: number of DWs this function should stay
> active. 0 is
> + *   equivalent to no TTL at all. This is a u32.

I think it should rather be "if the attribute is not specified, no TTL
is used", with 0 being an invalid value.

> + * @NL80211_NAN_FUNC_RX_MATCH_FILTER: Receive Matching filter. This
> is a nested
> + *   attribute. It is a list of binary values.

That probably needs to say what kind if "binary values"?

> + * @NL80211_NAN_FUNC_TX_MATCH_FILTER: Transmit Matching filter. This
> is a
> + *   nested attribute. It is a list of binary values.

Ditto.

> + * @NL80211_NAN_SRF_INCLUDE: true if the include bit of the SRF set.
> + *   This is a flag.
> + * @NL80211_NAN_SRF_TYPE_BF: true if the SRF is a Bloom Filter SRF.
> If false
> + *   the SRF will be _ATTR_MAC_ADDRS. This is a flag.

"true" and "false" aren't really states for a flag, it can be
"specified" or "not specified", or maybe "present" and "not present".

> + * @NL80211_NAN_SRF_BF: Bloom Filter. Relevant and mandatory if
> + *   _NAN_SRF_TYPE_BF is true. This attribute is
> binary.

Likewise.
However, why do you even need two attributes? It doesn't seem relevant
to specify the NAN_SRF_BF attribute when NAN_SRF_TYPE_BF isn't set?

> + * @NL80211_NAN_SRF_BF_IDX: index of the Bloom Filter. Relevant and
> + *   mandatory if _NAN_SRF_TYPE_BF is true. This is a
> u8.

Likewise - what do you need the SRF_TYPE_BF flag for at all?

> + * @NL80211_NAN_SRF_MAC_ADDRS: list of MAC addresses for the SRF.
> Relevant and
> + *   mandatory if _NAN_SRF_TYPE_BF is false. This is a
> nested
> + *   attribute. Each nested attribute is a MAC address.

And this is obviously the opposite - so both SRF_MAC_ADDRS and
SRF_BR/BF_IDX can't be specified together. No need for the flag?


> + if (tx) {
> + func->num_tx_filters = (u8)n_entries;
> + func->tx_filters = filter;
> + } else {
> + func->num_rx_filters = (u8)n_entries;
> + func->rx_filters = filter;

No need for those casts.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 RESEND 03/11] cfg80211: add add_nan_func / rm_nan_func

[Emmanuel Grumbach]

A NAN function can be either publish, subscribe or follow
up. Make all the necessary verifications and just pass the
request to the driver.

Signed-off-by: Andrei Otcheretianski 
Signed-off-by: Emmanuel Grumbach 
---
 include/net/cfg80211.h   |  79 +++
 include/uapi/linux/nl80211.h | 153 -
 net/wireless/core.c  |   3 +-
 net/wireless/nl80211.c   | 313 +++
 net/wireless/rdev-ops.h  |  21 +++
 net/wireless/trace.h |  40 ++
 6 files changed, 607 insertions(+), 2 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 69253f5..2dc63ec 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2272,6 +2272,73 @@ struct cfg80211_nan_conf {
 };
 
 /**
+ * struct cfg80211_nan_func_filter - a NAN function Rx / Tx filter
+ *
+ * @filter: the content of the filter
+ * @len: the length of the filter
+ */
+struct cfg80211_nan_func_filter {
+   const u8 *filter;
+   u8 len;
+};
+
+/**
+ * struct cfg80211_nan_func - a NAN function
+ *
+ * @type:  nl80211_nan_function_type
+ * @service_id: the service ID of the function
+ * @publish_type: _nan_publish_type
+ * @close_range: if true, the range should be limited. Threshold is
+ * implementation specific.
+ * @publish_bcast: if true, the solicited publish should be broadcasted
+ * @subscribe_active: if true, the subscribe is active
+ * @followup_id: the instance ID for follow up
+ * @followup_reqid: the requestor instance ID for follow up
+ * @followup_dest: MAC address of the recipient of the follow up
+ * @ttl: time to live counter in DW.
+ * @serv_spec_info: Service Specific Info
+ * @serv_spec_info_len: Service Specific Info length
+ * @srf_include: if true, SRF is inclusive
+ * @srf_bf: Bloom Filter
+ * @srf_bf_len: Bloom Filter length
+ * @srf_bf_idx: Bloom Filter index
+ * @srf_macs: SRF MAC addresses
+ * @srf_num_macs: number of MAC addresses in SRF
+ * @rx_filters: rx filters that are matched with corresponding peer's tx_filter
+ * @tx_filters: filters that should be transmitted in the SDF.
+ * @num_rx_filters: length of _filters.
+ * @num_tx_filters: length of _filters.
+ * @instance_id: driver allocated id of the function.
+ * @cookie: user defined cookie (will be returned with notifications)
+ */
+struct cfg80211_nan_func {
+   enum nl80211_nan_function_type type;
+   u8 service_id[NL80211_NAN_FUNC_SERVICE_ID_LEN];
+   u8 publish_type;
+   bool close_range;
+   bool publish_bcast;
+   bool subscribe_active;
+   u8 followup_id;
+   u8 followup_reqid;
+   struct mac_address followup_dest;
+   u32 ttl;
+   const u8 *serv_spec_info;
+   u8 serv_spec_info_len;
+   bool srf_include;
+   const u8 *srf_bf;
+   u8 srf_bf_len;
+   u8 srf_bf_idx;
+   struct mac_address *srf_macs;
+   int srf_num_macs;
+   struct cfg80211_nan_func_filter *rx_filters;
+   struct cfg80211_nan_func_filter *tx_filters;
+   u8 num_tx_filters;
+   u8 num_rx_filters;
+   u8 instance_id;
+   u64 cookie;
+};
+
+/**
  * struct cfg80211_ops - backend description for wireless configuration
  *
  * This struct is registered by fullmac card drivers and/or wireless stacks
@@ -2546,6 +2613,11 @@ struct cfg80211_nan_conf {
  * peers must be on the base channel when the call completes.
  * @start_nan: Start the NAN interface.
  * @stop_nan: Stop the NAN interface.
+ * @add_nan_func: Add a nan function. Returns negative value on failure.
+ * The data in cfg80211_nan_func must not be referenced outside the
+ * scope of this call. The function assigns a unique instance_id in the
+ * provided @nan_func.
+ * @rm_nan_func: Remove a nan function.
  */
 struct cfg80211_ops {
int (*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
@@ -2814,6 +2886,10 @@ struct cfg80211_ops {
int (*start_nan)(struct wiphy *wiphy, struct wireless_dev *wdev,
 struct cfg80211_nan_conf *conf);
void(*stop_nan)(struct wiphy *wiphy, struct wireless_dev *wdev);
+   int (*add_nan_func)(struct wiphy *wiphy, struct wireless_dev *wdev,
+   struct cfg80211_nan_func *nan_func);
+   void(*rm_nan_func)(struct wiphy *wiphy, struct wireless_dev *wdev,
+  u64 cookie);
 };
 
 /*
@@ -3236,6 +3312,7 @@ struct wiphy_vendor_command {
  * @bss_select_support: bitmask indicating the BSS selection criteria supported
  * by the driver in the .connect() callback. The bit position maps to the
  * attribute indices defined in  nl80211_bss_select_attr.
+ * @cookie_counter: unique generic cookie counter, used to identify objects.
  */
 struct wiphy {
/* assign these fields before you register the wiphy */
@@ -3360,6 +3437,8 @@ struct wiphy {
 
u32 bss_select_support;
 
+   u64