driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906

ArunmaniAlagarsamy2710 · 2025-02-18T06:03:01Z

Implemented support for Wi-Fi Access Point (AP) mode.
Added support for AP-related Wi-Fi commands.
Basic functionalities are included; further enhancements will be added in future PR.

jerome-pouiller · 2025-02-18T08:41:40Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		return SL_WIFI_BANDWIDTH_80MHz;
+	default:
+		LOG_ERR("Invalid bandwidth");
+		return -EAGAIN;


Probably -EINVAL is better suited.

I am not sure LOG_ERR() is useful (in fact, I don't know what policy we should have for the the logs).

jerome-pouiller · 2025-02-18T08:45:52Z

drivers/wifi/siwx91x/siwx91x_wifi.h

@@ -14,6 +14,8 @@
 #include "sl_si91x_types.h"
 #include "sl_si91x_protocol_types.h"

+#define SIWX91X_INTERFACE_MASK (0x03)


For now, SIWX91X_INTERFACE_MASK is only used in siwx91x_wifi.c. I am not sure it make sense to expose it in the header file.

jerome-pouiller · 2025-02-18T09:01:31Z

drivers/wifi/siwx91x/siwx91x_wifi.c

-	status = sl_wifi_get_mac_address(SL_WIFI_CLIENT_INTERFACE, &sidev->macaddr);
+	interface = sl_wifi_get_default_interface();
+	status = sl_wifi_get_mac_address(FIELD_GET(SIWX91X_INTERFACE_MASK, interface),
+					 &sidev->macaddr);


In what case the original code didn't work?

jerome-pouiller · 2025-02-18T09:04:27Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		}
+	}
+
+	ret = sl_wifi_start_ap(SL_WIFI_AP_2_4GHZ_INTERFACE, &configuration);


For the reader, it difficult to understand SL_WIFI_AP_2_4GHZ_INTERFACE == SL_WIFI_AP_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE. I suggest either:

Replace references to SL_WIFI_AP_INTERFACE by FIELD_GET(SIWX91X_INTERFACE_MASK, SL_WIFI_AP_2_4GHZ_INTERFACE)

Replace reference to SL_WIFI_AP_2_4GHZ_INTERFACE by SL_WIFI_AP_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE

drivers/wifi/siwx91x/siwx91x_wifi.c

jerome-pouiller · 2025-02-18T09:17:30Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		}
+
+		sl_si91x_set_join_configuration(SL_WIFI_AP_INTERFACE,
+						SL_SI91X_JOIN_FEAT_MFP_CAPABLE_REQUIRED);


I think MFP is orthogonal to the authentication scheme. Probably this configuration should done outside of this switch case.

BTW, WPA2-PSK w/o MFP seems legit. Is it a limitation of Wiseconnect?

I also assume configuration.security = SL_WIFI_WPA3 automatically enables MFP? If ye, we could check params->mfp == WIFI_MFP_REQUIRED.

drivers/wifi/siwx91x/siwx91x_wifi.c

jerome-pouiller · 2025-02-18T09:42:22Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	strncpy(status->ssid, wlan_info.ssid, WIFI_SSID_MAX_LEN);
+	status->ssid_len = strlen(status->ssid);
+	memcpy(status->bssid, wlan_info.mac_address, WIFI_MAC_ADDR_LEN);
+	status->mfp = WIFI_MFP_REQUIRED;


How can you be sure?

