Country code setting ignored on bootup

Country code setting ignored on bootup

[Sven Eckelmann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2154 bytes --]

Hi,

just tested here with HK01 based board. The firwmare 
WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1 doesn't show this problematic 
behavior all the time but 2.4.0.1.r1-00026-QCAHKSWPL_SILICONZ-2 or WLAN.HK.
2.4.0.1.r1-00019-QCAHKSWPL_SILICONZ-1 does.

With 2.1.0.1 I could basically do following:

    iw reg set CA
    echo "c000000.wifi1" > /sys/bus/platform/drivers/ath11k/unbind
    echo "c000000.wifi1" > /sys/bus/platform/drivers/ath11k/bind

and the global and self managed country codes were still all CA (in many 
tests). But with 2.4.0.1, they will end up with CA as global and US as self 
managed. I can see that the country code change is correctly send by cfg80211 
when the HW is initializes but the 2.4.0.1 firmware just ignores it.

I can see following send command by ath11k in ath11k_wmi_send_init_country_cmd:

    00000000: 38 00 61 02 00 00 00 00 00 00 00 00 43 41 00 00

and I also receive a CA answer for that. But the response from the firmware comes
in when the ATH11K_FLAG_REGISTERED bit is still not set. Thus it is rejected by
ath11k_reg_chan_list_event for the regd_update_work.

This seems to be a race between receiving the 
"ATH11K_QMI_EVENT_FW_READY" event and the ieee80211_register_hw() in 
__ath11k_mac_register. The ieee80211_register_hw will make the hw responsible 
for handling the regd updates but they will not be correctly processed until 
the ATH11K_QMI_EVENT_FW_READY was handled by the firmware. And if the regd 
change was processed after ath11k_regd_update was called in 
__ath11k_mac_register (but before the ATH11K_QMI_EVENT_FW_READY was processed)
then we will just loose the regd change.

So I am a little bit baffled why ATH11K_FLAG_REGISTERED is bound to something 
which is not related to the  IEEE802211 registration code. Sounds to me like 
somebody wanted a FW_READY flag but misused the ATH11K_FLAG_REGISTERED flag 
for this purpose.

Btw. there are similar problems with the use of ATH11K_FLAG_REGISTERED in context
of ath11k_debug_pdev_create, ath11k_mac_register and ath11k_mac_allocate.

Maybe something like the attached patch could be used to improve this situation.

Kind regards,
	Sven

