diff mbox series

wifi: cfg80211: call reg_notifier for self managed wiphy from driver hint

Message ID 20221214093937.14987-1-quic_wgong@quicinc.com (mailing list archive)
State Superseded
Delegated to: Johannes Berg
Headers show
Series wifi: cfg80211: call reg_notifier for self managed wiphy from driver hint | expand

Commit Message

Wen Gong Dec. 14, 2022, 9:39 a.m. UTC 
Currently the regulatory driver does not call the regulatory callback
reg_notifier for self managed wiphys. Sometimes driver needs cfg80211
to calculate the info of ieee80211_channel such as flags and power,
and driver needs to get the info of ieee80211_channel after hint of
driver, but driver does not know when calculation of the info of
ieee80211_channel become finished, so add notify to driver after
reg_process_self_managed_hint() from cfg80211 is a good way, then
driver could get the correct info in callback of reg_notifier.

Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
---
 net/wireless/reg.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)


base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025

Comments

Wen Gong Jan. 3, 2023, 10:11 a.m. UTC | #1 
On 12/14/2022 5:39 PM, Wen Gong wrote:
> Currently the regulatory driver does not call the regulatory callback
> reg_notifier for self managed wiphys. Sometimes driver needs cfg80211
> to calculate the info of ieee80211_channel such as flags and power,
> and driver needs to get the info of ieee80211_channel after hint of
> driver, but driver does not know when calculation of the info of
> ieee80211_channel become finished, so add notify to driver after
> reg_process_self_managed_hint() from cfg80211 is a good way, then
> driver could get the correct info in callback of reg_notifier.
>
Hi Johannes,

Could I get your comment for this?
Johannes Berg Jan. 19, 2023, 1:52 p.m. UTC | #2 
On Wed, 2022-12-14 at 04:39 -0500, Wen Gong wrote:
> Currently the regulatory driver does not call the regulatory callback
> reg_notifier for self managed wiphys. Sometimes driver needs cfg80211
> to calculate the info of ieee80211_channel such as flags and power,
> and driver needs to get the info of ieee80211_channel after hint of
> driver, but driver does not know when calculation of the info of
> ieee80211_channel become finished, so add notify to driver after
> reg_process_self_managed_hint() from cfg80211 is a good way, then
> driver could get the correct info in callback of reg_notifier.

Seems reasonable - but maybe unexpected to some drivers, perhaps it
should be opt-in?

Though I guess not many drivers actually use this infrastructure in the
first place?

> @@ -3095,6 +3107,13 @@ static void notify_self_managed_wiphys(struct regulatory_request *request)
>  		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
>  		    request->initiator == NL80211_REGDOM_SET_BY_USER)
>  			reg_call_notifier(wiphy, request);
> +
> +		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
> +		    request->initiator == NL80211_REGDOM_SET_BY_DRIVER &&
> +		    request->wiphy_idx == get_wiphy_idx(wiphy)) {
> +			reg_call_notifier(wiphy, request);
> +			request->wiphy_idx = WIPHY_IDX_INVALID;
> +		}

Why set the request->wiphy_idx here? Should this even go through
reg_process_pending_hints() at all?

> +	driver_request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
> +	if (!driver_request)
> +		return;
> +
> +	memcpy(driver_request, &request, sizeof(*driver_request));

kmemdup()?

> +	queue_regulatory_request(driver_request);

But again not sure you should do this, rather than calling the notifier
directly?

I mean, you could just do reg_call_notifier() here, it's already async?

johannes
Wen Gong Jan. 31, 2023, 6:07 a.m. UTC | #3 
On 1/19/2023 9:52 PM, Johannes Berg wrote:
> On Wed, 2022-12-14 at 04:39 -0500, Wen Gong wrote:
>> Currently the regulatory driver does not call the regulatory callback
>> reg_notifier for self managed wiphys. Sometimes driver needs cfg80211
>> to calculate the info of ieee80211_channel such as flags and power,
>> and driver needs to get the info of ieee80211_channel after hint of
>> driver, but driver does not know when calculation of the info of
>> ieee80211_channel become finished, so add notify to driver after
>> reg_process_self_managed_hint() from cfg80211 is a good way, then
>> driver could get the correct info in callback of reg_notifier.
> Seems reasonable - but maybe unexpected to some drivers, perhaps it
> should be opt-in?
>
> Though I guess not many drivers actually use this infrastructure in the
> first place?

Yes, I will add a new flag such as WIPHY_FLAG_NOTIFY_REGDOM_BY_DRIVER 
for this driver.

is it ok?

>> @@ -3095,6 +3107,13 @@ static void notify_self_managed_wiphys(struct regulatory_request *request)
>>   		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
>>   		    request->initiator == NL80211_REGDOM_SET_BY_USER)
>>   			reg_call_notifier(wiphy, request);
>> +
>> +		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
>> +		    request->initiator == NL80211_REGDOM_SET_BY_DRIVER &&
>> +		    request->wiphy_idx == get_wiphy_idx(wiphy)) {
>> +			reg_call_notifier(wiphy, request);
>> +			request->wiphy_idx = WIPHY_IDX_INVALID;
>> +		}
> Why set the request->wiphy_idx here? Should this even go through
> reg_process_pending_hints() at all?

