On 21 January 2014 16:52, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Mon, 2014-01-20 at 15:09 +0100, Michal Kazior wrote:
>
>> @@ -2583,6 +2588,7 @@ enum wiphy_flags {
>> WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL = BIT(21),
>> WIPHY_FLAG_SUPPORTS_5_10_MHZ = BIT(22),
>> WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
>> + WIPHY_FLAG_HAS_MULTI_IF_CHSWITCH = BIT(24),
>> };
>
> You could probably use an nl80211 feature flag directly, see e.g.
> NL80211_FEATURE_HT_IBSS (enum nl80211_feature_flags) and save the code
> to set the extra flag attribute.
>
>> + * @NL80211_ATTR_CH_SWITCH_IFACES: Nested attribute with channel switch
>> + * settings in each entry (ifindex, frequency, beacon IEs). Also used as a
>> + * device capability flag in nl80211_send_wiphy().
>
> Which would obviously change this.
>
>> +int ieee80211_channel_switch(struct wiphy *wiphy,
>> + struct cfg80211_csa_settings *params,
>> + int num_params)
>> +{
>> + struct ieee80211_sub_if_data *sdata;
>> + int err;
>> +
>> + /* multi-vif CSA is not implemented */
>> + if (num_params > 1)
>> + return -EOPNOTSUPP;
>
> The capability flag should be checked in cfg80211, so this shouldn't be
> needed?
Okay.
>> +int ieee80211_channel_switch(struct wiphy *wiphy,
>> + struct cfg80211_csa_settings *params,
>> + int num_params);
>
> This function can be static now as the other places call the __ version
> now. Careful with locking though - you just moved it as well, won't the
> callers have to be updated?
Good point. I'll make it static.
ieee80211_channel_switch() was always called with the assumption
wdev->mtx is held. Only cfg80211_channel_switch() caller is changed
wrt to locking of wdev->mtx (since you can't take a bunch of wdev
locks just like that..). That's why other callers use __ version now.
>> + /* static variables avoid waste of stack size - this
>> * function is called under RTNL lock, so this should not be a problem.
>> */
>> static struct nlattr *csa_attrs[NL80211_ATTR_MAX+1];
>
> Technically we could even just reuse family->attrs since we're also
> under genl_lock :)
>
>> - u8 radar_detect_width = 0;
>> + struct net_device *dev = NULL;
>> + struct wireless_dev *wdev;
>> int err;
>> bool need_new_beacon = false;
>>
>> - if (!rdev->ops->channel_switch ||
>> - !(rdev->wiphy.flags & WIPHY_FLAG_HAS_CHANNEL_SWITCH))
>> - return -EOPNOTSUPP;
>> + ASSERT_RTNL();
>> +
>> + if (!attrs[NL80211_ATTR_IFINDEX])
>> + return -EINVAL;
>
> Hmm, shouldn't you use the ifindex from the csa_attrs? Or are you
> requiring one to be in the outer data to identify the wiphy? But that
> seems odd too, you can just get the wiphy in whatever way by asking the
> pre_doit for that?
I'm not sure I understand you.
There are two command variants now:
1: the old one, which has ifindex in the root
2: the new one, which has wiphy in the root, and an array of (1)
within CH_SWITCH_IFACES (and each entry has ifindex at its root)
>> + nla_for_each_nested(attrs,
>> + info->attrs[NL80211_ATTR_CH_SWITCH_IFACES],
>> + tmp) {
>> + nla_parse(csa_attrs, NL80211_ATTR_MAX, nla_data(attrs),
>> + nla_len(attrs), nl80211_policy);
>
> I think nla_parse() can fail, you should check that, otherwise there's
> no full validity checking of the attributes contained in it I believe.
Good point. I'll fix that.
Michał
--
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