jerome-pouiller · 2025-02-18T09:45:55Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	memcpy(status->bssid, wlan_info.mac_address, WIFI_MAC_ADDR_LEN);
+	status->mfp = WIFI_MFP_REQUIRED;
+
+	if (FIELD_GET(SL_WIFI_2_4GHZ_INTERFACE, interface)) {


SL_WIFI_2_4GHZ_INTERFACE is not a mask. I would either write:

FIELD_GET(SIWX91X_BAND_MASK, interface) == 1

interface & SL_WIFI_2_4GHZ_INTERFACE

jerome-pouiller · 2025-02-18T09:47:25Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		status->band = WIFI_FREQ_BAND_2_4_GHZ;
+	}
+
+	if (FIELD_GET(SL_WIFI_CLIENT_INTERFACE, interface)) {


SL_WIFI_CLIENT_INTERFACE is not a mask. I suggest FIELD_GET(SIWX91X_INTERFACE_MASK, interface) == SL_WIFI_CLIENT_INTERFACE.

jerome-pouiller · 2025-02-18T09:49:11Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+
+		sl_wifi_get_listen_interval(SL_WIFI_CLIENT_INTERFACE, &listen_interval);
+		status->beacon_interval = listen_interval.listen_interval;
+	} else if (FIELD_GET(SL_WIFI_AP_INTERFACE, interface)) {


Can this condition be true? oper_mode is always SL_SI91X_CLIENT_MODE. So, I think interface never set SL_WIFI_AP_INTERFACE.

We have added the support to change oper_mode at runtime #85919

I believe #85919 should be a part of this current PR.

jerome-pouiller · 2025-02-18T09:50:23Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		sl_wifi_get_listen_interval(SL_WIFI_CLIENT_INTERFACE, &listen_interval);
+		status->beacon_interval = listen_interval.listen_interval;
+	} else if (FIELD_GET(SL_WIFI_AP_INTERFACE, interface)) {
+		sl_wifi_ap_configuration_t conf = { };


ap_cfg or sl_ap_cfg or siwx91x_ap_cfg.

jerome-pouiller

The 3 first commits depend on drivers: wifi: siwx91x: Add run time opermode, so you have place it first.

jerome-pouiller · 2025-02-19T09:45:23Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+						  SL_SI91X_TCP_IP_FEAT_BYPASS),
+			.custom_feature_bit_map = SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID,
+			.feature_bit_map = SL_SI91X_FEAT_SECURITY_OPEN,
+			.coex_mode = SL_SI91X_WLAN_ONLY_MODE,


How do you deal with BLE?

I feel this function is in fact shared with Bluetooth and event with Flash driver. Probably it should be located in soc/silabs/silabs_siwx91x/siwg917/nwp_init.c.

jerome-pouiller · 2025-02-24T14:33:58Z

soc/silabs/silabs_siwx91x/siwg917/nwp.h

+
+#include "sl_wifi.h"
+
+static inline int get_wifi_device_configuration(sl_wifi_device_configuration_t *net_dev_cfg)


Why this function is defined in the header file?

BTW, prefix the function name with the right prefix (siwg917_nwp_ I think).

jerome-pouiller · 2025-02-24T14:37:16Z

drivers/wifi/siwx91x/siwx91x_wifi.c

 NET_BUF_POOL_FIXED_DEFINE(siwx91x_tx_pool, 1, _NET_ETH_MAX_FRAME_SIZE, 0, NULL);

+static inline int siwx91x_get_mode(uint8_t z_mode, sl_wifi_interface_t iface, uint16_t *sl_opermode)


Why inline?

