wifi: API to check if an interface is Wi-Fi

[krish2718]

No description provided.

[rlubos]

I really don't like putting custom fields into the API structs, no matter how convenient it is.

[krish2718]

I really don't like putting custom fields into the API structs, no matter how convenient it is.

Would this be acceptable? I don't think we even need to rename the struct, as native == offload_disable, so, kind of works.

$ git diff
diff --git a/include/zephyr/net/wifi_mgmt.h b/include/zephyr/net/wifi_mgmt.h
index 8740d6b4bd..b083d80e33 100644
--- a/include/zephyr/net/wifi_mgmt.h
+++ b/include/zephyr/net/wifi_mgmt.h
@@ -298,6 +298,12 @@ typedef void (*scan_result_cb_t)(struct net_if *iface, int status,
 typedef void (*raw_scan_result_cb_t)(struct net_if *iface, int status,
                                     struct wifi_raw_scan_result *entry);
 #endif /* CONFIG_WIFI_MGMT_RAW_SCAN_RESULTS */
+
+enum wifi_offload_type {
+       /* Native */
+       WIFI_OFFLOAD_DISABLED = 0,
+       WIFI_OFFLOAD_FULL,
+} __packed;
 struct net_wifi_mgmt_offload {
        /**
         * Mandatory to get in first position.
@@ -310,6 +316,7 @@ struct net_wifi_mgmt_offload {
 #else
        struct offloaded_if_api wifi_iface;
 #endif
+       enum wifi_offload_type wifi_off_type;
 
        /* cb parameter is the cb that should be called for each
         * result by the driver. The wifi mgmt part will take care of

[rlubos]

I really don't like putting custom fields into the API structs, no matter how convenient it is.

Would this be acceptable? I don't think we even need to rename the struct, as native == offload_disable, so, kind of works.

$ git diff
diff --git a/include/zephyr/net/wifi_mgmt.h b/include/zephyr/net/wifi_mgmt.h
index 8740d6b4bd..b083d80e33 100644
--- a/include/zephyr/net/wifi_mgmt.h
+++ b/include/zephyr/net/wifi_mgmt.h
@@ -298,6 +298,12 @@ typedef void (*scan_result_cb_t)(struct net_if *iface, int status,
 typedef void (*raw_scan_result_cb_t)(struct net_if *iface, int status,
                                     struct wifi_raw_scan_result *entry);
 #endif /* CONFIG_WIFI_MGMT_RAW_SCAN_RESULTS */
+
+enum wifi_offload_type {
+       /* Native */
+       WIFI_OFFLOAD_DISABLED = 0,
+       WIFI_OFFLOAD_FULL,
+} __packed;
 struct net_wifi_mgmt_offload {
        /**
         * Mandatory to get in first position.
@@ -310,6 +316,7 @@ struct net_wifi_mgmt_offload {
 #else
        struct offloaded_if_api wifi_iface;
 #endif
+       enum wifi_offload_type wifi_off_type;
 
        /* cb parameter is the cb that should be called for each
         * result by the driver. The wifi mgmt part will take care of

Is this really different? IMO we should rather introduce some sort of get_capabilities() function at the struct net_wifi_mgmt_offload level (but still - net_wifi_mgmt_offload for fully native devices simply sounds wrong). Just like generic L2 API struct - it could have enum net_l2_flags field directly but it provides a getter function instead, same goes for Ethernet L2 and its capabilities.

[krish2718]

I really don't like putting custom fields into the API structs, no matter how convenient it is.

Would this be acceptable? I don't think we even need to rename the struct, as native == offload_disable, so, kind of works.

$ git diff
diff --git a/include/zephyr/net/wifi_mgmt.h b/include/zephyr/net/wifi_mgmt.h
index 8740d6b4bd..b083d80e33 100644
--- a/include/zephyr/net/wifi_mgmt.h
+++ b/include/zephyr/net/wifi_mgmt.h
@@ -298,6 +298,12 @@ typedef void (*scan_result_cb_t)(struct net_if *iface, int status,
 typedef void (*raw_scan_result_cb_t)(struct net_if *iface, int status,
                                     struct wifi_raw_scan_result *entry);
 #endif /* CONFIG_WIFI_MGMT_RAW_SCAN_RESULTS */
+
+enum wifi_offload_type {
+       /* Native */
+       WIFI_OFFLOAD_DISABLED = 0,
+       WIFI_OFFLOAD_FULL,
+} __packed;
 struct net_wifi_mgmt_offload {
        /**
         * Mandatory to get in first position.
@@ -310,6 +316,7 @@ struct net_wifi_mgmt_offload {
 #else
        struct offloaded_if_api wifi_iface;
 #endif
+       enum wifi_offload_type wifi_off_type;
 
        /* cb parameter is the cb that should be called for each
         * result by the driver. The wifi mgmt part will take care of

Is this really different? IMO we should rather introduce some sort of get_capabilities() function at the struct net_wifi_mgmt_offload level (but still - net_wifi_mgmt_offload for fully native devices simply sounds wrong). Just like generic L2 API struct - it could have enum net_l2_flags field directly but it provides a getter function instead, same goes for Ethernet L2 and its capabilities.

So, another op similar to enum ethernet_hw_caps (*get_capabilities)(const struct device *dev); for Wi-Fi?

[rlubos]

So, another op similar to enum ethernet_hw_caps (*get_capabilities)(const struct device *dev); for Wi-Fi?

That's what I had in mind. Not sure though, if this should be covered in this PR? It doesn't seem to be strictly related to the original problem this PR is trying to solve (verify if an interface is a Wi-Fi interface).

[krish2718]

So, another op similar to enum ethernet_hw_caps (*get_capabilities)(const struct device *dev); for Wi-Fi?

That's what I had in mind. Not sure though, if this should be covered in this PR? It doesn't seem to be strictly related to the original problem this PR is trying to solve (verify if an interface is a Wi-Fi interface).

The problem with this is that we need to know if the interface is Wi-Fi before calling get_capabilities (As this is not not exposed to net_if, we need to use net_mgmt to get these capabilities). We need this capability out of Wi-Fi management module, hence my first approach of making it part of L2 (Ethernet and offloded_netdev).

Well, whatever way we introduced WIFI_OFFLOAD support in this PR, it will be extended to WIFI_NATIVE and will be used in subsequent PR for wifi_mgmt native handling.

[krish2718]

To recap: (Had evaluated many options for this simple flag :) )

We need a way for drivers to tell type of Wi-Fi fundamentally (native, offloaded - default).

• net_if - Too far away for driver, but we could extend net_if_api but same concern as this PR (putting flag in API) and also not applicable for all L2s
  • Add a new WIFI_ variants for NET_DEVICE_OFFLOAD_INIT and NET_DEVICE_DT_INST_DEFINE
• L2 - Not possible, as this is driver agnostic
• L2_API -> This PR but a bit ugly putting a flag in the API
• wifi_api -> Not possible, as we don't know if the interface is Wi-Fi capable or not

@rlubos WDYT about option#1?

[rlubos]

To recap: (Had evaluated many options for this simple flag :) )

So let's try to keep it simple.

Let's for a moment put aside wifi_mgmt offloading and focus on net offloading only.

We have 2 types of devices: native (using Ethernet L2) and offloaded (using offloaded_netdev):

• For native devices - I really can't understand why do you insist on putting the information whether the interface is actually a Wi-Fi into the API structure, and not the Ethernet context. I don't see how:

struct ethernet_context *ctx = net_if_l2_data(iface);

is more complicated than:

const struct ethernet_api *api = net_if_get_device(iface)->api;

Or even, if the maintainer has no problem with that, we could simply add a capability whether it's a Wi-Fi device, and use get_capabilities() API.

• For offloaded devices - just extend the API structure, but with a function to obtain the device type, not the actual field.

That should be enough to tell whether any kind of interface is a Wi-Fi interface or not. And with this information, you can be confident that you work with struct net_wifi_mgmt_offload, and put whatever information you need in there (i. e. whether offloaded wifi_mgmt should be used or not).

[krish2718]

To recap: (Had evaluated many options for this simple flag :) )

So let's try to keep it simple.

Let's for a moment put aside wifi_mgmt offloading and focus on net offloading only.

We have 2 types of devices: native (using Ethernet L2) and offloaded (using offloaded_netdev):

• For native devices - I really can't understand why do you insist on putting the information whether the interface is actually a Wi-Fi into the API structure, and not the Ethernet context. I don't see how:

struct ethernet_context *ctx = net_if_l2_data(iface);

is more complicated than:

const struct ethernet_api *api = net_if_get_device(iface)->api;

This is fine with me, ethernet_context is accessible from all places

Or even, if the maintainer has no problem with that, we could simply add a capability whether it's a Wi-Fi device, and use get_capabilities() API.

I guess you are referring to add this to L2 API to get fetch underlying driver caps?

• For offloaded devices - just extend the API structure, but with a function to obtain the device type, not the actual field.

Ok.

That should be enough to tell whether any kind of interface is a Wi-Fi interface or not. And with this information, you can be confident that you work with struct net_wifi_mgmt_offload, and put whatever information you need in there (i. e. whether offloaded wifi_mgmt should be used or not).

Let me implement this, thanks for your inputs.

[krish2718]

• For offloaded devices - just extend the API structure, but with a function to obtain the device type, not the actual field.

Ok.

Do we have devices that offload Networking stack but not Wi-Fi? This won't work then, we again need indication from the driver.

[rlubos]

• For offloaded devices - just extend the API structure, but with a function to obtain the device type, not the actual field.

Ok.

Do we have devices that offload Networking stack but not Wi-Fi? This won't work then, we again need indication from the driver.

Not sure I follow the question. What do you mean by "offload Networking stack but not Wi-Fi?"?

[krish2718]

• For offloaded devices - just extend the API structure, but with a function to obtain the device type, not the actual field.

Ok.

Do we have devices that offload Networking stack but not Wi-Fi? This won't work then, we again need indication from the driver.

Not sure I follow the question. What do you mean by "offload Networking stack but not Wi-Fi?"?

I am referring to a hypothetical case where Wi-Fi stack is part of Zephry (Either wifi_mgmt or driver). Its a case of YAGNI, anyways I guess we can handle this easily but adding another enum.