it is to skip handle for NL80211_REGDOM_SET_BY_DRIVER in 
reg_process_pending_hints()/reg_process_hint().

After change to use reg_call_notifier(), then it is not need again.

>
>> +	driver_request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
>> +	if (!driver_request)
>> +		return;
>> +
>> +	memcpy(driver_request, &request, sizeof(*driver_request));
> kmemdup()?

yes.

After change to use reg_call_notifier(), then it is not need again.

>
>> +	queue_regulatory_request(driver_request);
> But again not sure you should do this, rather than calling the notifier
> directly?
>
> I mean, you could just do reg_call_notifier() here, it's already async?
Yes, I will change to use reg_call_notifier() here, then it will be simple.
> johannes
>
Johannes Berg Jan. 31, 2023, 9:12 a.m. UTC | #4 
On Tue, 2023-01-31 at 14:07 +0800, Wen Gong wrote:
> On 1/19/2023 9:52 PM, Johannes Berg wrote:
> > On Wed, 2022-12-14 at 04:39 -0500, Wen Gong wrote:
> > > Currently the regulatory driver does not call the regulatory callback
> > > reg_notifier for self managed wiphys. Sometimes driver needs cfg80211
> > > to calculate the info of ieee80211_channel such as flags and power,
> > > and driver needs to get the info of ieee80211_channel after hint of
> > > driver, but driver does not know when calculation of the info of
> > > ieee80211_channel become finished, so add notify to driver after
> > > reg_process_self_managed_hint() from cfg80211 is a good way, then
> > > driver could get the correct info in callback of reg_notifier.
> > Seems reasonable - but maybe unexpected to some drivers, perhaps it
> > should be opt-in?
> > 
> > Though I guess not many drivers actually use this infrastructure in the
> > first place?
> 
> Yes, I will add a new flag such as WIPHY_FLAG_NOTIFY_REGDOM_BY_DRIVER 
> for this driver.
> 
> is it ok?

Sounds OK.

johannes
diff mbox series

Patch

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4f3f31244e8b..e3f500832427 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -3085,6 +3085,18 @@  static void reg_process_hint(struct regulatory_request *reg_request)
 	reg_free_request(reg_request);
 }
 
+static void queue_regulatory_request(struct regulatory_request *request)
+{
+	request->alpha2[0] = toupper(request->alpha2[0]);
+	request->alpha2[1] = toupper(request->alpha2[1]);
+
+	spin_lock(&reg_requests_lock);
+	list_add_tail(&request->list, &reg_requests_list);
+	spin_unlock(&reg_requests_lock);
+
+	schedule_work(&reg_work);
+}
+
 static void notify_self_managed_wiphys(struct regulatory_request *request)
 {
 	struct cfg80211_registered_device *rdev;
@@ -3095,6 +3107,13 @@  static void notify_self_managed_wiphys(struct regulatory_request *request)
 		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
 		    request->initiator == NL80211_REGDOM_SET_BY_USER)
 			reg_call_notifier(wiphy, request);
+
+		if (wiphy->regulatory_flags & REGULATORY_WIPHY_SELF_MANAGED &&
+		    request->initiator == NL80211_REGDOM_SET_BY_DRIVER &&
+		    request->wiphy_idx == get_wiphy_idx(wiphy)) {
+			reg_call_notifier(wiphy, request);
+			request->wiphy_idx = WIPHY_IDX_INVALID;
+		}
 	}
 }
 
@@ -3172,6 +3191,7 @@  static void reg_process_self_managed_hint(struct wiphy *wiphy)
 	const struct ieee80211_regdomain *regd;
 	enum nl80211_band band;
 	struct regulatory_request request = {};
+	struct regulatory_request *driver_request;
 
 	ASSERT_RTNL();
 	lockdep_assert_wiphy(wiphy);
@@ -3199,6 +3219,13 @@  static void reg_process_self_managed_hint(struct wiphy *wiphy)
 	request.initiator = NL80211_REGDOM_SET_BY_DRIVER;
 
 	nl80211_send_wiphy_reg_change_event(&request);
+
+	driver_request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
+	if (!driver_request)
+		return;
+
+	memcpy(driver_request, &request, sizeof(*driver_request));
+	queue_regulatory_request(driver_request);
 }
 
 static void reg_process_self_managed_hints(void)
@@ -3225,18 +3252,6 @@  static void reg_todo(struct work_struct *work)
 	rtnl_unlock();
 }
 
-static void queue_regulatory_request(struct regulatory_request *request)
-{
-	request->alpha2[0] = toupper(request->alpha2[0]);
-	request->alpha2[1] = toupper(request->alpha2[1]);
-
-	spin_lock(&reg_requests_lock);
-	list_add_tail(&request->list, &reg_requests_list);
-	spin_unlock(&reg_requests_lock);
-
-	schedule_work(&reg_work);
-}
-
 /*
  * Core regulatory hint -- happens during cfg80211_init()
  * and when we restore regulatory settings.