Hi Vinay,
Thanks for reviewing, just a general note, can you make sure you reply inline in plaintext, so it makes it easier to reply 😊
> > + * @low_band_cfg: configuration for the low band.
> > + * @high_band_cfg: configuration for the high band.
> > + * @enable_scan: Flag to enable or disable scanning on 5 GHz band.
> s/enable_scan/enable_hb_scan
Will fix. Thanks.
> > + * @extra_nan_attrs: pointer to additional NAN attributes. >
> > + * @extra_nan_attrs_len: length of the additional NAN attributes.
> > + * @vendor_elems: pointer to vendor-specific elements.
> > + * @vendor_elems_len: length of the vendor-specific elements.
> Can you please review if the following additional parameters would be required for the drivers to start nan or change the config.
> * @max_publish_sids_in_beacon: This indicates maximum number of service IDs that can be accommodated in the Service ID List attribute in a NAN beacon.
> * @max_subscribe_sids_in_beacon: This indicates maximum number of service IDs that can be accommodated in the Subscribe Service ID List attribute in a NAN beacon.
These look like capabilities of the device and not as configuration no?
I think it makes more sense that the device will report it. (See my next capability patch).
However, specifically for these parameters, as I see it, the user space will configure service ids list using extra_nan_attrs.
So if needed, I would instead report more generic max_extran_nan_attrs_size capability. (Same for vendor elements).
> * @hop_limit: Maximum number of hops that is allowed to join a cluster. Value 0 indicates no such restriction.
Actually we considered it and even had some long discussions with Mahesh (cc'ed) why is this needed.
In my understanding limiting hop may result in cluster synchronization being broken, or some undefined behavior.
Can you explain what is the scenario that you are trying to solve by limiting the hop count?
> * @rssi_window_size: Moving average of RSSI to decide NAN role switch.
This should be implementation specific. Implementing moving average for all the beacons around is not trivial.
The device may also implement instead some other algorithms for averaging like (EWA) etc..
Do you have any specific scenario where user space needs to configure it?
> * @nss: No of TX and RX chains to be used. This can be reconfigured using change configs for any change in power setting.
Not sure what it means. Is it for ranging only maybe?
I didn't cover any ranging related parameters in this patch series. If needed, maybe it should be added separately.
If it's for general NAN synchronization, I'm not sure what it means as NAN device uses only 6mbit rate - I think it will always use a single antenna.
> * @enable_dw_early_termination: Flag to enable/disable early DW termination. The device can decide to move out of the DW before the dwell time if this is enabled.
This one makes sense, but we will also need a corresponding capability since it's an optional feature.
> /**
> @@ -3779,10 +3828,32 @@ struct cfg80211_nan_conf {
> *
> * @CFG80211_NAN_CONF_CHANGED_PREF: master preference
> * @CFG80211_NAN_CONF_CHANGED_BANDS: operating bands
> + * @CFG80211_NAN_CONF_CHANGED_CLUSTER_ID: cluster ID
> + * @CFG80211_NAN_CONF_CHANGED_SCAN_PERIOD: scan period
> + * @CFG80211_NAN_CONF_CHANGED_SCAN_DWELL_TIME: scan dwell time
>
>
Andrei
