drivers: wifi: infineon: add .iface_status method #88718

drensber · 2025-04-16T13:46:41Z

The .iface_status method of the wifi_mgmt_ops API needs to be added so that the "wifi status" command on the network shell will work.

jukkar

Minor nits about the coding style, you could see what the rest of the airoc_wifi.c looks like and follow suit.

drivers/wifi/infineon/airoc_wifi.c

drensber · 2025-04-17T13:41:49Z

I changed all of these things according to your preferences. Looking at the Zephyr Coding Guidelines, I'm not sure that any of them are actually established rules, so perhaps that's why the automated checks didn't catch any of them.

drensber · 2025-04-17T14:07:59Z

I think the twister failure is a problem on your side, not a problem with the code that I pushed. Can someone look into this and re-run the tests?

rlubos · 2025-04-17T14:18:28Z

I think the twister failure is a problem on your side, not a problem with the code that I pushed. Can someone look into this and re-run the tests?

Looked like some environmental issue, I've retriggered failed stage, tests are now running.

Copilot

Pull Request Overview

This PR adds an .iface_status method to the Infineon WiFi driver so that the "wifi status" command on the network shell works as expected.

Added the implementation of airoc_iface_status to report interface mode, state, SSID, channel, and other connection parameters.
Updated the wifi_mgmt_ops structure to include the new .iface_status method.
Introduced an additional header include for whd_wlioctl.h required by the new functionality.

Comments suppressed due to low confidence (1)

drivers/wifi/infineon/airoc_wifi.c:774

The variable 'airoc_if' is referenced without a visible declaration in this diff. Verify that it is properly declared and initialized in an accessible scope before use.

if (airoc_if == NULL) {

Copilot · 2025-04-19T07:14:51Z

drivers/wifi/infineon/airoc_wifi.c

+		strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);
+


Using strncpy without explicitly appending a null terminator risks leaving status->ssid unterminated. Consider ensuring the string is null-terminated (and that the destination buffer is large enough) after copying.

Suggested change

strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);

if (status->ssid_len < sizeof(status->ssid)) {

strncpy(status->ssid, bss_info.SSID, status->ssid_len);

status->ssid[status->ssid_len] = '\0'; // Ensure null-termination

} else {

LOG_ERR("SSID length exceeds buffer size");

return -EINVAL; // Return an error if the buffer is too small

}

I think Copilot's advice is invalid in this particular case. The SSID is defined by zephyr to be a character string with a max length of 32 and an accompanying SSID_len field to specify the actual length. If I were to use one character of the ssid field as a null terminator, the maximum length would become 31.

You don't need to NULL terminate SSID buffer in the status, but you are using source length to copy to destination, so, this can overflow, it's always better to use destination length or do explicit NULL checking.

The .iface_status method of the wifi_mgmt_ops API needs to be added so that the "wifi status" command on the network shell will work. Signed-off-by: Dave Rensberger <[email protected]>

drensber · 2025-05-06T19:45:00Z

Do I need to do anything else here, or are we just waiting for someone else to review?

beechwoods-software · 2025-06-03T19:30:55Z

@Kludentwo @rlubos @krish2718 @MaochenWang1 Could someone else please review this? My understanding is that after a certain amount of time (60 days?) it will just be closed to inactivity. I don't think this should be too controversial of a PR. Even if it isn't a perfect implementation, it's better than having "wifi status" or .iface_status() not work at all, and any deficiencies can be addressed after there's a baseline implementation in place.

krish2718

Please remove all whitespace changes or move them to a separate commit for easier review.

krish2718 · 2025-06-03T19:35:45Z

drivers/wifi/infineon/airoc_wifi.c

@@ -25,7 +26,7 @@ LOG_MODULE_REGISTER(infineon_airoc_wifi, CONFIG_WIFI_LOG_LEVEL);
 #endif

 #ifndef AIROC_WIFI_PACKET_POOL_SIZE
-#define AIROC_WIFI_PACKET_POOL_SIZE     (1600)
+#define AIROC_WIFI_PACKET_POOL_SIZE (1600)


nit, whitespace changes (you can do those in a separate commit if you want for easier review)

krish2718 · 2025-06-03T19:35:56Z

drivers/wifi/infineon/airoc_wifi.c

@@ -49,8 +50,8 @@ int airoc_wifi_init_primary(const struct device *dev, whd_interface_t *interface
 			    whd_netif_funcs_t *netif_funcs, whd_buffer_funcs_t *buffer_if);

 /* Allocate network pool */
