| Message ID | 20221214093937.14987-1-quic_wgong@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: cfg80211: call reg_notifier for self managed wiphy from driver hint | expand |
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?
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
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 >
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 --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(®_requests_lock); + list_add_tail(&request->list, ®_requests_list); + spin_unlock(®_requests_lock); + + schedule_work(®_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(®_requests_lock); - list_add_tail(&request->list, ®_requests_list); - spin_unlock(®_requests_lock); - - schedule_work(®_work); -} - /* * Core regulatory hint -- happens during cfg80211_init() * and when we restore regulatory settings.
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