[PATCH v2 1/2] cfg80211: introduce sync regdom set API for self-managed

[PATCH v2 1/2] cfg80211: introduce sync regdom set API for self-managed

[Arik Nemtsov]

A self-managed device will sometimes need to set its regdomain synchronously.
Notably it should be set before usermode has a chance to query it. Expose
a new API to accomplish this which requires the RTNL.

Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
Reviewed-by: Ilan Peer <ilan.peer@intel.com>
Reviewed-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
 include/net/cfg80211.h | 14 ++++++++++++++
 net/wireless/reg.c     | 31 +++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f38645f..39d102d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3830,6 +3830,20 @@ int regulatory_set_wiphy_regd(struct wiphy *wiphy,
 			      struct ieee80211_regdomain *rd);
 
 /**
+ * regulatory_set_wiphy_regd_sync_rtnl - set regdom for self-managed drivers
+ * @wiphy: the wireless device we want to process the regulatory domain on
+ * @rd: the regulatory domain information to use for this wiphy
+ *
+ * This functions requires the RTNL to be held and applies the new regdomain
+ * synchronously to this wiphy. For more details see
+ * regulatory_set_wiphy_regd().
+ *
+ * Return: 0 on success. -EINVAL, -EPERM
+ */
+int regulatory_set_wiphy_regd_sync_rtnl(struct wiphy *wiphy,
+					struct ieee80211_regdomain *rd);
+
+/**
  * wiphy_apply_custom_regulatory - apply a custom driver regulatory domain
  * @wiphy: the wireless device we want to process the regulatory domain on
  * @regd: the custom regulatory domain to use for this wiphy
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index fde4e17..521f3a4 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2909,8 +2909,8 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 	return 0;
 }
 
-int regulatory_set_wiphy_regd(struct wiphy *wiphy,
-			      struct ieee80211_regdomain *rd)
+static int __regulatory_set_wiphy_regd(struct wiphy *wiphy,
+				       struct ieee80211_regdomain *rd)
 {
 	const struct ieee80211_regdomain *regd;
 	const struct ieee80211_regdomain *prev_regd;
@@ -2940,12 +2940,39 @@ int regulatory_set_wiphy_regd(struct wiphy *wiphy,
 	spin_unlock(&reg_requests_lock);
 
 	kfree(prev_regd);
+	return 0;
+}
+
+int regulatory_set_wiphy_regd(struct wiphy *wiphy,
+			      struct ieee80211_regdomain *rd)
+{
+	int ret = __regulatory_set_wiphy_regd(wiphy, rd);
+
+	if (ret)
+		return ret;
 
 	schedule_work(&reg_work);
 	return 0;
 }
 EXPORT_SYMBOL(regulatory_set_wiphy_regd);
 
+int regulatory_set_wiphy_regd_sync_rtnl(struct wiphy *wiphy,
+					struct ieee80211_regdomain *rd)
+{
+	int ret;
+
+	ASSERT_RTNL();
+
+	ret = __regulatory_set_wiphy_regd(wiphy, rd);
+	if (ret)
+		return ret;
+
+	/* process the request immediately */
+	reg_process_self_managed_hints();
+	return 0;
+}
+EXPORT_SYMBOL(regulatory_set_wiphy_regd_sync_rtnl);
+
 void wiphy_regulatory_register(struct wiphy *wiphy)
 {
 	struct regulatory_request *lr;
-- 
2.1.0

[PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Arik Nemtsov]

When a system contains only self-managed regulatory devices all hints
from the regulatory core are ignored. Stop hint processing early in this
case. These systems usually don't have CRDA deployed, which results in
endless (irrelevent) logs of the form:
cfg80211: Calling CRDA to update world regulatory domain

Make sure there's at least one self-managed device before discarding a
hint, in order to prevent initial hints from disappearing on CRDA
managed systems.

Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
---
 net/wireless/reg.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 521f3a4..588e45f 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2120,6 +2120,26 @@ out_free:
 	reg_free_request(reg_request);
 }
 
