RE: [PATCHv3 RESEND 01/11] cfg80211: add start / stop NAN commands
> -Original Message- > From: Malinen, Jouni [mailto:jo...@qca.qualcomm.com] > Sent: Wednesday, April 6, 2016 12:34 PM > To: Grumbach, Emmanuel <emmanuel.grumb...@intel.com> > Cc: johan...@sipsolutions.net; linux-wireless@vger.kernel.org; > Otcheretianski, Andrei <andrei.otcheretian...@intel.com> > Subject: Re: [PATCHv3 RESEND 01/11] cfg80211: add start / stop NAN > commands > > On Tue, Mar 29, 2016 at 12:34:59PM +0300, Emmanuel Grumbach wrote: > > Define several attributes that may be configured by user space when > > starting NAN functionality (master preference and dual band operation) > > > diff --git a/include/uapi/linux/nl80211.h > > b/include/uapi/linux/nl80211.h @@ -826,6 +826,16 @@ > > + * @NL80211_CMD_START_NAN: Start NAN operation, identified by its > > + * %NL80211_ATTR_WDEV interface. This interface must have been > previously > > + * created with %NL80211_CMD_NEW_INTERFACE. After it has been > started, the > > + * NAN interface will create or join a cluster. This command must have a > > + * valid %NL80211_ATTR_NAN_MASTER_PREF attribute and optional > > + * %NL80211_ATTR_NAN_DUAL attributes. > > > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > > > +static int nl80211_start_nan(struct sk_buff *skb, struct genl_info > > +*info) > > > + if (!info->attrs[NL80211_ATTR_NAN_MASTER_PREF]) > > + return -EINVAL; > > Why NL80211_ATTR_NAN_MASTER_PREF is mandatory? The spec suggests > to assume master preference value of 128 if not provided. Please see quoted > text from NAN 1.0 specification: > "A NAN Device which out-of-the-box will have a Master Preference greater > than or equal to 128 with the intention of being a NAN Master Device" > I think that this quote just defines "NAN infrastructure device". I don't see where the spec specifies that 128 should be used as a default value. Andrei > -- > Jouni MalinenPGP id EFC895FA -- 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 01/11] cfg80211: add start / stop NAN commands
>
> On Tue, Mar 29, 2016 at 12:34:59PM +0300, Emmanuel Grumbach wrote:
> > This allows user space to start/stop NAN interface.
> > A NAN interface is like P2P device in a few aspects: it doesn't have a
> > netdev associated to it.
> > Add the new interface type and prevent operations that can't be
> > executed on NAN interface like scan.
>
> What is the need for this new NAN interface from the view point of
> supporting NAN discovery?
I guess you can ask the same question for P2P device. They are both
very similar. No traffic, discovery only. So all the reasons to have a P2P
device interface apply here as well.
> Are you planning to use the same iface type
> eventually for supporting data traffic over NAN interface as well?
We don't really know yet. The plan is to have another interface type.
Probably one instance per Nan Data Path or the interface type.
>
> > + * @start_nan: Start the NAN interface.
> > + * @stop_nan: Stop the NAN interface.
>
> > 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);
>
> And similarly from 4/11:
> + int (*nan_change_conf)(struct wiphy *wiphy,
> +struct wireless_dev *wdev,
> +struct cfg80211_nan_conf *conf,
> +u32 changes);
>
> There is no matching cookie (such as transaction id) in these requests to the
> driver, are you expecting synchronous response from the driver for these
> requests? Or would some kind of event after the operation has been
> completed be possible?
Yes - these are expected to be synchronous since they don't involve network
transactions. Does your solution require this operation to be asynchronous? I
find a bit weird to make a local operation asynchronous though.
>
> --
> Jouni MalinenPGP id EFC895FA
--
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 01/11] cfg80211: add start / stop NAN commands
On Tue, Mar 29, 2016 at 12:34:59PM +0300, Emmanuel Grumbach wrote:
> This allows user space to start/stop NAN interface.
> A NAN interface is like P2P device in a few aspects: it
> doesn't have a netdev associated to it.
> Add the new interface type and prevent operations that
> can't be executed on NAN interface like scan.
What is the need for this new NAN interface from the view point of
supporting NAN discovery? Are you planning to use the same iface type
eventually for supporting data traffic over NAN interface as well?
> + * @start_nan: Start the NAN interface.
> + * @stop_nan: Stop the NAN interface.
> 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);
And similarly from 4/11:
+ int (*nan_change_conf)(struct wiphy *wiphy,
+ struct wireless_dev *wdev,
+ struct cfg80211_nan_conf *conf,
+ u32 changes);
There is no matching cookie (such as transaction id) in these requests
to the driver, are you expecting synchronous response from the driver
for these requests? Or would some kind of event after the operation has
been completed be possible?
--
Jouni MalinenPGP id EFC895FA--
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 01/11] cfg80211: add start / stop NAN commands
> On Tue, Mar 29, 2016 at 12:34:59PM +0300, Emmanuel Grumbach wrote: > > Define several attributes that may be configured by user space when > > starting NAN functionality (master preference and dual band operation) > > > diff --git a/include/uapi/linux/nl80211.h > > b/include/uapi/linux/nl80211.h @@ -826,6 +826,16 @@ > > + * @NL80211_CMD_START_NAN: Start NAN operation, identified by its > > + * %NL80211_ATTR_WDEV interface. This interface must have been > previously > > + * created with %NL80211_CMD_NEW_INTERFACE. After it has been > started, the > > + * NAN interface will create or join a cluster. This command must have a > > + * valid %NL80211_ATTR_NAN_MASTER_PREF attribute and optional > > + * %NL80211_ATTR_NAN_DUAL attributes. > > > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > > > +static int nl80211_start_nan(struct sk_buff *skb, struct genl_info > > +*info) > > > + if (!info->attrs[NL80211_ATTR_NAN_MASTER_PREF]) > > + return -EINVAL; > > Why NL80211_ATTR_NAN_MASTER_PREF is mandatory? The spec suggests > to assume master preference value of 128 if not provided. Please see quoted > text from NAN 1.0 specification: > "A NAN Device which out-of-the-box will have a Master Preference greater > than or equal to 128 with the intention of being a NAN Master Device" > Yes, but you can still have this default being defined in user space. While I agree that the kernel could accept the command even if that attribute is not present, this code, by itself, doesn't mean that it does not comply with the spec. It just defines a different partition of the roles. I read the spec again, and I fail to see how you deduce from this that 128 should be the default. Different devices will have different master preferences. Infrastructure devices will have >=128 which hints that they want to be master (consume more power) most probably because they are powered. Other devices that would prefer to avoid being master should set their out of the box master preference to <128. -- 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 01/11] cfg80211: add start / stop NAN commands
> Why NL80211_ATTR_NAN_MASTER_PREF is mandatory? The spec suggests to > assume master preference value of 128 if not provided. Please see > quoted text from NAN 1.0 specification: > "A NAN Device which out-of-the-box will have a Master Preference > greater than or equal to 128 with the intention of being a NAN Master > Device" I guess the question would be at which level of API this default would be set? I'm perfectly happy setting it at the nl80211 level, but I don't expect applications to (be able to) use the nl80211 API, and some sort of management application could just as well set the default and always provide the attribute. 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 01/11] cfg80211: add start / stop NAN commands
On Tue, Mar 29, 2016 at 12:34:59PM +0300, Emmanuel Grumbach wrote: > Define several attributes that may be configured by user space > when starting NAN functionality (master preference and dual > band operation) > diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h > @@ -826,6 +826,16 @@ > + * @NL80211_CMD_START_NAN: Start NAN operation, identified by its > + * %NL80211_ATTR_WDEV interface. This interface must have been previously > + * created with %NL80211_CMD_NEW_INTERFACE. After it has been started, the > + * NAN interface will create or join a cluster. This command must have a > + * valid %NL80211_ATTR_NAN_MASTER_PREF attribute and optional > + * %NL80211_ATTR_NAN_DUAL attributes. > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > +static int nl80211_start_nan(struct sk_buff *skb, struct genl_info *info) > + if (!info->attrs[NL80211_ATTR_NAN_MASTER_PREF]) > + return -EINVAL; Why NL80211_ATTR_NAN_MASTER_PREF is mandatory? The spec suggests to assume master preference value of 128 if not provided. Please see quoted text from NAN 1.0 specification: "A NAN Device which out-of-the-box will have a Master Preference greater than or equal to 128 with the intention of being a NAN Master Device" -- Jouni MalinenPGP id EFC895FA-- 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 01/11] cfg80211: add start / stop NAN commands
> /** > + * struct cfg80211_nan_conf - nan configuration > + * > + * This struct defines nan configuration parameters I think you should consistently capitalize "NAN" in comments. > + * @start_nan: Start the NAN interface. > + * @stop_nan: Stop the NAN interface. 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
