Re: [PATCHv3 RESEND 03/11] cfg80211: add add_nan_func / rm_nan_func

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2016-03-29 at 12:35 +0300, Emmanuel Grumbach wrote:

> + * @cookie: user defined cookie (will be returned with
> notifications)

Didn't we change it to not be user defined?

> + * @NL80211_NAN_FUNC_TTL: number of DWs this function should stay
> active. 0 is
> + *	equivalent to no TTL at all. This is a u32.

I think it should rather be "if the attribute is not specified, no TTL
is used", with 0 being an invalid value.

> + * @NL80211_NAN_FUNC_RX_MATCH_FILTER: Receive Matching filter. This
> is a nested
> + *	attribute. It is a list of binary values.

That probably needs to say what kind if "binary values"?

> + * @NL80211_NAN_FUNC_TX_MATCH_FILTER: Transmit Matching filter. This
> is a
> + *	nested attribute. It is a list of binary values.

Ditto.

> + * @NL80211_NAN_SRF_INCLUDE: true if the include bit of the SRF set.
> + *	This is a flag.
> + * @NL80211_NAN_SRF_TYPE_BF: true if the SRF is a Bloom Filter SRF.
> If false
> + *	the SRF will be &NL80211_ATTR_MAC_ADDRS. This is a flag.

"true" and "false" aren't really states for a flag, it can be
"specified" or "not specified", or maybe "present" and "not present".

> + * @NL80211_NAN_SRF_BF: Bloom Filter. Relevant and mandatory if
> + *	&NL80211_NAN_SRF_TYPE_BF is true. This attribute is
> binary.

Likewise.
However, why do you even need two attributes? It doesn't seem relevant
to specify the NAN_SRF_BF attribute when NAN_SRF_TYPE_BF isn't set?

> + * @NL80211_NAN_SRF_BF_IDX: index of the Bloom Filter. Relevant and
> + *	mandatory if &NL80211_NAN_SRF_TYPE_BF is true. This is a
> u8.

Likewise - what do you need the SRF_TYPE_BF flag for at all?

> + * @NL80211_NAN_SRF_MAC_ADDRS: list of MAC addresses for the SRF.
> Relevant and
> + *	mandatory if &NL80211_NAN_SRF_TYPE_BF is false. This is a
> nested
> + *	attribute. Each nested attribute is a MAC address.

And this is obviously the opposite - so both SRF_MAC_ADDRS and
SRF_BR/BF_IDX can't be specified together. No need for the flag?


> +	if (tx) {
> +		func->num_tx_filters = (u8)n_entries;
> +		func->tx_filters = filter;
> +	} else {
> +		func->num_rx_filters = (u8)n_entries;
> +		func->rx_filters = filter;

No need for those casts.

johannes