zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.71k stars 6.54k forks source link

wifi: nrf70: Regulatory handling broken #79916

Open kapbh opened 5 days ago

kapbh commented 5 days ago

Describe the bug

The wifi command wifi reg_domain is not setting the regulatory domain. To Reproduce

  1. Build with board: west build -p -b nrf7002dk/nrf5340/cpuapp
  2. Give command: wifi reg_domain IN -f
  3. Error: Cannot set Regulatory domain: -1

Expected behavior

It should set the regulatory domain: Wi-Fi Regulatory domain set to: IN

Logs and console output

wifi reg_domain IN -f Cannot set Regulatory domain: -1 Environment (please complete the following information):

github-actions[bot] commented 5 days ago

Hi @kapbh! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

krish2718 commented 4 days ago

Thanks @kapbh, I have triaged the issue:

  1. Due to missing CONFIG_WPA_CLI, set is treated as set_network and command fails, this was already found in WPA3 debug, should be fixed by https://github.com/zephyrproject-rtos/zephyr/pull/79739 (fix is part of hostap)
  2. nRF70 should implement the newly added set_country ops in https://github.com/zephyrproject-rtos/zephyr/commit/5a195001e393fcfbb48111f8376e45b2bb63b233 which broke the regulatory in nrf70, but this design is still incorrect and has below issues
    • get_country is unused in WPA supplicant and is unused
    • supp_api.c uses wifi_mgmt_api->reg_domain for GET and wpa_cli set country for SET which is incorrect because
      • the SET ignores the force flag
      • Regulatory and country are not interchangeable, country is a subset of regulatory information.
      • the reg_domain cannot be removed because it is still used by GET

But as SET is the main operation here and is same for both regulatory and country, so, we can live with the current design, just need to see how to fix force flag and also maybe remove get_country. @Rex-Chen-NXP

krish2718 commented 4 days ago

We also should start enforcing fixes for all drivers when making such changes to avoid breakages.

Rex-Chen-NXP commented 4 days ago

Thanks @kapbh, I have triaged the issue:

  1. Due to missing , is treated as and command fails, this was already found in WPA3 debug, should be fixed by CONFIG_WPA_CLI``set``set_networkmodules: hostap: Fix memory leak of network #79739 (fix is part of hostap)
  2. nRF70 should implement the newly added ops in 5a19500 which broke the regulatory in nrf70, but this design is still incorrect and has below issues set_country
    • get_country is unused in WPA supplicant and is unused
    • supp_api.c uses for and for which is incorrect because wifi_mgmt_api->reg_domain``GET``wpa_cli set country``SET
      • the ignores the flagSET``force
      • Regulatory and are not interchangeable, is a subset of information.country``country``regulatory
      • the cannot be removed because it is still used by reg_domain``GET

But as is the main operation here and is same for both and , so, we can live with the current design, just need to see how to fix flag and also maybe remove . @Rex-Chen-NXPSET``regulatory``country``force``get_country

Yes, the previous changes of country from my side with below reasons:

  1. For GET option, don't support by ctrl interface, so I hook it to vendor driver implementation.
  2. I don't know what option -f means, so I didn't deal it.

If any suggestion or need fix pls ping me.

krish2718 commented 4 days ago

I don't know what option -f means, so I didn't deal it.

If you see the shell help, this basically means that disable any regulatory hints (e.g., beacon) and use the reg_domain from user as a only hint.

Problem is, supplicant doesn't have this concept, so, we cannot implement this using set country. This can only be implemented using vendor implementation, so, may be we just add a note to this effect and leave it? @jukkar WDYT?