RE: [PATCH v2 1/9] cfg80211: add start / stop NAN commands

2016-09-19 Thread Otcheretianski, Andrei 
> -Original Message-
> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: Sunday, September 18, 2016 21:54
> To: Otcheretianski, Andrei <andrei.otcheretian...@intel.com>; Luca Coelho
> <l...@coelho.fi>; johan...@sipsolutions.net
> Cc: linux-wireless@vger.kernel.org; Beker, Ayala <ayala.be...@intel.com>;
> Grumbach, Emmanuel <emmanuel.grumb...@intel.com>; Coelho, Luciano
> <luciano.coe...@intel.com>
> Subject: Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands
> 
> On 18-9-2016 9:44, Otcheretianski, Andrei wrote:
> >> -Original Message-
> >> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com]
> >> Sent: Friday, September 16, 2016 13:59
> >> To: Luca Coelho <l...@coelho.fi>; johan...@sipsolutions.net
> >> Cc: linux-wireless@vger.kernel.org; Beker, Ayala
> >> <ayala.be...@intel.com>; Otcheretianski, Andrei
> >> <andrei.otcheretian...@intel.com>; Grumbach, Emmanuel
> >> <emmanuel.grumb...@intel.com>; Coelho, Luciano
> >> <luciano.coe...@intel.com>
> >> Subject: Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands
> >>
> >> On 16-9-2016 10:33, Luca Coelho wrote:
> >>> From: Ayala Beker <ayala.be...@intel.com>
> >>>
> >>> 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.
> >>>
> >>> Define several attributes that may be configured by user space when
> >>> starting NAN functionality (master preference and dual band
> >>> operation)
> >>>
> >>> Signed-off-by: Andrei Otcheretianski
> >>> <andrei.otcheretian...@intel.com>
> >>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumb...@intel.com>
> >>> Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> >>> ---
> >>>  include/net/cfg80211.h   | 21 +-
> >>>  include/uapi/linux/nl80211.h | 52 +
> >>>  net/mac80211/cfg.c   |  2 +
> >>>  net/mac80211/chan.c  |  3 ++
> >>>  net/mac80211/iface.c |  4 ++
> >>>  net/mac80211/offchannel.c|  1 +
> >>>  net/mac80211/rx.c|  3 ++
> >>>  net/mac80211/util.c  |  1 +
> >>>  net/wireless/chan.c  |  2 +
> >>>  net/wireless/core.c  | 34 
> >>>  net/wireless/core.h  |  3 ++
> >>>  net/wireless/mlme.c  |  1 +
> >>>  net/wireless/nl80211.c   | 93
> >> ++--
> >>>  net/wireless/rdev-ops.h  | 20 ++
> >>>  net/wireless/trace.h | 27 +
> >>>  net/wireless/util.c  |  6 ++-
> >>>  16 files changed, 267 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index
> >>> d5e7f69..ca64d69 100644
> >>> --- a/include/net/cfg80211.h
> >>> +++ b/include/net/cfg80211.h
> >>> @@ -2293,6 +2293,19 @@ struct cfg80211_qos_map {  };
> >>
> >> [...]
> >>
> >>> +/**
> >>> + * enum nl80211_nan_dual_band_conf - NAN dual band configuration
> >>> + *
> >>> + * Defines the NAN dual band mode of operation
> >>
> >> Does it make sense to have such a notion of bands in use. And what
> >> does 5.2GHz mean. Is this a subband within 5G channels? Probably I
> >> should read the NAN spec to understand what is meant here. I would
> >> consider 5.2G as lower 5G, ie. discovery on channel 44 but not sure if that
> is meant here.
> >>
> >
> > The NAN spec defines single and dual band modes of operation. The
> channels are fixed for each band.
> > On 2.4Ghz is channel 6 and 5GHz is either 44 or 149. Regarding 5.2 - it's 
> > just a
> typo. It should be 5G - no deeper meaning.
> 
> The thing is that it seems likely other bands will be added so that would 
> kinda
> obsolete this whole enum. So I would propose to have another way to
> configure the bands to use. This enum is not really extensible though it may
> reflect the current state of the spec, which is still in draft if I am not 
> mistaken.
> 

I guess you are talking about additional bands that are mentioned in NAN2 spec 
(like sub-1Ghz etc..).
I'm not sure that these bands will be used for sync or NAN2 specific operations 
only (like data path or ranging).
Nevertheless, you're right, I guess it doesn't harm to make it a bitmask of 
supported bands.

> Regards,
> Arend

Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands

2016-09-18 Thread Arend Van Spriel 
On 18-9-2016 9:44, Otcheretianski, Andrei wrote:
>> -Original Message-
>> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com]
>> Sent: Friday, September 16, 2016 13:59
>> To: Luca Coelho <l...@coelho.fi>; johan...@sipsolutions.net
>> Cc: linux-wireless@vger.kernel.org; Beker, Ayala <ayala.be...@intel.com>;
>> Otcheretianski, Andrei <andrei.otcheretian...@intel.com>; Grumbach,
>> Emmanuel <emmanuel.grumb...@intel.com>; Coelho, Luciano
>> <luciano.coe...@intel.com>
>> Subject: Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands
>>
>> On 16-9-2016 10:33, Luca Coelho wrote:
>>> From: Ayala Beker <ayala.be...@intel.com>
>>>
>>> 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.
>>>
>>> Define several attributes that may be configured by user space when
>>> starting NAN functionality (master preference and dual band operation)
>>>
>>> Signed-off-by: Andrei Otcheretianski <andrei.otcheretian...@intel.com>
>>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumb...@intel.com>
>>> Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
>>> ---
>>>  include/net/cfg80211.h   | 21 +-
>>>  include/uapi/linux/nl80211.h | 52 +
>>>  net/mac80211/cfg.c   |  2 +
>>>  net/mac80211/chan.c  |  3 ++
>>>  net/mac80211/iface.c |  4 ++
>>>  net/mac80211/offchannel.c|  1 +
>>>  net/mac80211/rx.c|  3 ++
>>>  net/mac80211/util.c  |  1 +
>>>  net/wireless/chan.c  |  2 +
>>>  net/wireless/core.c  | 34 
>>>  net/wireless/core.h  |  3 ++
>>>  net/wireless/mlme.c  |  1 +
>>>  net/wireless/nl80211.c   | 93
>> ++--
>>>  net/wireless/rdev-ops.h  | 20 ++
>>>  net/wireless/trace.h | 27 +
>>>  net/wireless/util.c  |  6 ++-
>>>  16 files changed, 267 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index
>>> d5e7f69..ca64d69 100644
>>> --- a/include/net/cfg80211.h
>>> +++ b/include/net/cfg80211.h
>>> @@ -2293,6 +2293,19 @@ struct cfg80211_qos_map {  };
>>
>> [...]
>>
>>> +/**
>>> + * enum nl80211_nan_dual_band_conf - NAN dual band configuration
>>> + *
>>> + * Defines the NAN dual band mode of operation
>>
>> Does it make sense to have such a notion of bands in use. And what does
>> 5.2GHz mean. Is this a subband within 5G channels? Probably I should read
>> the NAN spec to understand what is meant here. I would consider 5.2G as
>> lower 5G, ie. discovery on channel 44 but not sure if that is meant here.
>>
> 
> The NAN spec defines single and dual band modes of operation. The channels 
> are fixed for each band.
> On 2.4Ghz is channel 6 and 5GHz is either 44 or 149. Regarding 5.2 - it's 
> just a typo. It should be 5G - no deeper meaning.

The thing is that it seems likely other bands will be added so that
would kinda obsolete this whole enum. So I would propose to have another
way to configure the bands to use. This enum is not really extensible
though it may reflect the current state of the spec, which is still in
draft if I am not mistaken.

Regards,
Arend

RE: [PATCH v2 1/9] cfg80211: add start / stop NAN commands

2016-09-18 Thread Otcheretianski, Andrei 
> -Original Message-
> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: Friday, September 16, 2016 13:59
> To: Luca Coelho <l...@coelho.fi>; johan...@sipsolutions.net
> Cc: linux-wireless@vger.kernel.org; Beker, Ayala <ayala.be...@intel.com>;
> Otcheretianski, Andrei <andrei.otcheretian...@intel.com>; Grumbach,
> Emmanuel <emmanuel.grumb...@intel.com>; Coelho, Luciano
> <luciano.coe...@intel.com>
> Subject: Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands
> 
> On 16-9-2016 10:33, Luca Coelho wrote:
> > From: Ayala Beker <ayala.be...@intel.com>
> >
> > 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.
> >
> > Define several attributes that may be configured by user space when
> > starting NAN functionality (master preference and dual band operation)
> >
> > Signed-off-by: Andrei Otcheretianski <andrei.otcheretian...@intel.com>
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumb...@intel.com>
> > Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
> > ---
> >  include/net/cfg80211.h   | 21 +-
> >  include/uapi/linux/nl80211.h | 52 +
> >  net/mac80211/cfg.c   |  2 +
> >  net/mac80211/chan.c  |  3 ++
> >  net/mac80211/iface.c |  4 ++
> >  net/mac80211/offchannel.c|  1 +
> >  net/mac80211/rx.c|  3 ++
> >  net/mac80211/util.c  |  1 +
> >  net/wireless/chan.c  |  2 +
> >  net/wireless/core.c  | 34 
> >  net/wireless/core.h  |  3 ++
> >  net/wireless/mlme.c  |  1 +
> >  net/wireless/nl80211.c   | 93
> ++--
> >  net/wireless/rdev-ops.h  | 20 ++
> >  net/wireless/trace.h | 27 +
> >  net/wireless/util.c  |  6 ++-
> >  16 files changed, 267 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index
> > d5e7f69..ca64d69 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2293,6 +2293,19 @@ struct cfg80211_qos_map {  };
> 
> [...]
> 
> > +/**
> > + * enum nl80211_nan_dual_band_conf - NAN dual band configuration
> > + *
> > + * Defines the NAN dual band mode of operation
> 
> Does it make sense to have such a notion of bands in use. And what does
> 5.2GHz mean. Is this a subband within 5G channels? Probably I should read
> the NAN spec to understand what is meant here. I would consider 5.2G as
> lower 5G, ie. discovery on channel 44 but not sure if that is meant here.
>

The NAN spec defines single and dual band modes of operation. The channels are 
fixed for each band.
On 2.4Ghz is channel 6 and 5GHz is either 44 or 149. Regarding 5.2 - it's just 
a typo. It should be 5G - no deeper meaning.
 
> > + * @NL80211_NAN_BAND_DEFAULT: device default mode
> > + * @NL80211_NAN_BAND_SINGLE: 2.4GHz only mode
> > + * @NL80211_NAN_BAND_DUAL: 2.4GHz and 5.2GHz mode
> > +  */
> > +enum nl80211_nan_dual_band_conf {
> > +   NL80211_NAN_BAND_DEFAULT,
> > +   NL80211_NAN_BAND_SINGLE,
> > +   NL80211_NAN_BAND_DUAL,
> > +
> > +   /* keep last */
> > +   __NL80211_NAN_BAND_AFTER_LAST,
> > +   NL80211_NAN_BAND_MAX =
> > +   __NL80211_NAN_BAND_AFTER_LAST - 1,
> > +};
> > +
> >  #endif /* __LINUX_NL80211_H */
> > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index
> > e29ff57..a74027f 100644
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -257,6 +257,7 @@ static int ieee80211_add_key(struct wiphy *wiphy,
> struct net_device *dev,
> > case NL80211_IFTYPE_WDS:
> > case NL80211_IFTYPE_MONITOR:
> > case NL80211_IFTYPE_P2P_DEVICE:
> > +   case NL80211_IFTYPE_NAN:
> > case NL80211_IFTYPE_UNSPECIFIED:
> > case NUM_NL80211_IFTYPES:
> 
> Huh? What is this doing here? Not yours but weird still.
> 
> > case NL80211_IFTYPE_P2P_CLIENT:
> > @@ -2036,6 +2037,7 @@ static int ieee80211_scan(struct wiphy *wiphy,
> >  !(req->flags & NL80211_SCAN_FLAG_AP)))
> > return -EOPNOTSUPP;
> > break;
> > +   case NL80211_IFTYPE_NAN:
> > default:
> > return -EOPNOTSUPP;
> > }
> > diff --git a/net/mac80211/chan.c b/net/

Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands

