On 26-1-2016 14:56, Johannes Berg wrote:
> On Tue, 2016-01-26 at 14:06 +0100, Arend van Spriel wrote:
>>
>> + * @behaviour: requested BSS selection behaviour.
>> + * @param: parameters for requestion behaviour.
>> + * @band_pref: preferred band for
>> %NL80211_BSS_SELECT_ATTR_BAND_PREF.
>> + * @adjust: parameters for %NL80211_BSS_SELECT_ATTR_RSSI_ADJUST.
>
> Sadly, I don't think this works with kernel-doc. You'd have to split it
> out into a named union to get this working properly.
Yeah. I did not run kernel-doc. Will look into it.
>> +/**
>> + * enum nl80211_bss_select_attr - attributes for bss selection.
>> + *
>> + * @__NL80211_BSS_SELECT_ATTR_INVALID: reserved.
>> + * @NL80211_BSS_SELECT_ATTR_RSSI: Flag indicating only RSSI-based BSS selection
>> + * is requested.
>> + * @NL80211_BSS_SELECT_ATTR_BAND_PREF: attribute indicating BSS
>> + *> > selection should be done such that the specified band is preferred.
>> + *> > When there are multiple BSS-es in the preferred band, the driver
>> + *> > shall use RSSI-based BSS selection as a second step. The value of
>> + *> > this attribute is according to &enum nl80211_band (u32).
>> + * @NL80211_BSS_SELECT_ATTR_RSSI_ADJUST: When present the RSSI level for
>> + *> > BSS-es in the specified band is to be adjusted before doing
>> + *> > RSSI-based BSS selection. The attribute value is a packed two-byte
>> + *> > value. The lower byte contains the adjustment value (s8) and the
>> + * high byte contains the band according &enum nl80211_band.
>
> I think it might be nicer to define an explicit struct for this, then
> you don't have to use u8 for the band in one attribute and u32 for the
> band in the other attribute either.
>
> As long as there's no u64 in the struct that's pretty much safe - if
> u64 is needed use compat_u64 :)
So you mean mapping the explicit structure over the nla_data()?
>> + * @NL80211_BSS_SELECT_ATTR_MAX: highest bss select attribute number.
>> + *@__NL80211_BSS_SELECT_ATTR_AFTER_LAST: internal use.
>> + *
>> + * These attributes are found within %NL80211_ATTR_BSS_SELECT and
>> + * indicate the required BSS selection behaviour which the driver
>> + * should use.
>
> You should probably indicate that only a single one can ever be
> specified?
Realized that was missing indeed. Will add it.
>> +static const struct nla_policy
>> +nl80211_bss_select_policy[NL80211_BSS_SELECT_ATTR_MAX + 1] = {
>> + [NL80211_BSS_SELECT_ATTR_RSSI] = { .type = NLA_FLAG },
>> + [NL80211_BSS_SELECT_ATTR_BAND_PREF] = { .type = NLA_U32 },
>> + [NL80211_BSS_SELECT_ATTR_RSSI_ADJUST] = { .type = NLA_U8 },
>
> The RSSI_ADJUST here seems wrong in any case? Should've been NLA_U16
> now?
It should have, yes.
>> @@ -5753,6 +5778,42 @@ static int validate_scan_freqs(struct nlattr
>> *freqs)
>> return n_channels;
>> }
>>
>> +static int parse_bss_select(struct nlattr *nla,
>> + struct cfg80211_bss_selection
>> *bss_select)
>> +{
>> + struct nlattr *attr[NL80211_BSS_SELECT_ATTR_MAX + 1];
>> + u16 band_delta;
>> + int err;
>
> This should perhaps reject specification of multiple attributes, since
> otherwise the order of the code here dictates which one "wins".
I was waiting for your opinion on this as it did not feel right to me
either.
> But these are small things - looks good!
Thanks. Will work on final patch (famous last words).
Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
