Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hostap: Relocate hostapd related source code to new files #83253

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nxf58150
Copy link
Contributor

Created new files and relocate hostapd support code in glue layer to new files. The new files will be compiled only if hostapd support is enabled.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Dec 20, 2024

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@b5f6470 (master) zephyrproject-rtos/hal_nxp#492 zephyrproject-rtos/hal_nxp#492/files
hostap zephyrproject-rtos/hostap@516cf57 (main) zephyrproject-rtos/hostap#72 zephyrproject-rtos/hostap#72/files

DNM label due to: 2 projects with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hostap manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Dec 20, 2024
@nxf58150
Copy link
Contributor Author

This PR is related to issue #77383

@nxf58150 nxf58150 force-pushed the hostapd_split branch 3 times, most recently from 97c0595 to db8d204 Compare December 25, 2024 02:48
@nxf58150
Copy link
Contributor Author

Hi @krish2718 , please help review this PR. As discussed before, I moved the hostapd related source code to new files. Please also help add Jerome as reviewer. Thanks!

@nxf58150 nxf58150 force-pushed the hostapd_split branch 2 times, most recently from 3ca0e3d to e75bcee Compare January 13, 2025 10:18
Created new files and relocate hostapd support code in glue layer to new
files. The new files will be compiled only if hostapd support is enabled.

Signed-off-by: Hui Bai <[email protected]>
Copy link
Contributor

@jerome-pouiller jerome-pouiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely goes in the right direction.

(I would have appreciated smaller commits, but this is not a big deal).

bss->wps_state = WPS_STATE_CONFIGURED;
bss->eap_server = 1;
#ifdef CONFIG_WPS
bss->ap_setup_locked = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR (because this PR only relocate code without modifying it): we could write bss->ap_setup_locked = IS_ENABLED(CONFIG_WPS);.

conf->channel = 1;
conf->acs = conf->channel == 0;
#ifdef CONFIG_ACS
conf->acs_num_scans = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, in a further PR, we could change this line in conf->acs_num_scans = IS_ENABLED(CONFIG_ACS);.

#ifdef CONFIG_WIFI_NM_WPA_SUPPLICANT_CRYPTO_ENTERPRISE
#include "eap_peer/eap.h"
#endif
#include "supp_events.h"
#ifdef CONFIG_WIFI_NM_HOSTAPD_AP
#include "hapd_api.h"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including a header do not increase the size of the code and can simplify the code. Typically, it allow to write:

    if (IS_ENABLED(CONFIG_FOO))
        function_declared_but_not_defined_since_CONFIG_FOO_is_not_set()

So, unless you have a good reason, I would avoid to include header conditionally.

if (!interfaces->dpp) {
return;
}
#endif /* CONFIG_DPP */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be written:

    if (IS_ENABLED(CONFIG_DPP)) {
        os_memset(&dpp_conf, 0, sizeof(dpp_conf));
        [...]
    }

Then, you can also drop the #ifdef CONFIG_DPP around declaration of dpp_conf.
(if you do the change, I suggest to place it in another commit or PR)

return iface;
}

void zephyr_hostapd_init(void *ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to declare ctx as a struct hapd_interfaces *?

{
return &get_default_context()->hostapd;
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would make sense to relocate this function in hapd_main.c and do the same pattern than get_default_context(void):

static struct hapd_interfaces *zephyr_get_default_hapd_context(void)
{
	static struct hapd_interfaces ctx;

	return &ctx;
}

Thus, struct hapd_interfaces won't have to be known in supplicant. It may lead to further simplifications.

@@ -20,6 +21,14 @@
#define MAC_STR_LEN 18 /* for ':' or '-' separated MAC address string */
#define CHAN_NUM_LEN 6 /* for space-separated channel numbers string */

enum wifi_frequency_bands wpas_band_to_zephyr(enum wpa_radio_work_band band);

enum wifi_security_type wpas_key_mgmt_to_zephyr(int key_mgmt, int proto);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum wifi_security_type wpas_key_mgmt_to_zephyr(int key_mgmt, int proto);
enum wifi_security_type wpas_key_mgmt_to_zephyr(bool is_hapd, void *config, int key_mgmt, int proto, int pwe);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also remove the static inline from definition

"auth=MSCHAPV2"},
{WIFI_SECURITY_TYPE_EAP_PEAP_TLS, WIFI_EAP_TYPE_PEAP, WIFI_EAP_TYPE_TLS, "PEAP",
"auth=TLS"},
{WIFI_SECURITY_TYPE_EAP_TLS_SHA256, WIFI_EAP_TYPE_TLS, WIFI_EAP_TYPE_NONE, "TLS", NULL},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TOT, WIFI_SECURITY_TYPE_EAP_TLS_SHA256 has been removed

Copy link
Collaborator

@MaochenWang1 MaochenWang1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please try to build as the codebase updated

@jerome-pouiller jerome-pouiller self-requested a review January 17, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Wi-Fi Wi-Fi DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_nxp manifest-hostap
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants