> > + * @rssi_close: RSSI close threshold used for NAN master selection. If not
> > + * specified (set to 0), default device value is used. The value should
> > + * be greater than -60 dBm (unsigned).
> > + * @rssi_middle: RSSI middle threshold used for NAN master selection. If
> not
> > + * specified (set to 0), default device value is used. The value should be
> > + * greater than -75 dBm and less than rssi_close.
>
> I think these are not described well, and I also don't understand why we
> shouldn't use negative values in the APIs? I know the iwlwifi firmware doesn't
> like to do it, but that's not a good reason not to do it in other places?
Sure. Will make it signed.
>
> > + struct cfg80211_nan_band_config low_band_cfg;
> > + struct cfg80211_nan_band_config high_band_cfg;
> > + bool enable_hb_scan;
>
> When we have "nan_supported_bands", it seems to me these should really be
> by arbitrary band, and bitmap of bands to enable scan on, or something like
> that ... also this really applies to the nl80211 API.
NAN synchronization is only defined for 2.4 and 5 GHZ bands.
Now, "bands" (supported bands) field existed before, and I didn't want to change it.
Having an array of band_configs imho looks overkill.
Regarding enable_hb_scan (if this comment refers to this field),
it can't be for "arbitrary" band, as disabling scanning on 2.4GHz is not allowed.
>
> > + u8 *extra_nan_attrs;
> > + u8 *vendor_elems;
>
> const u8 *, presumably
>
> > + size_t vendor_elems_len;
>
> Also not sure I see a need for size_t here, it's certainly going to be limited to a
> netlink attribute (u16?) anyway?
Will change to u16
>
> > + * @NL80211_NAN_BAND_CONF_CHAN: Discovery channel.
>
> > Ignored on 2.4GHz band.
>
> Shouldn't be ignored. Either require a correct value, or reject the presence of
> the attribute.
Ok.
>
> > + * Either 44 or 149 for 5 GHz band.
>
> We should use frequencies.
Ok. I'll change to freq.
>
> A lot of these are missing docs about their attribute type too.
>
Sure. Fixed
> > + * @NL80211_NAN_BAND_CONF_RSSI_CLOSE: RSSI close for NAN cluster
> state changes.
> > + * This is unsigned 8-bit value in dBm (absolute value).
>
> Nah, see above.
Will make it signed
>
> > + /* Check if the channel is allowed */
> > + if (!cfg80211_reg_can_beacon(wiphy, &def, NL80211_IFTYPE_NAN))
> > + return false;
> > +
> > + return true;
>
> return cfg80211_reg_can_beacon()?
>
Of course :)
> > + if (!conf->low_band_cfg.chan) {
> > + /* If no 2GHz channel is specified, use the default */
> > + conf->low_band_cfg.chan =
> > + ieee80211_get_channel(wiphy, 2437);
> > + if (!conf->low_band_cfg.chan ||
> > + !nl80211_valid_nan_freq(wiphy, 2437))
> > + return -EINVAL;
>
> code style
>
Fixed
> > +static int nl80211_start_nan(struct sk_buff *skb, struct genl_info
> > +*info) {
> > + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> > + struct wireless_dev *wdev = info->user_ptr[1];
> > + struct cfg80211_nan_conf conf = {};
> > + int err;
> > + u32 changed = 0;
>
> what's that 'changed' even doing?
Nothing here. It is needed only in change_config(), but I still need to pass a valid pointer.
I'll make it support NULL..
>
> johannes
