On 20-9-2016 13:45, Beker, Ayala wrote:
> -----Original Message-----
> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com] 
> Sent: Sunday, September 18, 2016 22:01
> 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 2/9] mac80211: add boilerplate code for start / stop 
> NAN
> 
> On 18-9-2016 9:59, Otcheretianski, Andrei wrote:
>>> -----Original Message-----
>>> From: Arend Van Spriel [mailto:arend.vanspr...@broadcom.com]
>>> Sent: Friday, September 16, 2016 14:09
>>> 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 2/9] mac80211: add boilerplate code for start 
>>> / stop NAN
>>>
>>> On 16-9-2016 10:33, Luca Coelho wrote:
>>>> From: Ayala Beker <ayala.be...@intel.com>
>>>>
>>>> This code doesn't do much besides allowing to start and stop the vif.
>>>>
>>>> Signed-off-by: Andrei Otcheretianski 
>>>> <andrei.otcheretian...@intel.com>
>>>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumb...@intel.com>
>>>> Signed-off-by: Ayala Beker <ayala.be...@intel.com>
>>>> Signed-off-by: Luca Coelho <luciano.coe...@intel.com>
>>>> ---
>>>>  include/net/mac80211.h    |  9 +++++++++
>>>>  net/mac80211/cfg.c        | 36 ++++++++++++++++++++++++++++++++++
>>>>  net/mac80211/chan.c       |  3 +++
>>>>  net/mac80211/driver-ops.h | 29 ++++++++++++++++++++++++++-
>>>>  net/mac80211/iface.c      |  8 ++++++--
>>>>  net/mac80211/main.c       |  5 +++++
>>>>  net/mac80211/offchannel.c |  3 ++-
>>>>  net/mac80211/trace.h      | 50
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  net/mac80211/util.c       |  3 ++-
>>>>  9 files changed, 141 insertions(+), 5 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h 
>>>> index fe35a1c..67b42c8 100644
>>>> --- a/net/mac80211/driver-ops.h
>>>> +++ b/net/mac80211/driver-ops.h
>>>> @@ -163,7 +163,8 @@ static inline void drv_bss_info_changed(struct 
>>>> ieee80211_local *local,
>>>>
>>>>    if (WARN_ON_ONCE(sdata->vif.type ==
>>> NL80211_IFTYPE_P2P_DEVICE ||
>>>>                     (sdata->vif.type == NL80211_IFTYPE_MONITOR &&
>>>> -                    !sdata->vif.mu_mimo_owner)))
>>>> +                    !sdata->vif.mu_mimo_owner) ||
>>>> +                   sdata->vif.type == NL80211_IFTYPE_NAN))
>>>
>>> Might be more clear to move this up right after P2P_DEVICE check.
>>
>> Why? It's a completely separate new condition - so it goes to the end.
> 
>> I would say readability. Both P2P_DEVICE and NAN checks are single 
>> comparisons as opposed to the MONITOR check.
> 
>>>
>>>>            return;
>>>>
>>>>    if (!check_sdata_in_driver(sdata)) @@ -1165,4 +1166,30 @@ static 
>>>> inline void drv_wake_tx_queue(struct
>>> ieee80211_local *local,
>>>>    local->ops->wake_tx_queue(&local->hw, &txq->txq);  }
>>>>
>>>> +static inline int drv_start_nan(struct ieee80211_local *local,
>>>> +                          struct ieee80211_sub_if_data *sdata,
>>>> +                          struct cfg80211_nan_conf *conf) {
>>>> +  int ret;
>>>> +
>>>> +  might_sleep();
>>>> +  check_sdata_in_driver(sdata);
>>>> +
>>>> +  trace_drv_start_nan(local, sdata, conf);
>>>> +  ret = local->ops->start_nan(&local->hw, &sdata->vif, conf);
>>>> +  trace_drv_return_int(local, ret);
>>>> +  return ret;
>>>> +}
>>>> +
>>>> +static inline void drv_stop_nan(struct ieee80211_local *local,
>>>> +                          struct ieee80211_sub_if_data *sdata) {
>>>> +  might_sleep();
>>>> +  check_sdata_in_driver(sdata);
>>>> +
>>>> +  trace_drv_stop_nan(local, sdata);
>>>> +  local->ops->stop_nan(&local->hw, &sdata->vif);
>>>> +  trace_drv_return_void(local);
>>>> +}
>>>> +
>>>>  #endif /* __MAC80211_DRIVER_OPS */
>>>> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index 
>>>> e694ca2..507f46a 100644
>>>> --- a/net/mac80211/iface.c
>>>> +++ b/net/mac80211/iface.c
>>>> @@ -327,6 +327,9 @@ static int ieee80211_check_queues(struct
>>> ieee80211_sub_if_data *sdata,
>>>>    int n_queues = sdata->local->hw.queues;
>>>>    int i;
>>>>
>>>> +  if (iftype == NL80211_IFTYPE_NAN)
>>>> +          return 0;
>>>> +
>>>>    if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
>>>>            for (i = 0; i < IEEE80211_NUM_ACS; i++) {
>>>>                    if (WARN_ON_ONCE(sdata->vif.hw_queue[i] == @@
>>> -647,7 +650,8 @@ int
>>>> ieee80211_do_open(struct wireless_dev *wdev, bool coming_up)
>>>>                    local->fif_probe_req++;
>>>>            }
>>>>
>>>> -          if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE)
>>>> +          if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE &&
>>>> +              sdata->vif.type != NL80211_IFTYPE_NAN)
>>>
>>> similar check keeps reoccuring in various places so maybe we can 
>>> create a helper function for it.
>>
>> Right, but not sure that it deserves a function.
> 
>> If similar new iftypes are anticipated it would make sense as it would mean 
>> adding it in one place.
> 
> I don't think there is something in common to those interface types that can 
> fit such a function semantically, so I decided not to change it.
> Do you have any idea?

What is common is that these are both non-netdev interface types.

>> Regards,
>> Arend
> 
> ---------------------------------------------------------------------
> A member of the Intel Corporation group of companies
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Maybe better to avoid such disclaimers when you are posting on community
mailing lists.

Regards,
Arend

Reply via email to