2016-09-16 Thread Johannes Berg 

> I think NUM_NL80211_IFTYPES should not be in the switch. If it must I
> would leave it as last one here.
> 

It just suppresses compiler warnings :)

johannes

Re: [PATCH v2 1/9] cfg80211: add start / stop NAN commands

2016-09-16 Thread Arend Van Spriel 
On 16-9-2016 10:33, Luca Coelho wrote:
> From: Ayala Beker 
> 
> 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.
> 
> Define several attributes that may be configured by user space
> when starting NAN functionality (master preference and dual
> band operation)
> 
> Signed-off-by: Andrei Otcheretianski 
> Signed-off-by: Emmanuel Grumbach 
> Signed-off-by: Luca Coelho 
> ---
>  include/net/cfg80211.h   | 21 +-
>  include/uapi/linux/nl80211.h | 52 +
>  net/mac80211/cfg.c   |  2 +
>  net/mac80211/chan.c  |  3 ++
>  net/mac80211/iface.c |  4 ++
>  net/mac80211/offchannel.c|  1 +
>  net/mac80211/rx.c|  3 ++
>  net/mac80211/util.c  |  1 +
>  net/wireless/chan.c  |  2 +
>  net/wireless/core.c  | 34 
>  net/wireless/core.h  |  3 ++
>  net/wireless/mlme.c  |  1 +
>  net/wireless/nl80211.c   | 93 
> ++--
>  net/wireless/rdev-ops.h  | 20 ++
>  net/wireless/trace.h | 27 +
>  net/wireless/util.c  |  6 ++-
>  16 files changed, 267 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index d5e7f69..ca64d69 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -2293,6 +2293,19 @@ struct cfg80211_qos_map {
>  };