-NET_BUF_POOL_FIXED_DEFINE(airoc_pool, AIROC_WIFI_PACKET_POOL_COUNT,
-				AIROC_WIFI_PACKET_POOL_SIZE, 0, NULL);
+NET_BUF_POOL_FIXED_DEFINE(airoc_pool, AIROC_WIFI_PACKET_POOL_COUNT, AIROC_WIFI_PACKET_POOL_SIZE, 0,


krish2718 · 2025-06-03T19:36:02Z

drivers/wifi/infineon/airoc_wifi.c

@@ -236,7 +237,7 @@ static whd_result_t airoc_wifi_host_buffer_get(whd_buffer_t *buffer, whd_buffer_
 	*buffer = buf;

 	/* Set buffer size */
-	(void) airoc_wifi_buffer_set_size(*buffer, size);
+	(void)airoc_wifi_buffer_set_size(*buffer, size);


krish2718 · 2025-06-03T19:36:14Z

drivers/wifi/infineon/airoc_wifi.c

@@ -302,7 +303,7 @@ static int airoc_mgmt_send(const struct device *dev, struct net_pkt *pkt)
 	}

 	/* Allocate Network Buffer from pool with Packet Length + Data Header */
-	ret = airoc_wifi_host_buffer_get((whd_buffer_t *) &buf, WHD_NETWORK_TX,
+	ret = airoc_wifi_host_buffer_get((whd_buffer_t *)&buf, WHD_NETWORK_TX,


krish2718 · 2025-06-03T19:38:42Z

drivers/wifi/infineon/airoc_wifi.c

+		strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);
+


You don't need to NULL terminate SSID buffer in the status, but you are using source length to copy to destination, so, this can overflow, it's always better to use destination length or do explicit NULL checking.

krish2718 · 2025-06-03T19:39:36Z

drivers/wifi/infineon/airoc_wifi.c

+
+	/* Unbelievably, this appears to be the only way to determine the phy mode with
+	 *  the whd SDK that we're currently using. Note that the logic below is only valid on
+	 *  devices that are limited to the 2.4Ghz band. Other versions of the SDK and chip


Even for 2.4G only devices, WIFI_6 can still be an option if rate > 54, if your device supports.

krish2718 · 2025-06-03T19:40:22Z

drivers/wifi/infineon/airoc_wifi.c

+
+		whd_wifi_get_channel(airoc_if, (int *)&status->channel);
+
+		status->band = (status->channel <= CH_MAX_2G_CHANNEL) ? WIFI_FREQ_BAND_2_4_GHZ


nit, may not be applicable here, but won't be true once 6GHz support is added :).

github-actions bot added the area: Wi-Fi Wi-Fi label Apr 16, 2025

github-actions bot requested review from jukkar, Kludentwo, krish2718, MaochenWang1 and rlubos April 16, 2025 13:47

github-actions bot assigned jukkar Apr 16, 2025

drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch 2 times, most recently from 246a979 to 40a8960 Compare April 16, 2025 15:15

jukkar reviewed Apr 17, 2025

View reviewed changes

drivers/wifi/infineon/airoc_wifi.c Outdated Show resolved Hide resolved

drivers/wifi/infineon/airoc_wifi.c Outdated Show resolved Hide resolved

drivers/wifi/infineon/airoc_wifi.c Outdated Show resolved Hide resolved

drivers/wifi/infineon/airoc_wifi.c Outdated Show resolved Hide resolved

drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch 2 times, most recently from beff44c to f09fb8d Compare April 17, 2025 13:28

drensber requested a review from jukkar April 17, 2025 15:35

jukkar previously approved these changes Apr 19, 2025

View reviewed changes

kartben requested a review from Copilot April 19, 2025 07:14

Copilot AI reviewed Apr 19, 2025

View reviewed changes

drivers: wifi: infineon: add .iface_status method

298f0a3

The .iface_status method of the wifi_mgmt_ops API needs to be added so that the "wifi status" command on the network shell will work. Signed-off-by: Dave Rensberger <[email protected]>

drensber dismissed jukkar’s stale review via 298f0a3 April 24, 2025 16:06

drensber force-pushed the adding_iface_status_to_infineon_wifi_driver branch from f09fb8d to 298f0a3 Compare April 24, 2025 16:06

krish2718 reviewed Jun 3, 2025

View reviewed changes

-		strncpy(status->ssid, bss_info.SSID, bss_info.SSID_len);
+		if (status->ssid_len < sizeof(status->ssid)) {
+			strncpy(status->ssid, bss_info.SSID, status->ssid_len);
+			status->ssid[status->ssid_len] = '\0'; // Ensure null-termination
+		} else {
+			LOG_ERR("SSID length exceeds buffer size");
+			return -EINVAL; // Return an error if the buffer is too small
+		}


		whd_wifi_get_channel(airoc_if, (int *)&status->channel);

		status->band = (status->channel <= CH_MAX_2G_CHANNEL) ? WIFI_FREQ_BAND_2_4_GHZ

drivers: wifi: infineon: add .iface_status method #88718

Are you sure you want to change the base?

drivers: wifi: infineon: add .iface_status method #88718

Conversation

drensber commented Apr 16, 2025

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drensber commented Apr 17, 2025

Uh oh!

drensber commented Apr 17, 2025

Uh oh!

rlubos commented Apr 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drensber commented May 6, 2025

Uh oh!

beechwoods-software commented Jun 3, 2025

Uh oh!

krish2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!