> -----Original Message----- > From: Arend Van Spriel [mailto:arend.vanspriel@xxxxxxxxxxxx] > Sent: Wednesday, September 21, 2016 12:40 > To: Luca Coelho <luca@xxxxxxxxx>; johannes@xxxxxxxxxxxxxxxx > Cc: linux-wireless@xxxxxxxxxxxxxxx; Beker, Ayala <ayala.beker@xxxxxxxxx>; > Otcheretianski, Andrei <andrei.otcheretianski@xxxxxxxxx>; Coelho, Luciano > <luciano.coelho@xxxxxxxxx> > Subject: Re: [PATCH v3 0/9] Add support for Neighbor Awareness > Networking > > > > On 20-9-2016 16:31, Luca Coelho wrote: > > From: Luca Coelho <luciano.coelho@xxxxxxxxx> > > > > Hi, > > > > Now v3 of the NAN patchset. Ayala has taken care of the kbuild bot > > compilation errors and of all Arend's comments, except for the one > > about adding a helper function instead checking for P2P_DEVICE and NAN > > for non-netdev interfaces. > > > > Comments are welcome, as always. > > This series exports three functions to be used by drivers: > > void cfg80211_free_nan_func(struct cfg80211_nan_func *f); void > cfg80211_nan_match(struct wireless_dev *wdev, > struct cfg80211_nan_match_params *match, gfp_t > gfp); void cfg80211_nan_func_terminated(struct wireless_dev *wdev, > u8 inst_id, > enum nl80211_nan_func_term_reason > reason, > u64 cookie, gfp_t gfp); > > Some more descriptive text about the use of these functions would be > appropriate I think. For example looking at the patch series it seems the > responsibility to call cfg80211_free_nan_func() is in the driver although > allocation is done by cfg80211. Regarding the cfg80211_free_nan_func() it is documented in patch "[PATCH v3 3/9] cfg80211: add add_nan_func / del_nan_func" Here: * @add_nan_func: Add a NAN function. Returns negative value on failure. * On success @nan_func ownership is transferred to the driver and * it may access it outside of the scope of this function. The driver * should free the @nan_func when no longer needed by calling * cfg80211_free_nan_func(). * On success the driver should assign an instance_id in the * provided @nan_func. All the others are pretty much straight forward. Match and termination events are defined in NAN spec - so it should be easy to know when to call these functions. > Actually, I do not see mac80211 calling it so are > we leaking there? I would expect it being called in > .del_nan_func() callback and/or .stop_nan(). It is. Look at "[PATCH v3 8/9] mac80211: Implement add_nan_func and rm_nan_func". It is called in stop_nan() and nan_func_terminated() callback. The nan function can't be freed directly in del_nan_func() - otherwise it would be racy with match/termination notifications. > Rather than exporting > cfg80211_free_nan_func() I would prefer calling > cfg80211_nan_func_terminated() and have cfg80211 take care of freeing > nan_func resources. As I already answered, there are some situations when the wdev isn't available and mac80211 can't call cfg80211_nan_func_terminated() - for example nan interface is being stopped (at least it's true for mac80211). Also I think that the driver should be able to free the nan function without being forced to send notification to the user space and the other way around. > > Regards, > Arend > > > Cheers, > > Luca. > > > > > > Ayala Beker (9): > > cfg80211: add start / stop NAN commands > > mac80211: add boilerplate code for start / stop NAN > > cfg80211: add add_nan_func / del_nan_func > > cfg80211: allow the user space to change current NAN configuration > > cfg80211: provide a function to report a match for NAN > > cfg80211: Provide an API to report NAN function termination > > mac80211: implement nan_change_conf > > mac80211: Implement add_nan_func and rm_nan_func > > mac80211: Add API to report NAN function match > > > > include/net/cfg80211.h | 184 ++++++++++++- > > include/net/mac80211.h | 65 +++++ > > include/uapi/linux/nl80211.h | 253 +++++++++++++++++ > > net/mac80211/cfg.c | 208 ++++++++++++++ > > net/mac80211/chan.c | 6 + > > net/mac80211/driver-ops.h | 80 ++++++ > > net/mac80211/ieee80211_i.h | 17 ++ > > net/mac80211/iface.c | 28 +- > > net/mac80211/main.c | 8 + > > net/mac80211/offchannel.c | 4 +- > > net/mac80211/rx.c | 3 + > > net/mac80211/trace.h | 133 +++++++++ > > net/mac80211/util.c | 50 +++- > > net/wireless/chan.c | 2 + > > net/wireless/core.c | 35 +++ > > net/wireless/core.h | 3 + > > net/wireless/mlme.c | 1 + > > net/wireless/nl80211.c | 642 > ++++++++++++++++++++++++++++++++++++++++++- > > net/wireless/rdev-ops.h | 58 ++++ > > net/wireless/trace.h | 90 ++++++ > > net/wireless/util.c | 28 +- > > 21 files changed, 1888 insertions(+), 10 deletions(-) > >