[...]

> +/**
> + * enum nl80211_nan_dual_band_conf - NAN dual band configuration
> + *
> + * Defines the NAN dual band mode of operation

Does it make sense to have such a notion of bands in use. And what does
5.2GHz mean. Is this a subband within 5G channels? Probably I should
read the NAN spec to understand what is meant here. I would consider
5.2G as lower 5G, ie. discovery on channel 44 but not sure if that is
meant here.

> + * @NL80211_NAN_BAND_DEFAULT: device default mode
> + * @NL80211_NAN_BAND_SINGLE: 2.4GHz only mode
> + * @NL80211_NAN_BAND_DUAL: 2.4GHz and 5.2GHz mode
> +  */
> +enum nl80211_nan_dual_band_conf {
> + NL80211_NAN_BAND_DEFAULT,
> + NL80211_NAN_BAND_SINGLE,
> + NL80211_NAN_BAND_DUAL,
> +
> + /* keep last */
> + __NL80211_NAN_BAND_AFTER_LAST,
> + NL80211_NAN_BAND_MAX =
> + __NL80211_NAN_BAND_AFTER_LAST - 1,
> +};
> +
>  #endif /* __LINUX_NL80211_H */
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index e29ff57..a74027f 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -257,6 +257,7 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct 
> net_device *dev,
>   case NL80211_IFTYPE_WDS:
>   case NL80211_IFTYPE_MONITOR:
>   case NL80211_IFTYPE_P2P_DEVICE:
> + case NL80211_IFTYPE_NAN:
>   case NL80211_IFTYPE_UNSPECIFIED:
>   case NUM_NL80211_IFTYPES:

