Re: [PATCHv3 RESEND 08/11] mac80211: implement nan_change_conf

[Johannes Berg]

On Tue, 2016-03-29 at 12:35 +0300, Emmanuel Grumbach wrote:
> 
>   * @start_nan: join an existing nan cluster, or create a new one.
>   * @stop_nan: leave the nan cluster.
> + * @nan_change_conf: change nan configuration. The data in
> cfg80211_nan_conf
> + *   contains full new configuration and changes specify which
> parameters

@changes I guess? at least you should be consistent, even if there's no
perfectly correct syntax here.

Also please specify where the change flags come from.

I'm also not sure that the description is actually correct? How can
both "contains [the] full new configuration" and "changes speicfy which
parameters" be correct? You have a full new configuration but still
want to indicate the changes?

> + *   are changed with respect to the last nan config.
> 

Also, more nan vs. NAN, I'm sure I didn't comment on them all.

> + int (*nan_change_conf)(struct ieee80211_hw *hw,
> +    struct ieee80211_vif *vif,
> +    struct cfg80211_nan_conf *conf, u8
> changes);

An earlier patch in your series used u32, why u8 here?

> + memcpy(>u.nan.nan_conf, conf, sizeof(sdata-
> >u.nan.nan_conf));

why not use struct assignment?

sdata->u.nan.conf = conf;

> + memcpy(>u.nan.nan_conf, _conf,
> +    sizeof(sdata->u.nan.nan_conf));

ditto

> +/**
> + * struct ieee80211_if_nan - NAN state
> + *
> + * @nan_conf: current nan configuration
> + */
> +struct ieee80211_if_nan {
> + struct cfg80211_nan_conf nan_conf;
> +};

There's no point in calling it nan_conf since it's within a nan struct
and then later called "nan.nan_conf"...

> + __field(u32, changes)

You're not being very consistent with the type of the "changes"
parameter :)

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 08/11] mac80211: implement nan_change_conf

[Emmanuel Grumbach]

From: Andrei Otcheretianski 

Implement nan_change_conf callback which allows to change current
nan configuration (master preference and dual band operation).
Store the current nan configuration in sdata, so it can be used
both to provide the driver the updated configuration with changes
and also it will be used in hw reconfig flows in next patches.

Signed-off-by: Andrei Otcheretianski 
Signed-off-by: Emmanuel Grumbach 
---
 include/net/mac80211.h |  6 ++
 net/mac80211/cfg.c | 33 +
 net/mac80211/driver-ops.h  | 21 +
 net/mac80211/ieee80211_i.h | 10 ++
 net/mac80211/trace.h   | 31 +++
 5 files changed, 101 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 12860de..011f979 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3361,6 +3361,9 @@ enum ieee80211_reconfig_type {
  *
  * @start_nan: join an existing nan cluster, or create a new one.
  * @stop_nan: leave the nan cluster.
+ * @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.
  */
 struct ieee80211_ops {
void (*tx)(struct ieee80211_hw *hw,
@@ -3605,6 +3608,9 @@ struct ieee80211_ops {
 struct cfg80211_nan_conf *conf);
int (*stop_nan)(struct ieee80211_hw *hw,
struct ieee80211_vif *vif);
+   int (*nan_change_conf)(struct ieee80211_hw *hw,
+  struct ieee80211_vif *vif,
+  struct cfg80211_nan_conf *conf, u8 changes);
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a9d66b4..9215232 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -150,6 +150,8 @@ static int ieee80211_start_nan(struct wiphy *wiphy,
if (ret)
ieee80211_sdata_stop(sdata);
 
+   memcpy(>u.nan.nan_conf, conf, sizeof(sdata->u.nan.nan_conf));
+
return ret;
 }
 
@@ -162,6 +164,36 @@ static void ieee80211_stop_nan(struct wiphy *wiphy,
ieee80211_sdata_stop(sdata);
 }
 
+static int ieee80211_nan_change_conf(struct wiphy *wiphy,
+struct wireless_dev *wdev,
+struct cfg80211_nan_conf *conf,
+u32 changes)
+{
+   struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
+   struct cfg80211_nan_conf new_conf;
+   int ret = 0;
+
+   if (sdata->vif.type != NL80211_IFTYPE_NAN)
+   return -EOPNOTSUPP;
+
+   if (!ieee80211_sdata_running(sdata))
+   return -ENETDOWN;
+
+   memcpy(_conf, >u.nan.nan_conf, sizeof(new_conf));
+   if (changes & CFG80211_NAN_CONF_CHANGED_PREF)
+   new_conf.master_pref = conf->master_pref;
+
+   if (changes & CFG80211_NAN_CONF_CHANGED_DUAL)
+   new_conf.dual = conf->dual;
+
+   ret = drv_nan_change_conf(sdata->local, sdata, _conf, changes);
+   if (!ret)
+   memcpy(>u.nan.nan_conf, _conf,
+  sizeof(sdata->u.nan.nan_conf));
+
+   return ret;
+}
+
 static int ieee80211_set_noack_map(struct wiphy *wiphy,
  struct net_device *dev,
  u16 noack_map)
@@ -3473,4 +3505,5 @@ const struct cfg80211_ops mac80211_config_ops = {
.del_tx_ts = ieee80211_del_tx_ts,
.start_nan = ieee80211_start_nan,
.stop_nan = ieee80211_stop_nan,
+   .nan_change_conf = ieee80211_nan_change_conf,
 };
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 3352ce2..3c84fbd 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1206,4 +1206,25 @@ static inline void drv_stop_nan(struct ieee80211_local 
*local,
trace_drv_return_void(local);
 }
 
+static inline int drv_nan_change_conf(struct ieee80211_local *local,
+  struct ieee80211_sub_if_data *sdata,
+  struct cfg80211_nan_conf *conf,
+  u32 changes)
+{
+   int ret;
+
+   might_sleep();
+   check_sdata_in_driver(sdata);
+
+   if (!local->ops->nan_change_conf)
+   return -EOPNOTSUPP;
+
+   trace_drv_nan_change_conf(local, sdata, conf, changes);
+   ret = local->ops->nan_change_conf(>hw, >vif, conf,
+ changes);
+   trace_drv_return_int(local, ret);
+
+   return ret;
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c6830fb..f985ed2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -813,6 +813,15 @@ struct txq_info {