+static bool reg_only_self_managed_wiphys(void)
+{
+	struct cfg80211_registered_device *rdev;
+	struct wiphy *wiphy;
+	bool self_managed_found = false;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
+		wiphy = &rdev->wiphy;
+		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
+			self_managed_found = true;
+		else
+			return false;
+	}
+
+	/* make sure at least one self-managed wiphy exists */
+	return self_managed_found;
+}
+
 /*
  * Processes regulatory hints, this is all the NL80211_REGDOM_SET_BY_*
  * Regulatory hints come on a first come first serve basis and we
@@ -2151,6 +2171,11 @@ static void reg_process_pending_hints(void)
 
 	spin_unlock(&reg_requests_lock);
 
+	if (reg_only_self_managed_wiphys()) {
+		reg_free_request(reg_request);
+		return;
+	}
+
 	reg_process_hint(reg_request);
 }
 
-- 
2.1.0

Re: [PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Julian Calaby]

Hi Arik,

On Thu, Jan 8, 2015 at 1:47 AM, Arik Nemtsov <arik@wizery.com> wrote:
> When a system contains only self-managed regulatory devices all hints
> from the regulatory core are ignored. Stop hint processing early in this
> case. These systems usually don't have CRDA deployed, which results in
> endless (irrelevent) logs of the form:
> cfg80211: Calling CRDA to update world regulatory domain
>
> Make sure there's at least one self-managed device before discarding a
> hint, in order to prevent initial hints from disappearing on CRDA
> managed systems.
>
> Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
> ---
>  net/wireless/reg.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 521f3a4..588e45f 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -2120,6 +2120,26 @@ out_free:
>         reg_free_request(reg_request);
>  }
>
> +static bool reg_only_self_managed_wiphys(void)
> +{
> +       struct cfg80211_registered_device *rdev;
> +       struct wiphy *wiphy;
> +       bool self_managed_found = false;
> +
> +       ASSERT_RTNL();

Would it make sense to quickly return false here if the list is empty
rather than the whole mess with the new variable?

> +
> +       list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
> +               wiphy = &rdev->wiphy;
> +               if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED)
> +                       self_managed_found = true;
> +               else
> +                       return false;
> +       }
> +
> +       /* make sure at least one self-managed wiphy exists */
> +       return self_managed_found;
> +}
> +
>  /*
>   * Processes regulatory hints, this is all the NL80211_REGDOM_SET_BY_*
>   * Regulatory hints come on a first come first serve basis and we

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Arik Nemtsov]

On Tue, Jan 13, 2015 at 6:26 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Arik,
>
> On Thu, Jan 8, 2015 at 1:47 AM, Arik Nemtsov <arik@wizery.com> wrote:
>> When a system contains only self-managed regulatory devices all hints
>> from the regulatory core are ignored. Stop hint processing early in this
>> case. These systems usually don't have CRDA deployed, which results in
>> endless (irrelevent) logs of the form:
>> cfg80211: Calling CRDA to update world regulatory domain
>>
>> Make sure there's at least one self-managed device before discarding a
>> hint, in order to prevent initial hints from disappearing on CRDA
>> managed systems.
>>
>> Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
>> ---
>>  net/wireless/reg.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index 521f3a4..588e45f 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -2120,6 +2120,26 @@ out_free:
>>         reg_free_request(reg_request);
>>  }
>>
>> +static bool reg_only_self_managed_wiphys(void)
>> +{
>> +       struct cfg80211_registered_device *rdev;
>> +       struct wiphy *wiphy;
>> +       bool self_managed_found = false;
>> +
>> +       ASSERT_RTNL();
>
> Would it make sense to quickly return false here if the list is empty
> rather than the whole mess with the new variable?

I'm thinking the "mess" isn't really such a mess - are you expecting a
real performance hit?

Also this is a corner case - you won't really get regulatory updates
when no network cards are presents. You only get a single update to
the core.

Arik

Re: [PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Julian Calaby]

Hi Arik,

On Tue, Jan 13, 2015 at 9:11 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Tue, Jan 13, 2015 at 6:26 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Arik,
>>
>> On Thu, Jan 8, 2015 at 1:47 AM, Arik Nemtsov <arik@wizery.com> wrote:
>>> When a system contains only self-managed regulatory devices all hints
>>> from the regulatory core are ignored. Stop hint processing early in this
>>> case. These systems usually don't have CRDA deployed, which results in
>>> endless (irrelevent) logs of the form:
>>> cfg80211: Calling CRDA to update world regulatory domain
>>>
>>> Make sure there's at least one self-managed device before discarding a
>>> hint, in order to prevent initial hints from disappearing on CRDA
>>> managed systems.
>>>
>>> Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
>>> ---
>>>  net/wireless/reg.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>>> index 521f3a4..588e45f 100644
>>> --- a/net/wireless/reg.c
>>> +++ b/net/wireless/reg.c
>>> @@ -2120,6 +2120,26 @@ out_free:
>>>         reg_free_request(reg_request);
>>>  }
>>>
>>> +static bool reg_only_self_managed_wiphys(void)
>>> +{
>>> +       struct cfg80211_registered_device *rdev;
>>> +       struct wiphy *wiphy;
>>> +       bool self_managed_found = false;
>>> +
>>> +       ASSERT_RTNL();
>>
>> Would it make sense to quickly return false here if the list is empty
>> rather than the whole mess with the new variable?
>
> I'm thinking the "mess" isn't really such a mess - are you expecting a
> real performance hit?

Personally, purely for readability, I prefer the style of checking and
returning as early as possible.

Arguably doing it my way trades speed for memory efficiency and your
way trades memory efficiency for speed, but it's so small (and this is
so far from a hot path) that there's no real argument either way
whatsoever.

> Also this is a corner case - you won't really get regulatory updates
> when no network cards are presents. You only get a single update to
> the core.

Very true.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Arik Nemtsov]

On Tue, Jan 13, 2015 at 12:30 PM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Arik,
>
> On Tue, Jan 13, 2015 at 9:11 PM, Arik Nemtsov <arik@wizery.com> wrote:
>> On Tue, Jan 13, 2015 at 6:26 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>>> Hi Arik,
>>>
>>> On Thu, Jan 8, 2015 at 1:47 AM, Arik Nemtsov <arik@wizery.com> wrote:
>>>> When a system contains only self-managed regulatory devices all hints
>>>> from the regulatory core are ignored. Stop hint processing early in this
>>>> case. These systems usually don't have CRDA deployed, which results in
>>>> endless (irrelevent) logs of the form:
>>>> cfg80211: Calling CRDA to update world regulatory domain
>>>>
>>>> Make sure there's at least one self-managed device before discarding a
>>>> hint, in order to prevent initial hints from disappearing on CRDA
>>>> managed systems.
>>>>
>>>> Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
>>>> ---
>>>>  net/wireless/reg.c | 25 +++++++++++++++++++++++++
>>>>  1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>>>> index 521f3a4..588e45f 100644
>>>> --- a/net/wireless/reg.c
>>>> +++ b/net/wireless/reg.c
>>>> @@ -2120,6 +2120,26 @@ out_free:
>>>>         reg_free_request(reg_request);
>>>>  }
>>>>
>>>> +static bool reg_only_self_managed_wiphys(void)
>>>> +{
>>>> +       struct cfg80211_registered_device *rdev;
>>>> +       struct wiphy *wiphy;
>>>> +       bool self_managed_found = false;
>>>> +
>>>> +       ASSERT_RTNL();
>>>
>>> Would it make sense to quickly return false here if the list is empty
>>> rather than the whole mess with the new variable?
>>
>> I'm thinking the "mess" isn't really such a mess - are you expecting a
>> real performance hit?
>
> Personally, purely for readability, I prefer the style of checking and
> returning as early as possible.

Actually I thought readability was on my side - wanted to point out
the shorter version of the code is more legible.
I guess it's a matter of personal taste :)

>
> Arguably doing it my way trades speed for memory efficiency and your
> way trades memory efficiency for speed, but it's so small (and this is
> so far from a hot path) that there's no real argument either way
> whatsoever.

Agree it doesn't matter for these reasons.

Arik

Re: [PATCH v2 2/2] cfg80211: avoid reg-hints in self-managed only systems

[Julian Calaby]

Hi Arik,

On Tue, Jan 13, 2015 at 9:35 PM, Arik Nemtsov <arik@wizery.com> wrote:
> On Tue, Jan 13, 2015 at 12:30 PM, Julian Calaby <julian.calaby@gmail.com> wrote:
>> Hi Arik,
>>
>> On Tue, Jan 13, 2015 at 9:11 PM, Arik Nemtsov <arik@wizery.com> wrote:
>>> On Tue, Jan 13, 2015 at 6:26 AM, Julian Calaby <julian.calaby@gmail.com> wrote:
>>>> Hi Arik,
>>>>
>>>> On Thu, Jan 8, 2015 at 1:47 AM, Arik Nemtsov <arik@wizery.com> wrote:
>>>>> When a system contains only self-managed regulatory devices all hints
>>>>> from the regulatory core are ignored. Stop hint processing early in this
>>>>> case. These systems usually don't have CRDA deployed, which results in
>>>>> endless (irrelevent) logs of the form:
>>>>> cfg80211: Calling CRDA to update world regulatory domain
>>>>>
>>>>> Make sure there's at least one self-managed device before discarding a
>>>>> hint, in order to prevent initial hints from disappearing on CRDA
>>>>> managed systems.
>>>>>
>>>>> Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
>>>>> ---
>>>>>  net/wireless/reg.c | 25 +++++++++++++++++++++++++
>>>>>  1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>>>>> index 521f3a4..588e45f 100644
>>>>> --- a/net/wireless/reg.c
>>>>> +++ b/net/wireless/reg.c
>>>>> @@ -2120,6 +2120,26 @@ out_free:
>>>>>         reg_free_request(reg_request);
>>>>>  }
>>>>>
>>>>> +static bool reg_only_self_managed_wiphys(void)
>>>>> +{
>>>>> +       struct cfg80211_registered_device *rdev;
>>>>> +       struct wiphy *wiphy;
>>>>> +       bool self_managed_found = false;
>>>>> +
>>>>> +       ASSERT_RTNL();
>>>>
>>>> Would it make sense to quickly return false here if the list is empty
>>>> rather than the whole mess with the new variable?
>>>
>>> I'm thinking the "mess" isn't really such a mess - are you expecting a
>>> real performance hit?
>>
>> Personally, purely for readability, I prefer the style of checking and
>> returning as early as possible.
>
> Actually I thought readability was on my side - wanted to point out
> the shorter version of the code is more legible.
> I guess it's a matter of personal taste :)

Indeed it is.

In terms of length, your version is a variable definition and two
lines in the internal if statement. Mine is a blank line and two lines
in a separate if statement. (Assuming the condition doesn't run over
80 chars)

>> Arguably doing it my way trades speed for memory efficiency and your
>> way trades memory efficiency for speed, but it's so small (and this is
>> so far from a hot path) that there's no real argument either way
>> whatsoever.
>
> Agree it doesn't matter for these reasons.

At the end of the day, it's a stalemate for a lot of reasons. You
prefer this way, it's your code, so leave it as it is. I apologise for
the noise =)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH v2 1/2] cfg80211: introduce sync regdom set API for self-managed

[Johannes Berg]

Both applied.

johannes