Huh? What is this doing here? Not yours but weird still.

>   case NL80211_IFTYPE_P2P_CLIENT:
> @@ -2036,6 +2037,7 @@ static int ieee80211_scan(struct wiphy *wiphy,
>!(req->flags & NL80211_SCAN_FLAG_AP)))
>   return -EOPNOTSUPP;
>   break;
> + case NL80211_IFTYPE_NAN:
>   default:
>   return -EOPNOTSUPP;
>   }
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index 74142d0..acb50f8 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -274,6 +274,7 @@ ieee80211_get_chanctx_max_required_bw(struct 
> ieee80211_local *local,
>   ieee80211_get_max_required_bw(sdata));
>   break;
>   case NL80211_IFTYPE_P2P_DEVICE:
> + case NL80211_IFTYPE_NAN:
>   continue;
>   case NL80211_IFTYPE_ADHOC:
>   case NL80211_IFTYPE_WDS:
> @@ -718,6 +719,7 @@ void ieee80211_recalc_smps_chanctx(struct ieee80211_local 
> *local,
>  
>   switch (sdata->vif.type) {
>   case NL80211_IFTYPE_P2P_DEVICE:
> + case NL80211_IFTYPE_NAN:
>   continue;
>   case NL80211_IFTYPE_STATION:
>   if (!sdata->u.mgd.associated)
> @@ -981,6 +983,7 @@ ieee80211_vif_chanctx_reservation_complete(struct 
> ieee80211_sub_if_data *sdata)
>   case NL80211_IFTYPE_P2P_GO:
>   case NL80211_IFTYPE_P2P_DEVICE:
>   case NUM_NL80211_IFTYPES:

I think NUM_NL80211_IFTYPES should not be in the switch. If it must I
would leave it as last one here.

> + case NL80211_IFTYPE_NAN:
>   WARN_ON(1);
>   break;
>   }

Regards,
Arend