[-- Attachment #1.1.2: 0001-ath11k-Accept-new-regdomain-during-initialization.patch --]
[-- Type: text/x-patch, Size: 3687 bytes --]

From 931e5eb59e64804f3a90598b26c56643c43deb35 Mon Sep 17 00:00:00 2001
From: Sven Eckelmann <sven@narfation.org>
Date: Fri, 30 Oct 2020 12:02:21 +0100
Subject: [RFC PATCH] ath11k: Accept new regdomain during initialization

The driver is registering as iee80211_hw with its OPs and is then able to
be called by the upper layer. This for example happens early in the phase
when the correct regulary domain should be set. But the regulary domain
will only be accepted when the ATH11K_FLAG_REGISTERED flag was set after
the ATH11K_QMI_EVENT_FW_READY was processed. So it can easily happen that
the regularly domain is not correctly processed when
ATH11K_QMI_EVENT_FW_READY isn't handled immediately:

  $ iw reg set CA
  $ iw reg get|grep country
  country CA: DFS-FCC
  country CA: DFS-FCC
  country CA: DFS-FCC

  $ echo "c000000.wifi1" > /sys/bus/platform/drivers/ath11k/unbind
  $ echo "c000000.wifi1" > /sys/bus/platform/drivers/ath11k/bind
  $ iw reg get|grep country
  country CA: DFS-FCC
  country US: DFS-FCC
  country US: DFS-FCC

It is therefore essential to accept the regulatory changes without having
seen the ATH11K_QMI_EVENT_FW_READY. And since there are also potentially
more problems in ath11k_debug_pdev_create, ath11k_mac_register and
ath11k_mac_allocate with their use of ATH11K_FLAG_REGISTERED, it is better
to move the ATH11K_QMI_EVENT_FW_READY. to a new flag.

Tested with WLAN.HK.2.4.0.1.r1-00019-QCAHKSWPL_SILICONZ-1

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/net/wireless/ath/ath11k/core.h | 2 +-
 drivers/net/wireless/ath/ath11k/mac.c  | 2 ++
 drivers/net/wireless/ath/ath11k/qmi.c  | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index 18b97420f0d8..1d214eed9ea1 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -170,7 +170,7 @@ enum ath11k_scan_state {
 
 enum ath11k_dev_flags {
 	ATH11K_CAC_RUNNING,
-	ATH11K_FLAG_CORE_REGISTERED,
+	ATH11K_FLAG_FW_READY,
 	ATH11K_FLAG_CRASH_FLUSH,
 	ATH11K_FLAG_RAW_MODE,
 	ATH11K_FLAG_HW_CRYPTO_DISABLED,
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 7f8dd47d2333..00aca46505a6 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -6280,6 +6280,8 @@ static int __ath11k_mac_register(struct ath11k *ar)
 		 */
 		ar->hw->wiphy->interface_modes &= ~BIT(NL80211_IFTYPE_MONITOR);
 
+	set_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags);
+
 	/* Apply the regd received during initialization */
 	ret = ath11k_regd_update(ar, true);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index c2b165158225..417f63d9fb6d 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -2612,7 +2612,7 @@ static void ath11k_qmi_driver_event_work(struct work_struct *work)
 			ath11k_qmi_event_load_bdf(qmi);
 			break;
 		case ATH11K_QMI_EVENT_FW_READY:
-			if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags)) {
+			if (test_bit(ATH11K_FLAG_FW_READY, &ab->dev_flags)) {
 				ath11k_hal_dump_srng_stats(ab);
 				queue_work(ab->workqueue, &ab->restart_work);
 				break;
@@ -2620,7 +2620,7 @@ static void ath11k_qmi_driver_event_work(struct work_struct *work)
 
 			ath11k_core_qmi_firmware_ready(ab);
 			ab->qmi.cal_done = 1;
-			set_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags);
+			set_bit(ATH11K_FLAG_FW_READY, &ab->dev_flags);
 
 			break;
 		case ATH11K_QMI_EVENT_COLD_BOOT_CAL_DONE:
-- 
2.28.0


[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 102 bytes --]

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: ath11k: Country code setting ignored on bootup

[Sriram R]

> This seems to be a race between receiving the
> "ATH11K_QMI_EVENT_FW_READY" event and the ieee80211_register_hw() in
> __ath11k_mac_register. The ieee80211_register_hw will make the hw
> responsible for handling the regd updates but they will not be
> correctly processed until the ATH11K_QMI_EVENT_FW_READY was handled by
> the firmware. And if the regd change was processed after
> ath11k_regd_update was called in __ath11k_mac_register (but before the
> ATH11K_QMI_EVENT_FW_READY was processed) then we will just loose the
> regd change.
Agree that there is a possibility of a race between FW reg event
for user regd update and where the ATH11K_REGISTERED flag gets
set in the above scenario.
> 
> So I am a little bit baffled why ATH11K_FLAG_REGISTERED is bound to
> something which is not related to the  IEEE802211 registration code.
> Sounds to me like somebody wanted a FW_READY flag but misused the
> ATH11K_FLAG_REGISTERED flag for this purpose.
> 
> Btw. there are similar problems with the use of ATH11K_FLAG_REGISTERED
> in context of ath11k_debug_pdev_create, ath11k_mac_register and
> ath11k_mac_allocate.
> 
> Maybe something like the attached patch could be used to improve this 
> situation.
> 
With and without this patch, i also see there are slight chances the
default regd (provided by fw during 
init) can get overwritten by new reg rules
from FW in similar scenario you had mentioned.
But, Overwriting of default regd is not allowed.

I get a feeling that we can remove the use of ATH11k_FLAG_REGISTERED for 
the
purpose of checking whether to update default or new_regd.

Since the default reg rules wil be received only once per pdev on FW 
load,
the above flag based check can be replaced with a check to see
if default_regd is already set, so that we can now update the new_regd. 
Also if the
new_regd is set, irrespective of the argument in ath11k_regd_update() we 
can use
the new_regd data then to update the reg info for the phy.

This would avoid both overwriting of the default regd as well as the 
race you
had mentioned and also a possibility of another case where default_regd 
overwrites new_regd
after the previous call to regulatory_set_wiphy_regd_sync_rtnl.

Do you see any issues?

btw, i dont see any possible issues in current usage of 
ATH11k_FLAG_REGISTERED in
  ath11k_debug_pdev_create, ath11k_mac_register and ath11k_mac_allocate
since they sequenced and fall within the same context. Do you face any 
issues?

Thanks,
Sriram.R

> Kind regards,
> Sven

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: ath11k: Country code setting ignored on bootup

[Sven Eckelmann]

[-- Attachment #1.1: Type: text/plain, Size: 603 bytes --]

On Wednesday, 4 November 2020 17:25:53 CET Sriram R wrote:
> btw, i dont see any possible issues in current usage of 
> ATH11k_FLAG_REGISTERED in
>   ath11k_debug_pdev_create, ath11k_mac_register and ath11k_mac_allocate
> since they sequenced and fall within the same context. Do you face any 
> issues?

A firmware crash before ATH11K_QMI_EVENT_FW_READY was processed/received. So 
we might want to recreate the mac/debugfs/... even when it was already created 
- at least this is the stuff mentioned in the commit(s) which introduce the 
ATH11k_FLAG_REGISTERED in these positions.

Kind regards,
	Sven

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 102 bytes --]

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k

Re: ath11k: Country code setting ignored on bootup

[Sriram R]

On 2020-11-04 22:00, Sven Eckelmann wrote:
> On Wednesday, 4 November 2020 17:25:53 CET Sriram R wrote:
>> btw, i dont see any possible issues in current usage of
>> ATH11k_FLAG_REGISTERED in
>>   ath11k_debug_pdev_create, ath11k_mac_register and 
>> ath11k_mac_allocate
>> since they sequenced and fall within the same context. Do you face any
>> issues?
> 
> A firmware crash before ATH11K_QMI_EVENT_FW_READY was 
> processed/received. So
> we might want to recreate the mac/debugfs/... even when it was already 
> created
> - at least this is the stuff mentioned in the commit(s) which introduce 
> the
> ATH11k_FLAG_REGISTERED in these positions.
Yeah right, i think it was added to avoid multiple driver registration.
The ATH11K_FLAG_REGISTERED currently implies core registration complete 
where
mac registration is one part of it. Agree that checking this flag in reg 
path
is not the right approach to decide if mac is registered and default 
rules are
applied for it.
I can also prepare a patch to avoid the issue you had mentioned and a
similar race as below (which i noted with this patch in my setup)and
see if it works fine for all use cases.
Please note below on the 4th line, the Default regd for pdev 0 gets
overwritten from US to IN(the country set before ath11k bind), which is 
not desirable.

[  949.056244] ath11k c000000.wifi1: Default REGD update pdev 0 US
[  949.056493] ath11k c000000.wifi1: Default REGD update pdev 1 US
[  949.056595] ath11k c000000.wifi1: Default REGD update pdev 2 US
[  949.061231] ath11k c000000.wifi1: Default REGD update pdev 0 IN
[  949.063915] ath11k c000000.wifi1: User REGD update pdev 1 IN
[  949.066156] ath11k c000000.wifi1: User REGD update pdev 2 IN

Thanks and Regards,
Sriram.R
> 
> Kind regards,
> 	Sven

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k