On Mon, 2025-06-23 at 01:24 +0300, Andrei Otcheretianski wrote:
>
> + * @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?
> + 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.
> + 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?
> + * @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.
> + * Either 44 or 149 for 5 GHz band.
We should use frequencies.
A lot of these are missing docs about their attribute type too.
> + * @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.
> + /* 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()?
> + 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
> +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?
johannes
