MIME-Version: 1.0
In-Reply-To: <20141113224556.GD24486@wotan.suse.de>
References: <1415895219-19848-1-git-send-email-arik@wizery.com> <20141113224556.GD24486@wotan.suse.de>
From: Arik Nemtsov <arik@wizery.com>
Date: Sun, 16 Nov 2014 13:00:16 +0200
Message-ID: <CA+XVXffuKE2heFPWmVWGre8cL3VZX5U63_Rru9uXKiqbpz5xYA@mail.gmail.com> (sfid-20141116_120040_576680_43486F1C)
Subject: Re: [PATCH v2 1/4] cfg80211: leave invalid channels on regdomain change
To: "Luis R. Rodriguez" <mcgrof@suse.com>
Cc: Jouni Malinen <jouni@qca.qualcomm.com>,
	Johannes Berg <johannes.berg@intel.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-wireless-owner@vger.kernel.org

On Fri, Nov 14, 2014 at 12:45 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> + * @REGULATORY_ENFORCE_CHANNELS: the regulatory core will make sure all
>> + *   interfaces on this wiphy reside on allowed channels. Upon a regdomain
>> + *   change, the interfaces are given a grace period to disconnect or move
>> + *   to an allowed channels. Interfaces on forbidden channels are forcibly
>> + *   disconnected.
>
> I don't like this name, it would seem folks not using this don't
> get to enforce channels, and that's not right, this is a feature,
> and in fact I am not sure why this is being implemented as optional
> rather than a standard feature. Care to explain the reasoning there?

This is a big change in behavior. It can hurt certification tests etc.
I believe a chip vendor should opt-in for this change. Otherwise we
risk bad user experience.

>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 7449a8c..6459ddd 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -56,6 +56,7 @@
>>  #include <net/cfg80211.h>
>>  #include "core.h"
>>  #include "reg.h"
>> +#include "rdev-ops.h"
>>  #include "regdb.h"
>>  #include "nl80211.h"
>>
>> @@ -66,6 +67,12 @@
>>  #define REG_DBG_PRINT(args...)
>>  #endif
>>
>> +/*
>> + * Grace period we give before making sure all current interfaces reside on
>> + * channels allowed by the current regulatory domain.
>> + */
>> +#define REG_ENFORCE_GRACE_MS 60000
>> +
>>  /**
>>   * enum reg_request_treatment - regulatory request treatment
>>   *
>> @@ -210,6 +217,9 @@ struct reg_beacon {
>>       struct ieee80211_channel chan;
>>  };
>>
>> +static void reg_check_chans_work(struct work_struct *work);
>> +static DECLARE_DELAYED_WORK(reg_check_chans, reg_check_chans_work);
>> +
>>  static void reg_todo(struct work_struct *work);
>>  static DECLARE_WORK(reg_work, reg_todo);
>>
>> @@ -1518,6 +1528,90 @@ static void reg_call_notifier(struct wiphy *wiphy,
>>               wiphy->reg_notifier(wiphy, request);
>>  }
>>
>> +static bool reg_wdev_chan_valid(struct wiphy *wiphy, struct wireless_dev *wdev)
>> +{
>> +     struct ieee80211_channel *ch;
>> +     struct cfg80211_chan_def chandef;
>> +     struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
>> +     bool ret = true;
>> +
>> +     wdev_lock(wdev);
>> +
>> +     if (!wdev->netdev || !netif_running(wdev->netdev))
>> +             goto out;
>> +
>> +     switch (wdev->iftype) {
>> +     case NL80211_IFTYPE_AP:
>> +     case NL80211_IFTYPE_P2P_GO:
>> +             if (!wdev->beacon_interval)
>> +                     goto out;
>> +
>> +             ret = cfg80211_reg_can_beacon(wiphy,
>> +                                           &wdev->chandef, wdev->iftype);
>> +             break;
>> +     case NL80211_IFTYPE_STATION:
>> +     case NL80211_IFTYPE_P2P_CLIENT:
>> +             if (!wdev->current_bss ||
>> +                 !wdev->current_bss->pub.channel)
>> +                     goto out;
>> +
>> +             ch = wdev->current_bss->pub.channel;
>> +             if (rdev->ops->get_channel &&
>> +                 !rdev_get_channel(rdev, wdev, &chandef))
>> +                     ret = cfg80211_chandef_usable(wiphy, &chandef,
>> +                                                   IEEE80211_CHAN_DISABLED);
>> +             else
>> +                     ret = !(ch->flags & IEEE80211_CHAN_DISABLED);
>> +             break;
>> +     default:
>> +             /* others not implemented for now */
>> +             pr_info("Regulatory channel check not implemented for mode %d\n",
>> +                     wdev->iftype);
>
> I feel you are being lazy here, come on, think of it and address it.
> It can't be that hard. In fact cfg80211_leave() already deals with
> all the logic to leave properly for all types of interfaces, you
> just have to come up with the logic to know things should kick
> the device off. Its not that hard.

I don't want to add modes I cannot test with HW I have. I think that's
irresponsible, especially when the side-effects are disconnections.
I can add IBSS as well with the HW I have, but that's about it.

Arik