jerome-pouiller · 2025-02-24T14:39:44Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+{
+	sl_status_t status;
+
+	status = sl_wifi_deinit();


What is the impact of this on the other services: bluetooth and flash?

If Bluetooth is enabled, it will also be deinitialized along with Wi-Fi. I did not find any logic in sl_si91x_host_power_cycle affecting the flash, so I believe there is no impact on it

I assume any Flash access will also abort because of the reset of a DMA somewhere.

I believe the user do not expect the bluetooth will be impacted. I assume it is very difficult (impossible?) remove this limitation?

At least, we have at least to add a FIXME in the code and a LOG_WARN().

I think we could detect Bluetooth or Flash is in use (typically with an atomic counter, but maybe WiseConnect already provide an API for that). Then we could return an error if Bluetooth/Flash is in use.

Anyway, I believe it make more sense to relocate this function in the "shared" area of the 917 (nwp_init.c). I believe it will be easier if it take the new oper_mode in parameter rather than the whole sl_wifi_device_configuration_t.

jerome-pouiller · 2025-02-24T14:49:33Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	case WIFI_STA_MODE:
+		if (FIELD_GET(SIWX91X_INTERFACE_MASK, iface) == SL_WIFI_CLIENT_INTERFACE) {
+			return -EALREADY;
+		}


I feel this function is bit misleading. Reader does not expect get_mode() does such test. Probably you want one function that return SL_SI91X_CLIENT_MODE / SL_SI91X_ACCESS_POINT_MODE from z_mode and another that return SL_SI91X_CLIENT_MODE / SL_SI91X_ACCESS_POINT_MODE from the sl_wifi_interface_t. Then you can check if nwp_reboot() is required or not.

soc/silabs/silabs_siwx91x/siwg917/nwp_init.c

jerome-pouiller · 2025-02-25T13:27:41Z

soc/silabs/silabs_siwx91x/siwg917/nwp_init.c

 #include "sl_wifi_callback_framework.h"
 #ifdef CONFIG_BT_SILABS_SIWX91X
 #include "rsi_ble_common_config.h"
 #endif

-static int siwg917_nwp_init(void)
+int siwg917_nwp_get_configuration(sl_wifi_device_configuration_t *net_dev_cfg)
 {
 	sl_wifi_device_configuration_t network_config = {


Can you find more consistent names, rather than net_dev_cfg and network_config (and cfg).

jerome-pouiller · 2025-02-25T16:19:09Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+{
+	sl_status_t status;
+
+	status = sl_wifi_deinit();


I assume any Flash access will also abort because of the reset of a DMA somewhere.

I believe the user do not expect the bluetooth will be impacted. I assume it is very difficult (impossible?) remove this limitation?

At least, we have at least to add a FIXME in the code and a LOG_WARN().

I think we could detect Bluetooth or Flash is in use (typically with an atomic counter, but maybe WiseConnect already provide an API for that). Then we could return an error if Bluetooth/Flash is in use.

Anyway, I believe it make more sense to relocate this function in the "shared" area of the 917 (nwp_init.c). I believe it will be easier if it take the new oper_mode in parameter rather than the whole sl_wifi_device_configuration_t.

jerome-pouiller · 2025-02-25T16:28:00Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	}
+
+	if (siwx91x_get_mode(mode->mode) < 0) {
+		return -EINVAL;


If siwx91x_get_mode() < 0, it means that we don't have to reboot, so we should return 0, no?

BTW, I believe the following code would be more explicit for the reader:

int current_mode = siwx91x_get_iface_mode(interface); int requested_mode = siwx91x_convert_mode(mode->mode); if (current_mode == requested_mode) { return 0; } siwx91x_nwp_reboot(requested_mode);

jerome-pouiller

Code has now a good shape. This is probably the last iteration.

jerome-pouiller · 2025-03-07T09:09:15Z

soc/silabs/silabs_siwx91x/siwg917/nwp.c

+	/* FIXME: Calling sl_wifi_deinit() impacts Bluetooth if coexistence is enabled */
+	if (IS_ENABLED(CONFIG_BT_SILABS_SIWX91X) && oper_mode == WIFI_AP_MODE) {
+		LOG_WRN("Bluetooth is not supported in AP mode");
+	}


I believe it make more sense to make this test in siwg91x_get_nwp_config() / if (wifi_oper_mode == SL_SI91X_ACCESS_POINT_MODE).

jerome-pouiller · 2025-03-07T09:51:31Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		ret = siwx91x_nwp_mode_switch(mode->mode);
+		if (ret) {
+			return ret;
+		}


Once siwx91x_nwp_mode_switch() takes a Zephyr mode as argument, the idea was to simplify everything:

static int siwx91x_mode(const struct device *dev, struct wifi_mode_info *mode) { sl_wifi_interface_t interface = sl_wifi_get_default_interface(); struct siwx91x_dev *sidev = dev->data; int cur_mode; int ret = 0; __ASSERT(mode, "mode cannot be NULL"); cur_mode = siwx91x_sl_to_z_mode(FIELD_GET(SIWX91X_INTERFACE_MASK, interface)); if (cur_mode < 0) { return -EIO; } if (mode->oper == WIFI_MGMT_GET) { mode->mode = cur_mode; } else if (mode->oper == WIFI_MGMT_SET) { if (cur_mode != mode->mode) { ret = siwx91x_nwp_mode_switch(mode->mode); if (ret < 0) { return ret; } } sidev->state = WIFI_STATE_INACTIVE; } return 0; }

Then, siwx91x_nwp_mode_switch() does not have to check the current status of the interface.

ArunmaniAlagarsamy2710 · 2025-03-07T11:31:05Z

@jhedberg Could you please review it?

drivers/wifi/siwx91x/siwx91x_wifi.c

jhedberg · 2025-03-10T07:48:25Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	if (siwx91x_security(params->security) < 0) {
+		return -EINVAL;
+	}
+
+	siwx91x_ap_cfg.security = siwx91x_security(params->security);


Another minor thing (sorry, I should have pointed this out in the last review): it seems a bit inefficient to be calling siwx91x_security() twice on the exact same input parameter. Why not just store the return value in a local int sec variable?

Thank you for the comment, we followed this approach to maintain consistency with the style used in the silabs DMA driver.
https://github.com/zephyrproject-rtos/zephyr/blob/658f9357b87ab3e02351a57183743ec70f92cdd7/drivers/dma/dma_silabs_siwx91x.c#L143

Consistency is important, but that doesn't mean that you should not question existing code. The code you pointed to looks also inefficient. Any comment from @smalae? (the author)

jerome-pouiller · 2025-03-13T09:51:44Z

@ArunmaniAlagarsamy2710 Can you rebase your branch?

This commit enables the device to change its operating mode at runtime. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

This commit introduces Access Point (AP) mode support It enables the following AP-related commands: - ap enable: Enable the AP mode. - ap disable: Disable the AP mode. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

This commit adds support the following wifi commands: - wifi ap disconnect: Disconnect a specific station from the AP. - wifi ap stations: List connected devices. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

This commit adds support for Wi-Fi status and statistics functionalities: - wifi status : Displays the current mode status. - wifi statistics : Provides details on the transmitted and received packet counts. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

jerome-pouiller · 2025-03-20T09:41:39Z

@jukkar, do you have any other comments about this PR?

zephyrbot added the area: Wi-Fi Wi-Fi label Feb 18, 2025

zephyrbot requested review from jukkar, Kludentwo, krish2718, MaochenWang1 and rlubos February 18, 2025 06:03

zephyrbot assigned jukkar Feb 18, 2025

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 5add5e0 to d02f00a Compare February 18, 2025 07:20

jerome-pouiller self-requested a review February 18, 2025 08:40

jerome-pouiller reviewed Feb 18, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from d02f00a to d610343 Compare February 18, 2025 08:54

jerome-pouiller reviewed Feb 18, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from d610343 to 768343b Compare February 18, 2025 15:54

ArunmaniAlagarsamy2710 mentioned this pull request Feb 18, 2025

drivers: wifi: siwx91x: Add run time opermode #85919

Closed

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 768343b to d5c66f1 Compare February 18, 2025 15:56

ArunmaniAlagarsamy2710 mentioned this pull request Feb 18, 2025

Add AP mode support and error propagation improvements SiliconLabsSoftware/zephyr-silabs#87

Closed

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch 2 times, most recently from acff2d2 to d5d3c31 Compare February 19, 2025 05:03

jerome-pouiller reviewed Feb 19, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from d5d3c31 to e55ccfa Compare February 21, 2025 16:50

zephyrbot added the platform: Silabs Silicon Labs label Feb 21, 2025

zephyrbot requested review from asmellby, jhedberg and Martinhoff-maker February 21, 2025 16:51

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch 2 times, most recently from cdcfe3b to 6d05903 Compare February 22, 2025 02:35

jerome-pouiller reviewed Feb 24, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 6d05903 to 7e5681d Compare February 25, 2025 06:52

jerome-pouiller reviewed Feb 25, 2025

View reviewed changes

jerome-pouiller reviewed Mar 7, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch 2 times, most recently from 61862ae to 71a08a7 Compare March 7, 2025 10:52

jerome-pouiller previously approved these changes Mar 7, 2025

View reviewed changes

jhedberg reviewed Mar 9, 2025

View reviewed changes

drivers/wifi/siwx91x/siwx91x_wifi.c Outdated Show resolved Hide resolved

ArunmaniAlagarsamy2710 dismissed jerome-pouiller’s stale review via 730bbc5 March 10, 2025 07:39

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 71a08a7 to 730bbc5 Compare March 10, 2025 07:39

jhedberg reviewed Mar 10, 2025

View reviewed changes

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 730bbc5 to 26ebdd8 Compare March 10, 2025 08:48

ArunmaniAlagarsamy2710 changed the title ~~Add Access Point (AP) mode support to siwx91x wifi driver~~ driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver Mar 10, 2025

jerome-pouiller previously approved these changes Mar 10, 2025

View reviewed changes

jhedberg previously approved these changes Mar 10, 2025

View reviewed changes

jukkar previously approved these changes Mar 13, 2025

View reviewed changes

ArunmaniAlagarsamy2710 added 4 commits March 13, 2025 16:02

drivers: wifi: siwx91x: Add run time opermode

9ecb0bf

This commit enables the device to change its operating mode at runtime. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

drivers: wifi: siwx91x: Add Access Point (AP) mode support

d35a6d3

This commit introduces Access Point (AP) mode support It enables the following AP-related commands: - ap enable: Enable the AP mode. - ap disable: Disable the AP mode. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

drivers: wifi: siwx91x: Add AP station disconnect functionality

0266aa7

This commit adds support the following wifi commands: - wifi ap disconnect: Disconnect a specific station from the AP. - wifi ap stations: List connected devices. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>

ArunmaniAlagarsamy2710 dismissed stale reviews from jukkar, jhedberg, and jerome-pouiller via ad9bec9 March 13, 2025 10:38

ArunmaniAlagarsamy2710 force-pushed the driver/access_point branch from 26ebdd8 to ad9bec9 Compare March 13, 2025 10:38

jerome-pouiller approved these changes Mar 13, 2025

View reviewed changes

jhedberg approved these changes Mar 13, 2025

View reviewed changes

jerome-pouiller self-requested a review March 20, 2025 10:07

jerome-pouiller approved these changes Mar 20, 2025

View reviewed changes

jukkar approved these changes Mar 20, 2025

View reviewed changes

carlescufi merged commit 601a9fa into zephyrproject-rtos:main Mar 20, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906

driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906

ArunmaniAlagarsamy2710 commented Feb 18, 2025

jerome-pouiller Feb 18, 2025 •

edited

Loading

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

ArunmaniAlagarsamy2710 Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller Feb 18, 2025

jerome-pouiller left a comment

jerome-pouiller Feb 19, 2025

jerome-pouiller Feb 24, 2025

jerome-pouiller Feb 24, 2025

jerome-pouiller Feb 24, 2025

ArunmaniAlagarsamy2710 Feb 25, 2025

jerome-pouiller Feb 25, 2025

jerome-pouiller Feb 24, 2025

jerome-pouiller Feb 25, 2025

jerome-pouiller Feb 25, 2025

jerome-pouiller Feb 25, 2025

jerome-pouiller left a comment

jerome-pouiller Mar 7, 2025

jerome-pouiller Mar 7, 2025

ArunmaniAlagarsamy2710 commented Mar 7, 2025

jhedberg Mar 10, 2025

ArunmaniAlagarsamy2710 Mar 10, 2025

jhedberg Mar 10, 2025

jerome-pouiller commented Mar 13, 2025

jerome-pouiller commented Mar 20, 2025


		#include "sl_wifi.h"

		static inline int get_wifi_device_configuration(sl_wifi_device_configuration_t *net_dev_cfg)

		NET_BUF_POOL_FIXED_DEFINE(siwx91x_tx_pool, 1, _NET_ETH_MAX_FRAME_SIZE, 0, NULL);

		static inline int siwx91x_get_mode(uint8_t z_mode, sl_wifi_interface_t iface, uint16_t *sl_opermode)

driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906

driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906

Conversation

ArunmaniAlagarsamy2710 commented Feb 18, 2025

jerome-pouiller Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-pouiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-pouiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArunmaniAlagarsamy2710 commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-pouiller commented Mar 13, 2025

jerome-pouiller commented Mar 20, 2025

jerome-pouiller Feb 18, 2025 •

edited

Loading