[PATCH 1/6] netdev: use NL80211_STA_INFO_SIGNAL rather than average

[PATCH 1/6] netdev: use NL80211_STA_INFO_SIGNAL rather than average

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

Since GET_STATION (and in turn GetDiagnostics) gets the most
current station info this attribute serves as a better indication
of the current signal strength. In addition full mac cards don't
appear to always have the average attribute.
---
 src/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/netdev.c b/src/netdev.c
index 1f2aa51c..e0779de5 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -443,7 +443,7 @@ static bool netdev_parse_sta_info(struct l_genl_attr *attr,
 
 	while (l_genl_attr_next(attr, &type, &len, &data)) {
 		switch (type) {
-		case NL80211_STA_INFO_SIGNAL_AVG:
+		case NL80211_STA_INFO_SIGNAL:
 			if (len != 1)
 				return false;
 
-- 
2.26.2

[PATCH 2/6] scan: allow 'faked' scan_bss results

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]

When a full mac roam occurs IWD has no idea about how the
roam occurred, including any scan results associated with
it. IWD just gets a roam event (basically CMD_CONNECT) and
will need to 'fake' a scan result to provide to station.

To allow this scan_bss_rank_compare was exposed, and
scan_bss_addr_eq was also modified to work with queue's.
This will allow station to insert a 'faked' scan_bss result
into its list as if it did a prior scan. A new frame type
was added, SCAN_BSS_NONE, which will indicate this scan
result was obtained via other means (aka faked). This may
not be strictly required but it makes things clearer
for cleanup, and avoids frame type specific routines.
---
 src/p2p.c  | 3 +++
 src/scan.c | 7 ++++++-
 src/scan.h | 8 ++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 22b28f1c..578b356a 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -2716,6 +2716,9 @@ static bool p2p_peer_get_info(struct p2p_peer *peer,
 
 		*capability = &peer->bss->p2p_beacon_info->capability;
 		break;
+	case SCAN_BSS_NONE:
+		l_error("No source frame type set on BSS");
+		break;
 	}
 
 	return false;
diff --git a/src/scan.c b/src/scan.c
index 82e6a2ae..f42e1e49 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -1147,6 +1147,9 @@ static bool scan_parse_bss_information_elements(struct scan_bss *bss,
 		bss->p2p_probe_resp_info = NULL;
 		break;
 	}
+	default:
+		l_error("No source frame type set on BSS");
+		break;
 	}
 
 	bss->wfd = ie_tlv_extract_wfd_payload(data, len, &bss->wfd_size);
@@ -1315,7 +1318,7 @@ static struct scan_bss *scan_parse_result(struct l_genl_msg *msg,
 	return bss;
 }
 
-static void scan_bss_compute_rank(struct scan_bss *bss)
+void scan_bss_compute_rank(struct scan_bss *bss)
 {
 	static const double RANK_RSNE_FACTOR = 1.2;
 	static const double RANK_WPA_FACTOR = 1.0;
@@ -1450,6 +1453,8 @@ void scan_bss_free(struct scan_bss *bss)
 		p2p_clear_beacon(bss->p2p_beacon_info);
 		l_free(bss->p2p_beacon_info);
 		break;
+	default:
+		break;
 	}
 
 	l_free(bss);
diff --git a/src/scan.h b/src/scan.h
index 355b4b5a..dc7f4688 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -43,6 +43,7 @@ enum scan_bss_frame_type {
 	SCAN_BSS_PROBE_RESP,
 	SCAN_BSS_PROBE_REQ,
 	SCAN_BSS_BEACON,
+	SCAN_BSS_NONE,
 };
 
 struct scan_bss {
@@ -115,9 +116,11 @@ static inline int scan_bss_addr_cmp(const struct scan_bss *a1,
 	return memcmp(a1->addr, a2->addr, sizeof(a1->addr));
 }
 
-static inline bool scan_bss_addr_eq(const struct scan_bss *a1,
-					const struct scan_bss *a2)
+static inline bool scan_bss_addr_eq(const void *a, const void *b)
 {
+	const struct scan_bss *a1 = a;
+	const struct scan_bss *a2 = b;
+
 	return !memcmp(a1->addr, a2->addr, sizeof(a1->addr));
 }
 
@@ -153,6 +156,7 @@ uint64_t scan_get_triggered_time(uint64_t wdev_id, uint32_t id);
 
 void scan_bss_free(struct scan_bss *bss);
 int scan_bss_rank_compare(const void *a, const void *b, void *user);
+void scan_bss_compute_rank(struct scan_bss *bss);
 
 int scan_bss_get_rsn_info(const struct scan_bss *bss, struct ie_rsn_info *info);
 
-- 
2.26.2

Re: [PATCH 2/6] scan: allow 'faked' scan_bss results

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2657 bytes --]

Hi James,

On 3/10/21 2:27 PM, James Prestwood wrote:
> When a full mac roam occurs IWD has no idea about how the
> roam occurred, including any scan results associated with
> it. IWD just gets a roam event (basically CMD_CONNECT) and
> will need to 'fake' a scan result to provide to station.
> 
> To allow this scan_bss_rank_compare was exposed, and
> scan_bss_addr_eq was also modified to work with queue's.
> This will allow station to insert a 'faked' scan_bss result
> into its list as if it did a prior scan. A new frame type
> was added, SCAN_BSS_NONE, which will indicate this scan
> result was obtained via other means (aka faked). This may
> not be strictly required but it makes things clearer
> for cleanup, and avoids frame type specific routines.
> ---
>   src/p2p.c  | 3 +++
>   src/scan.c | 7 ++++++-
>   src/scan.h | 8 ++++++--
>   3 files changed, 15 insertions(+), 3 deletions(-)
> 

<snip>

> diff --git a/src/scan.h b/src/scan.h
> index 355b4b5a..dc7f4688 100644
> --- a/src/scan.h
> +++ b/src/scan.h
> @@ -43,6 +43,7 @@ enum scan_bss_frame_type {
>   	SCAN_BSS_PROBE_RESP,
>   	SCAN_BSS_PROBE_REQ,
>   	SCAN_BSS_BEACON,
> +	SCAN_BSS_NONE,

Ah, I'm not a huge fan of adding enum entries that don't represent anything. 
Given that this is a huge special case that only station cares about, can we 
just fake it by setting SCAN_BSS_PROBE_RESP?

>   };
>   
>   struct scan_bss {
> @@ -115,9 +116,11 @@ static inline int scan_bss_addr_cmp(const struct scan_bss *a1,
>   	return memcmp(a1->addr, a2->addr, sizeof(a1->addr));
>   }
>   
> -static inline bool scan_bss_addr_eq(const struct scan_bss *a1,
> -					const struct scan_bss *a2)
> +static inline bool scan_bss_addr_eq(const void *a, const void *b)
>   {
> +	const struct scan_bss *a1 = a;
> +	const struct scan_bss *a2 = b;
> +

Not really against this, but wouldn't you use bss_match_bssid inside station.c?

>   	return !memcmp(a1->addr, a2->addr, sizeof(a1->addr));
>   }
>   
> @@ -153,6 +156,7 @@ uint64_t scan_get_triggered_time(uint64_t wdev_id, uint32_t id);
>   
>   void scan_bss_free(struct scan_bss *bss);
>   int scan_bss_rank_compare(const void *a, const void *b, void *user);
> +void scan_bss_compute_rank(struct scan_bss *bss);

Okay, but don't you also want to parse the IEs obtained from GET_STATION / 
NEW_STATION?  i.e. scan_parse_bss_information_elements() or similar?

So maybe a better approach would be another constructor that takes mac, rssi, 
ies and frequency?

>   
>   int scan_bss_get_rsn_info(const struct scan_bss *bss, struct ie_rsn_info *info);
>   
> 

Regards,
-Denis

[PATCH 3/6] nl80211util: add WIPHY_FREQ to parse_attrs support

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

---
 src/nl80211util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/nl80211util.c b/src/nl80211util.c
index f226f155..7a752a66 100644
--- a/src/nl80211util.c
+++ b/src/nl80211util.c
@@ -137,6 +137,8 @@ static attr_handler handler_for_type(enum nl80211_attrs type)
 		return extract_mac;
 	case NL80211_ATTR_ACK:
 		return extract_flag;
+	case NL80211_ATTR_WIPHY_FREQ:
+		return extract_uint32;
 	default:
 		break;
 	}
-- 
2.26.2

Re: [PATCH 3/6] nl80211util: add WIPHY_FREQ to parse_attrs support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 175 bytes --]

Hi James,

On 3/10/21 2:27 PM, James Prestwood wrote:
> ---
>   src/nl80211util.c | 2 ++
>   1 file changed, 2 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 4/6] netdev: station: support full mac roaming

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 10927 bytes --]

Roaming on a full mac card is quite different than soft mac
and needs to be specially handled. The process starts with
the CMD_ROAM event, which tells us the driver is already
roamed and associated with a new AP. After this it expects
the 4-way handshake to be initiated. This in itself is quite
simple, the complexity comes with how this is piped into IWD.

There are a few issues:

1. station->connected_bss needs to be set to the new BSS
2. IWD has no past scan results to populate connected_bss
3. Full mac roaming has to be driven from netdev, and this
   is normally driven by station.

There is no way to *fully* populate a scan_bss with the new
BSS the driver roamed to, but we can get close enough to
at least rank it and provide basic info. Using GET_INTERFACE
and GET_STATION the frequency and signal strength can be
obtained respectively. This allows for ranking the new BSS
in stations bss_list. All the IE's are parsed using existing
code in netdev_connect_event.

For consistency station must also transition to a roaming state.
Since this roam is all handled by netdev two new events were
added, NETDEV_EVENT_ROAMING and NETDEV_EVENT_ROAMED. Both allow
station to transition between roaming/connected states, and ROAMED
provides station with the new (faked) scan_bss to replace
connected_bss.
---
 src/netdev.c  | 169 +++++++++++++++++++++++++++++++++++++++++++++++++-
 src/netdev.h  |   2 +
 src/station.c |  23 +++++++
 3 files changed, 191 insertions(+), 3 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index e0779de5..b87e3f2c 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -111,6 +111,7 @@ struct netdev {
 	uint32_t rekey_offload_cmd_id;
 	uint32_t qos_map_cmd_id;
 	uint32_t mac_change_cmd_id;
+	uint32_t get_iface_cmd_id;
 	enum netdev_result result;
 	uint16_t last_code; /* reason or status, depending on result */
 	struct l_timeout *neighbor_report_timeout;
@@ -128,6 +129,8 @@ struct netdev {
 	uint32_t rssi_poll_cmd_id;
 	uint8_t set_mac_once[6];
 
+	struct scan_bss *fmac_roam_bss;
+
 	uint32_t set_powered_cmd_id;
 	netdev_command_cb_t set_powered_cb;
 	void *set_powered_user_data;
@@ -652,6 +655,11 @@ static void netdev_connect_free(struct netdev *netdev)
 		netdev->group_handshake_timeout = NULL;
 	}
 
+	if (netdev->fmac_roam_bss) {
+		scan_bss_free(netdev->fmac_roam_bss);
+		netdev->fmac_roam_bss = NULL;
+	}
+
 	netdev->associated = false;
 	netdev->operational = false;
 	netdev->connected = false;
@@ -770,6 +778,11 @@ static void netdev_free(void *data)
 		netdev->get_station_cmd_id = 0;
 	}
 
+	if (netdev->get_iface_cmd_id) {
+		l_genl_family_cancel(nl80211, netdev->get_iface_cmd_id);
+		netdev->get_iface_cmd_id = 0;
+	}
+
 	if (netdev->events_ready)
 		WATCHLIST_NOTIFY(&netdev_watches, netdev_watch_func_t,
 					netdev, NETDEV_WATCH_EVENT_DEL);
@@ -1179,6 +1192,25 @@ static void netdev_operstate_cb(int error, uint16_t type,
 							strerror(-error));
 }
 
+static void try_roam_complete(struct netdev *netdev)
+{
+	/*
+	 * No guarantee that GET_IFACE/GET_STATION/eapol will complete in the
+	 * same order they were started.
+	 */
+	if (netdev->get_iface_cmd_id ||
+			netdev->fmac_roam_bss->signal_strength == 0 ||
+			!netdev->operational)
+		return;
+
+	if (netdev->event_filter)
+		netdev->event_filter(netdev, NETDEV_EVENT_ROAMED,
+					netdev->fmac_roam_bss,
+					netdev->user_data);
+	/* station will take ownership of fmac_roam_bss */
+	netdev->fmac_roam_bss = NULL;
+}
+
 static void netdev_connect_ok(struct netdev *netdev)
 {
 	l_rtnl_set_linkmode_and_operstate(rtnl, netdev->index,
@@ -1188,6 +1220,11 @@ static void netdev_connect_ok(struct netdev *netdev)
 
 	netdev->operational = true;
 
+	if (netdev->fmac_roam_bss) {
+		try_roam_complete(netdev);
+		return;
+	}
+
 	if (netdev->connect_cb) {
 		netdev->connect_cb(netdev, NETDEV_RESULT_OK, NULL,
 					netdev->user_data);
@@ -1807,6 +1844,7 @@ static void netdev_connect_event(struct l_genl_msg *msg, struct netdev *netdev)
 	struct ie_tlv_iter iter;
 	const uint8_t *resp_ies = NULL;
 	size_t resp_ies_len;
+	uint8_t cmd = l_genl_msg_get_command(msg);
 
 	l_debug("");
 
@@ -1860,9 +1898,16 @@ static void netdev_connect_event(struct l_genl_msg *msg, struct netdev *netdev)
 			goto error;
 	}
 
-	/* AP Rejected the authenticate / associate */
-	if (!status_code || *status_code != 0)
-		goto error;
+	/*
+	 * A CMD_ROAM event will not have a status code, since it indicates
+	 * the hardware has already roamed. A failed roam on fullmac should
+	 * result in an explicit disconnect event.
+	 */
+	if (cmd == NL80211_CMD_CONNECT) {
+		/* AP Rejected the authenticate / associate */
+		if (!status_code || *status_code != 0)
+			goto error;
+	}
 
 	if (!ies)
 		goto process_resp_ies;
@@ -1948,6 +1993,13 @@ process_resp_ies:
 	}
 
 	if (netdev->sm) {
+		/*
+		 * Let station know about the roam so a state change can occur.
+		 */
+		if (cmd == NL80211_CMD_ROAM && netdev->event_filter)
+			netdev->event_filter(netdev, NETDEV_EVENT_ROAMING,
+						NULL, netdev->user_data);
+
 		/*
 		 * Start processing EAPoL frames now that the state machine
 		 * has all the input data even in FT mode.
@@ -3905,6 +3957,113 @@ static void netdev_scan_notify(struct l_genl_msg *msg, void *user_data)
 	}
 }
 
+static void netdev_get_interface_cb(struct l_genl_msg *msg, void *user_data)
+{
+	struct netdev *netdev = user_data;
+
+	netdev->get_iface_cmd_id = 0;
+
+	if (nl80211_parse_attrs(msg, NL80211_ATTR_WIPHY_FREQ,
+				&netdev->fmac_roam_bss->frequency,
+				NL80211_ATTR_UNSPEC) < 0)
+		return;
+
+	netdev->prev_frequency = netdev->frequency;
+	netdev->frequency = netdev->fmac_roam_bss->frequency;
+
+	try_roam_complete(netdev);
+}
+
+static void netdev_roamed_station_cb(const struct diagnostic_station_info *info,
+					void *user_data)
+{
+	struct netdev *netdev = user_data;
+
+	netdev->fmac_roam_bss->signal_strength = info->cur_rssi * 1000;
+
+	try_roam_complete(netdev);
+}
+
+/*
+ * CMD_ROAM indicates that the driver has already roamed/associated with a new
+ * AP. This event is nearly identical to the CMD_CONNECT event which is why
+ * netdev_connect_event will handle all the parsing of IE's just as it does
+ * normally.
+ *
+ * One piece that is not included is the roamed BSS channel/frequency. Since the
+ * driver roamed by itself we do not have a recent scan to obtain this from so
+ * GET_INTERFACE is used so we can build as complete of a scan_bss object as
+ * possible to provide to station. The scan_bss object will not have much of
+ * the normal information as it should, but this is the best we can do on a
+ * fullmac roam.
+ *
+ * The current handshake/netdev_handshake objects are reused after being
+ * reset to allow eapol to happen again without it thinking this is a re-key.
+ */
+static bool netdev_roam_event(struct l_genl_msg *msg, struct netdev *netdev)
+{
+	struct netdev_handshake_state *nhs =
+			l_container_of(netdev->handshake,
+					struct netdev_handshake_state,
+					super);
+	const uint8_t *mac;
+
+	l_debug("");
+
+	netdev->operational = false;
+
+	if (nl80211_parse_attrs(msg, NL80211_ATTR_MAC, &mac,
+					NL80211_ATTR_UNSPEC) < 0) {
+		l_error("Failed to parse ATTR_MAC from CMD_ROAM");
+		goto failed;
+	}
+
+	/* create this now so its known that netdev is roaming */
+	netdev->fmac_roam_bss = l_new(struct scan_bss, 1);
+	netdev->fmac_roam_bss->source_frame = SCAN_BSS_NONE;
+	memcpy(netdev->fmac_roam_bss->addr, mac, 6);
+
+	/* Reset handshake state */
+	nhs->complete = false;
+	nhs->ptk_installed = false;
+	nhs->gtk_installed = true;
+	nhs->igtk_installed = true;
+	handshake_state_set_authenticator_address(netdev->handshake, mac);
+	netdev->handshake->ptk_complete = false;
+
+	msg = l_genl_msg_new(NL80211_CMD_GET_INTERFACE);
+
+	l_genl_msg_append_attr(msg, NL80211_ATTR_IFINDEX, 4, &netdev->index);
+
+	netdev->get_iface_cmd_id = l_genl_family_send(nl80211, msg,
+							netdev_get_interface_cb,
+							netdev, NULL);
+	if (!netdev->get_iface_cmd_id) {
+		l_error("Failed to GET_INTERFACE after roam");
+		l_genl_msg_unref(msg);
+		goto failed;
+	}
+
+	/*
+	 * We can safely do this now since station is not in a connected state,
+	 * meaning the diagnostics interface is not up
+	 */
+	if (netdev_get_station(netdev, mac, netdev_roamed_station_cb,
+				netdev, NULL) < 0) {
+		l_error("failed to GET_STATION after roam");
+		goto failed;
+	}
+
+	return true;
+
+failed:
+	l_error("Failed to roam");
+	netdev_connect_failed(netdev, NETDEV_RESULT_ABORTED,
+					MMPDU_REASON_CODE_UNSPECIFIED);
+
+	return false;
+}
+
 static void netdev_mlme_notify(struct l_genl_msg *msg, void *user_data)
 {
 	struct netdev *netdev = NULL;
@@ -3946,6 +4105,10 @@ static void netdev_mlme_notify(struct l_genl_msg *msg, void *user_data)
 	case NL80211_CMD_ASSOCIATE:
 		netdev_associate_event(msg, netdev);
 		break;
+	case NL80211_CMD_ROAM:
+		if (!netdev_roam_event(msg, netdev))
+			return;
+		/* fall through */
 	case NL80211_CMD_CONNECT:
 		netdev_connect_event(msg, netdev);
 		break;
diff --git a/src/netdev.h b/src/netdev.h
index d5adcf09..c2c43290 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -41,6 +41,8 @@ enum netdev_result {
 enum netdev_event {
 	NETDEV_EVENT_AUTHENTICATING,
 	NETDEV_EVENT_ASSOCIATING,
+	NETDEV_EVENT_ROAMING,
+	NETDEV_EVENT_ROAMED,
 	NETDEV_EVENT_DISCONNECT_BY_AP,
 	NETDEV_EVENT_DISCONNECT_BY_SME,
 	NETDEV_EVENT_RSSI_THRESHOLD_LOW,
diff --git a/src/station.c b/src/station.c
index 6496be10..f9fa0d71 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2291,6 +2291,23 @@ static void station_ok_rssi(struct station *station)
 	station->roam_min_time.tv_sec = 0;
 }
 
+static void station_event_roamed(struct station *station, struct scan_bss *new)
+{
+	struct scan_bss *stale;
+
+	/* Remove new BSS if it exists in past scan results */
+	stale = l_queue_remove_if(station->bss_list, scan_bss_addr_eq, new);
+	if (stale)
+		scan_bss_free(stale);
+
+	station->connected_bss = new;
+
+	scan_bss_compute_rank(new);
+	l_queue_insert(station->bss_list, new, scan_bss_rank_compare, NULL);
+
+	station_enter_state(station, STATION_STATE_CONNECTED);
+}
+
 static void station_rssi_level_changed(struct station *station,
 					uint8_t level_idx);
 
@@ -2319,6 +2336,12 @@ static void station_netdev_event(struct netdev *netdev, enum netdev_event event,
 	case NETDEV_EVENT_RSSI_LEVEL_NOTIFY:
 		station_rssi_level_changed(station, l_get_u8(event_data));
 		break;
+	case NETDEV_EVENT_ROAMING:
+		station_enter_state(station, STATION_STATE_ROAMING);
+		break;
+	case NETDEV_EVENT_ROAMED:
+		station_event_roamed(station, (struct scan_bss *) event_data);
+		break;
 	}
 }
 
-- 
2.26.2

[PATCH 5/6] wiphy: parse NL80211_ATTR_ROAM_SUPPORT flag

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

This tells us if the hardware is going to automatically
roam. We need this to know if station roaming logic should
be disabled.
---
 src/wiphy.c | 9 +++++++++
 src/wiphy.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/src/wiphy.c b/src/wiphy.c
index 3adc5669..d9c26bd3 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -95,6 +95,7 @@ struct wiphy {
 	bool support_adhoc_rsn:1;
 	bool support_qos_set_map:1;
 	bool support_cmds_auth_assoc:1;
+	bool support_fw_roam:1;
 	bool soft_rfkill : 1;
 	bool hard_rfkill : 1;
 	bool offchannel_tx_ok : 1;
@@ -461,6 +462,11 @@ bool wiphy_supports_qos_set_map(struct wiphy *wiphy)
 	return wiphy->support_qos_set_map;
 }
 
+bool wiphy_supports_fw_roam(struct wiphy *wiphy)
+{
+	return wiphy->support_fw_roam;
+}
+
 const char *wiphy_get_driver(struct wiphy *wiphy)
 {
 	return wiphy->driver_str;
@@ -989,6 +995,9 @@ static void wiphy_parse_attributes(struct wiphy *wiphy,
 			else
 				wiphy->max_roc_duration = *((uint32_t *) data);
 			break;
+		case NL80211_ATTR_ROAM_SUPPORT:
+			wiphy->support_fw_roam = true;
+			break;
 		}
 	}
 }
diff --git a/src/wiphy.h b/src/wiphy.h
index 50c8c936..e214ea70 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -92,6 +92,7 @@ const uint8_t *wiphy_get_supported_rates(struct wiphy *wiphy, unsigned int band,
 bool wiphy_supports_adhoc_rsn(struct wiphy *wiphy);
 bool wiphy_can_offchannel_tx(struct wiphy *wiphy);
 bool wiphy_supports_qos_set_map(struct wiphy *wiphy);
+bool wiphy_supports_fw_roam(struct wiphy *wiphy);
 const char *wiphy_get_driver(struct wiphy *wiphy);
 const char *wiphy_get_name(struct wiphy *wiphy);
 const uint8_t *wiphy_get_permanent_address(struct wiphy *wiphy);
-- 
2.26.2

[PATCH 6/6] station: disable roaming logic for auto-roaming cards

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

If the hardware roams automatically we want to be sure to not
react to CQM events and attempt to roam/disconnect on our own.

Note: this is only important for very new kernels where CQM
events were recently added to brcmfmac.
---
 src/station.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/station.c b/src/station.c
index f9fa0d71..fd985a1b 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2170,6 +2170,14 @@ static bool station_cannot_roam(struct station *station)
 	const struct l_settings *config = iwd_get_config();
 	bool disabled;
 
+	/*
+	 * Disable roaming with hardware that can roam automatically. Note this
+	 * is now required for recent kernels which have CQM event support on
+	 * this type of hardware (e.g. brcmfmac).
+	 */
+	if (wiphy_supports_fw_roam(station->wiphy))
+		return true;
+
 	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
 								&disabled))
 		disabled = false;
-- 
2.26.2

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Hi James,

On 3/10/21 2:27 PM, James Prestwood wrote:
> If the hardware roams automatically we want to be sure to not
> react to CQM events and attempt to roam/disconnect on our own.
> 
> Note: this is only important for very new kernels where CQM
> events were recently added to brcmfmac.
> ---
>   src/station.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/src/station.c b/src/station.c
> index f9fa0d71..fd985a1b 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -2170,6 +2170,14 @@ static bool station_cannot_roam(struct station *station)
>   	const struct l_settings *config = iwd_get_config();
>   	bool disabled;
>   
> +	/*
> +	 * Disable roaming with hardware that can roam automatically. Note this
> +	 * is now required for recent kernels which have CQM event support on
> +	 * this type of hardware (e.g. brcmfmac).
> +	 */
> +	if (wiphy_supports_fw_roam(station->wiphy))
> +		return true;
> +

Hmm, isn't this completely opposite to the intent of adding this capability to 
brcmfmac?

>   	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
>   								&disabled))
>   		disabled = false;
> 

Regards,
-Denis

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]

On Wed, 2021-03-10 at 15:01 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 3/10/21 2:27 PM, James Prestwood wrote:
> > If the hardware roams automatically we want to be sure to not
> > react to CQM events and attempt to roam/disconnect on our own.
> > 
> > Note: this is only important for very new kernels where CQM
> > events were recently added to brcmfmac.
> > ---
> >   src/station.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/station.c b/src/station.c
> > index f9fa0d71..fd985a1b 100644
> > --- a/src/station.c
> > +++ b/src/station.c
> > @@ -2170,6 +2170,14 @@ static bool station_cannot_roam(struct
> > station *station)
> >   	const struct l_settings *config = iwd_get_config();
> >   	bool disabled;
> >   
> > +	/*
> > +	 * Disable roaming with hardware that can roam automatically.
> > Note this
> > +	 * is now required for recent kernels which have CQM event
> > support on
> > +	 * this type of hardware (e.g. brcmfmac).
> > +	 */
> > +	if (wiphy_supports_fw_roam(station->wiphy))
> > +		return true;
> > +
> 
> Hmm, isn't this completely opposite to the intent of adding this
> capability to 
> brcmfmac?

Unless there is a way to disable FW roaming we cannot allow station to
roam based on CQM events. We don't have any control when the FW roams
so for cards like this I figured it was needed to disable this roaming
logic.

I've only tested this on my single brcmfmac card, which definitely will
not work right if station reacts to CQM events. Plus station cannot
even do FT on these cards anyways.

Maybe Alvin has some insight to this?

> >   	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
> >   								&disabl
> > ed))
> >   		disabled = false;
> > 
> 
> Regards,
> -Denis

Thanks,
James

[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 2795 bytes --]

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

Hi James,

>> Hmm, isn't this completely opposite to the intent of adding this capability to
>> brcmfmac?
> 
> Unless there is a way to disable FW roaming we cannot allow station to roam 

Hm, why not?  Isn't the worst case that we run our roam logic as well?  In 
theory we could set the CQM threshold to be more aggressive and attempt roams 
before the firmware does...

> based on CQM events. We don't have any control when the FW roams so for cards 
> like this I figured it was needed to disable this roaming logic.
> 
> I've only tested this on my single brcmfmac card, which definitely will not work 

What fails?

> right if station reacts to CQM events. Plus station cannot even do FT on these 
> cards anyways.

Sure, and FT is a bit tricky.  But FT is not the only way to roam...

> 
> Maybe Alvin has some insight to this?
> 

Yeah, I'm actually curious why we haven't heard of any NL80211_CMD_ROAM related 
issues ?  I believe I remember Alvin reporting that iwd was working fine on his 
brcmfmac hardware + cqm reporting enabled kernel...

Regards,
-Denis

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

On Wed, 2021-03-10 at 15:27 -0600, Denis Kenzior wrote:
> Hi James,
> 
> > > Hmm, isn't this completely opposite to the intent of adding this
> > > capability to
> > > brcmfmac?
> > 
> > Unless there is a way to disable FW roaming we cannot allow station
> > to roam 
> 
> Hm, why not?  Isn't the worst case that we run our roam logic as
> well?  In 
> theory we could set the CQM threshold to be more aggressive and
> attempt roams 
> before the firmware does...

Yes worst case is we get the CQM event, do a roam scan, and try to
roam. Then the FW decides its going to roam and CMD_ROAM comes in
during our own roam. I have no idea what that would even mean, I
suspect ultimately we would just get disconnected.

I could try grabbing Alvins patches and trying this. If we could roam
prior to the FW that should work. The problem is we have literally no
idea when or how the FW is going to roam. On these cards it really
seems like the intent is to let the hardware do it on its own. Maybe
Alvin's patches actually change the threshold in the FW?

> 
> > based on CQM events. We don't have any control when the FW roams so
> > for cards 
> > like this I figured it was needed to disable this roaming logic.
> > 
> > I've only tested this on my single brcmfmac card, which definitely
> > will not work 
> 
> What fails?

So as things stand now we get CMD_ROAM and then we just sit there and
don't do anything. This results in the 4-way timing out and a
disconnect.

> 
> > right if station reacts to CQM events. Plus station cannot even do
> > FT on these 
> > cards anyways.
> 
> Sure, and FT is a bit tricky.  But FT is not the only way to roam...
> 
> > Maybe Alvin has some insight to this?
> > 
> 
> Yeah, I'm actually curious why we haven't heard of any
> NL80211_CMD_ROAM related 
> issues ?  I believe I remember Alvin reporting that iwd was working
> fine on his 
> brcmfmac hardware + cqm reporting enabled kernel...

Yeah not sure on this one. I guess we just need to make a policy
decision here. If we always want to roam ourselves then we need a way
to prevent the FW from roaming. If we want to allow both we need to
disable our roaming logic for cards that do it on their own. Allowing
FW roaming plus our own roaming logic just seems like a logic
nightmare.

> 
> Regards,
> -Denis

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]

Hi both,

On 3/10/21 10:48 PM, James Prestwood wrote:
> On Wed, 2021-03-10 at 15:27 -0600, Denis Kenzior wrote:
>> Hi James,
>>
>>>> Hmm, isn't this completely opposite to the intent of adding this
>>>> capability to
>>>> brcmfmac?
>>>
>>> Unless there is a way to disable FW roaming we cannot allow station
>>> to roam
>>
>> Hm, why not?  Isn't the worst case that we run our roam logic as
>> well?  In
>> theory we could set the CQM threshold to be more aggressive and
>> attempt roams
>> before the firmware does...
> 
> Yes worst case is we get the CQM event, do a roam scan, and try to
> roam. Then the FW decides its going to roam and CMD_ROAM comes in
> during our own roam. I have no idea what that would even mean, I
> suspect ultimately we would just get disconnected.
> 
> I could try grabbing Alvins patches and trying this. If we could roam
> prior to the FW that should work. The problem is we have literally no
> idea when or how the FW is going to roam. On these cards it really
> seems like the intent is to let the hardware do it on its own. Maybe
> Alvin's patches actually change the threshold in the FW?

Roam offload is enabled by default in brcmfmac, but it can be disabled 
by setting the module parameter roamoff=1. Note this stands for "roam 
(offload) off" - not "roam off(load)" - so 1 means that the firmware 
will not roam autonomously. Anyway, in that case the driver will not set 
the WIPHY_FLAG_SUPPORTS_FW_ROAM flag:

drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:
7180	if (!ifp->drvr->settings->roamoff)
7181		wiphy->flags |= WIPHY_FLAG_SUPPORTS_FW_ROAM;

So from that perspective, I think James' patch looks perfectly fine. I 
have not tested it though.

>>> based on CQM events. We don't have any control when the FW roams so
>>> for cards
>>> like this I figured it was needed to disable this roaming logic.
>>>
>>> I've only tested this on my single brcmfmac card, which definitely
>>> will not work
>>
>> What fails?
> 
> So as things stand now we get CMD_ROAM and then we just sit there and
> don't do anything. This results in the 4-way timing out and a
> disconnect.

Yes, this was what I experienced when the firmware roamed by itself. 
Since we rely on iwd to handle the 4-Way Handshake, I instead worked on 
getting roaming to work with roam offload disabled. But I didn't think 
to go back and submit some iwd patches to handle the original failure 
case - my bad.

> 
>>
>>> right if station reacts to CQM events. Plus station cannot even do
>>> FT on these
>>> cards anyways.
>>
>> Sure, and FT is a bit tricky.  But FT is not the only way to roam...

Right, and I naively hoped that I might be able to get FT roaming to 
work too. The latest news I have from our hardware vendor is that - in 
theory - the chip firmware supports sending reassoc/auth frames, but I 
have not received any documentation yet. As it stands, brcmfmac does not 
implement the relevant cfg80211 ops, so one must settle for non-FT 
roaming if one wishes to use iwd. That would change with the incoming 
support for 4-Way Handshake offload in iwd, in which case both things 
can be offloaded to the firmware.

Personally I am a bit distrustful of the firmware, so it seemed like a 
better use of my time to rest control of the entire roaming process away 
from the firmware and into the supplicant. The lack of FT support is 
disappointing though, so perhaps I should have just worked on 4-Way 
Handshake offload support for iwd instead. I'm not working on the FT 
stuff these days but I'll follow up when I have some time.

>>> Maybe Alvin has some insight to this?
>>>
>>
>> Yeah, I'm actually curious why we haven't heard of any
>> NL80211_CMD_ROAM related
>> issues ?  I believe I remember Alvin reporting that iwd was working
>> fine on his
>> brcmfmac hardware + cqm reporting enabled kernel...
> 
> Yeah not sure on this one. I guess we just need to make a policy
> decision here. If we always want to roam ourselves then we need a way
> to prevent the FW from roaming. If we want to allow both we need to
> disable our roaming logic for cards that do it on their own. Allowing
> FW roaming plus our own roaming logic just seems like a logic
> nightmare.

The simple answer is that it never worked for me until I disabled 
roaming in the firmware (roamoff=1).

I agree with James when he says that enabling roaming in both supplicant 
and firmware is going to be a nightmare. I also don't think that the 
vendors have tested this use-case to begin with. The firmware actually 
has a number of variables that can be configured via the proprietary 
`wl` command line tool to set roaming thresholds and things like that. 
But the kernel patch I sent doesn't have anything to do with that - it 
is purely for RSSI monitoring.

Sadly I think there are some philosophical compromises that one has to 
make when dealing with fullmac chips/drivers on Linux, at least with the 
way things are right now...

> 
>>
>> Regards,
>> -Denis
> 

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3452 bytes --]

Hi Alvin,

> Roam offload is enabled by default in brcmfmac, but it can be disabled
> by setting the module parameter roamoff=1. Note this stands for "roam
> (offload) off" - not "roam off(load)" - so 1 means that the firmware
> will not roam autonomously. Anyway, in that case the driver will not set
> the WIPHY_FLAG_SUPPORTS_FW_ROAM flag:
> 
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:
> 7180	if (!ifp->drvr->settings->roamoff)
> 7181		wiphy->flags |= WIPHY_FLAG_SUPPORTS_FW_ROAM;
> 
> So from that perspective, I think James' patch looks perfectly fine. I
> have not tested it though.

Aha!  Thanks for the explanation.  In light of this I concur that we should take 
James' proposed approach and disable iwd's roaming logic if the flag is set.

> Yes, this was what I experienced when the firmware roamed by itself.
> Since we rely on iwd to handle the 4-Way Handshake, I instead worked on
> getting roaming to work with roam offload disabled. But I didn't think
> to go back and submit some iwd patches to handle the original failure
> case - my bad.
> 

No worries, I was just confused how you were not hitting this case and wondered 
whether you had more aggressive roaming thresholds so iwd roamed ahead of the 
firmware.

>>
>>>
>>>> right if station reacts to CQM events. Plus station cannot even do
>>>> FT on these
>>>> cards anyways.
>>>
>>> Sure, and FT is a bit tricky.  But FT is not the only way to roam...
> 
> Right, and I naively hoped that I might be able to get FT roaming to
> work too. The latest news I have from our hardware vendor is that - in
> theory - the chip firmware supports sending reassoc/auth frames, but I
> have not received any documentation yet. As it stands, brcmfmac does not
> implement the relevant cfg80211 ops, so one must settle for non-FT
> roaming if one wishes to use iwd. That would change with the incoming
> support for 4-Way Handshake offload in iwd, in which case both things
> can be offloaded to the firmware.

Right.  This is also a problem for SAE and OWE.  We are also having trouble 
locating brcmfmac based hardware that is SAE/WPA3 or FT offload capable.  Any 
pointers?

It may actually be nice if brcmfmac allowed us to do Authenticate/Associate 
without the offload.  I'm fairly certain 802.1X + offload still has many 
surprises in store for us...

> 
> Personally I am a bit distrustful of the firmware, so it seemed like a
> better use of my time to rest control of the entire roaming process away

Agreed.

> from the firmware and into the supplicant. The lack of FT support is
> disappointing though, so perhaps I should have just worked on 4-Way
> Handshake offload support for iwd instead. I'm not working on the FT
> stuff these days but I'll follow up when I have some time.

That'd be great.

<snip>

>> Yeah not sure on this one. I guess we just need to make a policy
>> decision here. If we always want to roam ourselves then we need a way
>> to prevent the FW from roaming. If we want to allow both we need to
>> disable our roaming logic for cards that do it on their own. Allowing
>> FW roaming plus our own roaming logic just seems like a logic
>> nightmare.
> 
> The simple answer is that it never worked for me until I disabled
> roaming in the firmware (roamoff=1).

May be academic at this point, but I'm still curious as to what actually failed?

Regards,
-Denis

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 10823 bytes --]

Hi Denis,

On 3/11/21 3:36 AM, Denis Kenzior wrote:
> Hi Alvin,
> 
>> Roam offload is enabled by default in brcmfmac, but it can be disabled
>> by setting the module parameter roamoff=1. Note this stands for "roam
>> (offload) off" - not "roam off(load)" - so 1 means that the firmware
>> will not roam autonomously. Anyway, in that case the driver will not set
>> the WIPHY_FLAG_SUPPORTS_FW_ROAM flag:
>>
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:
>> 7180    if (!ifp->drvr->settings->roamoff)
>> 7181        wiphy->flags |= WIPHY_FLAG_SUPPORTS_FW_ROAM;
>>
>> So from that perspective, I think James' patch looks perfectly fine. I
>> have not tested it though.
> 
> Aha!  Thanks for the explanation.  In light of this I concur that we 
> should take James' proposed approach and disable iwd's roaming logic if 
> the flag is set.
> 
>> Yes, this was what I experienced when the firmware roamed by itself.
>> Since we rely on iwd to handle the 4-Way Handshake, I instead worked on
>> getting roaming to work with roam offload disabled. But I didn't think
>> to go back and submit some iwd patches to handle the original failure
>> case - my bad.
>>
> 
> No worries, I was just confused how you were not hitting this case and 
> wondered whether you had more aggressive roaming thresholds so iwd 
> roamed ahead of the firmware.
> 
>>>
>>>>
>>>>> right if station reacts to CQM events. Plus station cannot even do
>>>>> FT on these
>>>>> cards anyways.
>>>>
>>>> Sure, and FT is a bit tricky.  But FT is not the only way to roam...
>>
>> Right, and I naively hoped that I might be able to get FT roaming to
>> work too. The latest news I have from our hardware vendor is that - in
>> theory - the chip firmware supports sending reassoc/auth frames, but I
>> have not received any documentation yet. As it stands, brcmfmac does not
>> implement the relevant cfg80211 ops, so one must settle for non-FT
>> roaming if one wishes to use iwd. That would change with the incoming
>> support for 4-Way Handshake offload in iwd, in which case both things
>> can be offloaded to the firmware.
> 
> Right.  This is also a problem for SAE and OWE.  We are also having 
> trouble locating brcmfmac based hardware that is SAE/WPA3 or FT offload 
> capable.  Any pointers?

I think it's at least partially a firmware constraint. My 
hardware/firmware pairing doesn't seem to support SAE/WPA3 because it's 
not in the list of features advertised with this wl command:

# wl cap
ap sta wme 802.11d 802.11h rm cqa ccx cac dualband ampdu ampdu_tx 
ampdu_rx amsdurx amsdutx radio_pwrsave p2p proptxstatus mchan wds 
vht-prop-rates dfrts txpwrcache stbc-tx stbc-rx-1ss pspretend rsdb 
probresp_mac_filter mfp wnm bsstrans ndoe mimo_ps epno pfnx scanmac

Nor is it seen here:

# cat /sys/kernel/debug/ieee80211/phy0/features
Features: 001218b6
	MCHAN
	PNO
	P2P
	RSDB
	SCAN_RANDOM_MAC
	MFP
	GSCAN
	DOT11H
	DUMP_OBSS

Quirks:   00000000

If you load the driver with the debug=4 parameter set (enables INFO 
debug logging, you may have to build with -DDEBUG), you can see which 
features get detected and enabled, which corresponds to the above sysfs 
output:

[   10.046614] brcmfmac: brcmf_feat_firmware_capabilities [ ap sta wme 
802.11d 802.11h rm cqa ccx cac dualband ampdu ampdu_tx ampdu_rx amsdurx 
amsdutx radio_pwrsave p2p proptxstatus mchan wds vht-prop-rates dfrts 
txpwrcache stbc-tx stbc-rx-1ss pspretend rsdb probresp_mac_filter mfp 
wnm bsstrans ndoe mimo_ps epno pfnx scanmac ]
[   10.046634] brcmfmac: brcmf_feat_firmware_capabilities enabling 
feature: MCHAN
[   10.046638] brcmfmac: brcmf_feat_firmware_capabilities enabling 
feature: P2P
[   10.046662] brcmfmac: brcmf_feat_firmware_capabilities enabling 
feature: DOT11H
[   10.046931] brcmfmac: brcmf_feat_iovar_data_set enabling feature: GSCAN
[   10.047153] brcmfmac: brcmf_feat_iovar_int_get enabling feature: PNO
[   10.047559] brcmfmac: brcmf_feat_iovar_int_get enabling feature: RSDB
[   10.047972] brcmfmac: brcmf_feat_iovar_int_get enabling feature: MFP
[   10.048179] brcmfmac: brcmf_feat_iovar_int_get enabling feature: 
DUMP_OBSS

Finally you can also grep the version string of the firmware file:

# strings /lib/firmware/cypress/cyfmac4359-pcie.bin | grep Ve
rsion
4359b1-roml/pcie-wl11u-mfp-nvramadj-sr-die3-pwrofs-wnm-mobfd-txbf-mimopscan-slna-nsslimit-lpc-wepso-pspretend-apbs-apcs-spurcan-clm_min-obss-cynctsf-obssdump-rxactfrm-kmdump 
Version: ...

Which can also give a hint as to the features (although it doesn't seem 
to map 1-1 with the above).

So to answer your question, I don't know directly, but then I did some 
searching online and found this thread[1] where a guy does the same grep:

$ strings /lib/firmware/brcm/brcmfmac43455-sdio.bin | grep Version
43455c0-roml/43455_sdio-pno-aoe-pktfilter-pktctx-wfds-mfp-dfsradar-wowlpf-idsup-idauth-noclminc-clm_min-obss-obssdump-swdiv-gtkoe-roamprof-txbf-ve-sae-sr-okc-bpd 
Version: ...

And there I can see 'sae', so there's a good chance that this firmware 
supports SAE offload. For WPA3 and FT offload I don't know what the 
corresponding string is but I believe (firmware driven) FT is a standard 
feature so it might not have its own string. More on that later...

So I checked out the latest FMAC/FW releases from Cypress[2] and did the 
same grepping:

alvin(a)capella ~/Downloads/cypress/5.4.18-2020_0925/firmware $ for i in 
`ls *.bin`; do x=$(strings $i | grep Version | grep sae); if [ ! -z "$x" 
]; then echo "$i"; echo "$x"; fi; done
cyfmac43012-sdio.bin
43012c0-roml/threadx-sdio-ag-p2p-keepalive-sr-proptxstatus-fcbs-ipa-idsup-idauth-consuartcc-dwt-pf2-mcs8support-cca-ipcamfeatures-bcntrim-wmetp-ulp-pno-aoe-txbf-wnm-bwte-ve-mfp-roamprof-dltro-mchan-dbguc-dbgmac-srampmac-utrace-tko-mchandump-tcpka-extgpio-sae-okc-scancache-clm_min-pwrstats-phyextiovar-dpp 
(config_sdio_release)  Version: 13.10.271.253 (c4c4c7c CY) CRC: 3c8e9ce4 
Date: Wed 2020-09-09 01:30:00 PDT Ucode Ver: 1182.166 FWID 01-3d0fbe29
cyfmac43430-sdio.bin
43430a1-roml/sdio-g-pool-p2p-idsup-idauth-pktfilter-keepalive-aoe-lpc-swdiv-srfast-fuart-btcxhybridhw-noclminc-clm_min-fbt-mfp-sae-tko 
Version: 7.45.98.97 (r724416 CY) CRC: 588b07d3 Date: Sun 2020-02-16 
22:41:02 PST Ucode Ver: 1043.2137 FWID 01-bf41ed64
cyfmac43455-sdio.bin
43455c0-roml/43455_sdio-pno-aoe-pktfilter-pktctx-wfds-mfp-dfsradar-wowlpf-idsup-idauth-noclminc-clm_min-obss-obssdump-swdiv-gtkoe-roamprof-txbf-ve-sae-dpp-sr-okc-bpd 
Version: 7.45.221 (3a6d3a0 CY) CRC: 8f977704 Date: Fri 2020-09-18 
02:30:48 PDT Ucode Ver: 1043.2156 FWID 01-bbd9282b
cyfmac4373-sdio.bin
4373a0-roml/sdio-ag-p2p-mchan-wfds-idsup-idauth-wowlpf-pktfilter-swdivenhanced-aoe-mfp-clm_min-sr-txbf-proptxstatus-sae-2040coex 
(config_sdio_release)  Version: 13.10.246.234 (c294843 CY) CRC: e0db40f8 
Date: Thu 2020-08-06 15:14:45 PDT Ucode Ver: 1191.1 FWID 01-f6e93fbb
cyfmac4373-usb.bin
4373a0-roml/usb-ag-p2p-mchan-wfds-idsup-idauth-wowlpf-pktfilter-swdivenhanced-aoe-mfp-clm_min-sr-txbf-proptxstatus-sae-2040coex-bdcsr-msgtrace 
(config_usb_release)  Version: 13.10.246.234 (c294843 CY) CRC: da9e5c6b 
Date: Thu 2020-08-06 15:09:17 PDT Ucode Ver: 1191.1 FWID 01-d5cfbedf
cyfmac54591-pcie.bin
4359d0-roml/pcie-wl11u-mfp-sr-apcs-phyflags-ag-pktfilter-p2p-mchan-proptxstatus-ampduhostreorder-powoff-pwrofs5g-pwrstats-lpc-clm_min-rxdesens-wapi-txbf-dfsradar-murx-pprhtoffset-txpwr-mbss-rsdbscan-btcdyn-apbs-dbgrsdb-cynctsf-sae-idsup-idauth 
(config_pcie_release_sae_cync)  Version: 13.35.173 (r725742 CY) CRC: 
d513bd43 Date: Fri 2020-05-22 00:12:18 PDT Ucode Ver: 1184.1035 FWID 
01-93eb441a

So that should be a good hint. I can see for example 
cyfmac43455-sdio.bin is used by some RPi models, which should be pretty 
easy to get your hands on. Then download the relevant firmware/NVRAM/CLM 
blob files and stick them in the /lib/firmware path and try running one 
of the commands I showed upstairs. You should probably see something like:

brcmfmac: brcmf_feat_firmware_capabilities enabling feature: SAE

Without having my hands on the particular hardware it's hard for me to 
be sure, but I hope the above can point you in the right direction.

One more thing about FT: Support for FT is only relevant with roam 
offload enabled, as I mentioned in my last mail. But if you want to test 
that out then you can just verify with a wireless monitor whether it's 
actually doing FT when it roams. Cumbersome I know, but I must admit I 
haven't actually verified the functionality myself because of the 
existing trouble I had with roam offload + iwd.

Hope that helps!

[1] https://github.com/raspberrypi/firmware/issues/1403
[2] 
https://community.cypress.com/t5/Resource-Library/Cypress-Linux-WiFi-Driver-Release-FMAC-2020-09-25/ta-p/251089

> 
> It may actually be nice if brcmfmac allowed us to do 
> Authenticate/Associate without the offload.  I'm fairly certain 802.1X + 
> offload still has many surprises in store for us...
> 
>>
>> Personally I am a bit distrustful of the firmware, so it seemed like a
>> better use of my time to rest control of the entire roaming process away
> 
> Agreed.
> 
>> from the firmware and into the supplicant. The lack of FT support is
>> disappointing though, so perhaps I should have just worked on 4-Way
>> Handshake offload support for iwd instead. I'm not working on the FT
>> stuff these days but I'll follow up when I have some time.
> 
> That'd be great.
> 
> <snip>
> 
>>> Yeah not sure on this one. I guess we just need to make a policy
>>> decision here. If we always want to roam ourselves then we need a way
>>> to prevent the FW from roaming. If we want to allow both we need to
>>> disable our roaming logic for cards that do it on their own. Allowing
>>> FW roaming plus our own roaming logic just seems like a logic
>>> nightmare.
>>
>> The simple answer is that it never worked for me until I disabled
>> roaming in the firmware (roamoff=1).
> 
> May be academic at this point, but I'm still curious as to what actually 
> failed?

It's been a while now and I'm not in the office this week to test again, 
but what James wrote sounds pretty much like what I experienced:

> So as things stand now we get CMD_ROAM and then we just sit there and
> don't do anything. This results in the 4-way timing out and a
> disconnect.

I should note that I tested this before implementing the CQM stuff in 
brcmfmac, so roaming did not work at all to begin with. I.e. roam 
offloading resulted in the above scenario, and roam offloading disabled 
resulted in iwd not doing anything because it didn't get notified about 
RSSI (lack of CQM API).

Kind regards,
Alvin

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

Hi Alvin,

<snip>

> So to answer your question, I don't know directly, but then I did
> some 
> searching online and found this thread[1] where a guy does the same
> grep:
> 
> $ strings /lib/firmware/brcm/brcmfmac43455-sdio.bin | grep Version
> 43455c0-roml/43455_sdio-pno-aoe-pktfilter-pktctx-wfds-mfp-dfsradar-
> wowlpf-idsup-idauth-noclminc-clm_min-obss-obssdump-swdiv-gtkoe-
> roamprof-txbf-ve-sae-sr-okc-bpd 
> Version: ...
> 
> And there I can see 'sae', so there's a good chance that this
> firmware 
> supports SAE offload. For WPA3 and FT offload I don't know what the 
> corresponding string is but I believe (firmware driven) FT is a
> standard 
> feature so it might not have its own string. More on that later...

As a side note here on SAE, would you mind just doing 'iw list' and
seeing if SAE_OFFLOAD is listed in the extended feature list (for your
card). I also see 'sae' when I grep the FW, but its still not included
in ext features.

I have trying this on a Raspberry Pi, which is the brcmfac43455-
sdio.bin FW. I see you mentioned "cyfmac43455-sdio.bin", which I'm
guessing is a non-released version or something? I got my FW from an
official cypress release, Nov. 2020 I believe it was.

Thanks,
James

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[KeithG]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

James,

This is confusing to me as well. Since cypress took it over, they have
gone and renamed the driver cyfmac instead of brcmfmac. That is in
what is form cypress on their web pages:
https://community.cypress.com/gfawx74859/attachments/gfawx74859/resourcelibrary/1030/1/cypress-fmac-v5.4.18-2020_0925.zip
I was able to patch the kernel from this package and build it and it
builds the cyfmac* drivers. THese require the cyfmac firmware... IN
addiiton to building this patched kernel, I was also able to rename
them and make the firmware work with the (older) official brcmfmac
drivers. all in all confusing.

On Thu, Mar 11, 2021 at 11:32 AM James Prestwood <prestwoj@gmail.com> wrote:
>
> Hi Alvin,
>
> <snip>
>
> > So to answer your question, I don't know directly, but then I did
> > some
> > searching online and found this thread[1] where a guy does the same
> > grep:
> >
> > $ strings /lib/firmware/brcm/brcmfmac43455-sdio.bin | grep Version
> > 43455c0-roml/43455_sdio-pno-aoe-pktfilter-pktctx-wfds-mfp-dfsradar-
> > wowlpf-idsup-idauth-noclminc-clm_min-obss-obssdump-swdiv-gtkoe-
> > roamprof-txbf-ve-sae-sr-okc-bpd
> > Version: ...
> >
> > And there I can see 'sae', so there's a good chance that this
> > firmware
> > supports SAE offload. For WPA3 and FT offload I don't know what the
> > corresponding string is but I believe (firmware driven) FT is a
> > standard
> > feature so it might not have its own string. More on that later...
>
> As a side note here on SAE, would you mind just doing 'iw list' and
> seeing if SAE_OFFLOAD is listed in the extended feature list (for your
> card). I also see 'sae' when I grep the FW, but its still not included
> in ext features.
>
> I have trying this on a Raspberry Pi, which is the brcmfac43455-
> sdio.bin FW. I see you mentioned "cyfmac43455-sdio.bin", which I'm
> guessing is a non-released version or something? I got my FW from an
> official cypress release, Nov. 2020 I believe it was.
>
> Thanks,
> James
> _______________________________________________
> iwd mailing list -- iwd(a)lists.01.org
> To unsubscribe send an email to iwd-leave(a)lists.01.org

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hi Keith,

On Thu, 2021-03-11 at 12:18 -0600, KeithG wrote:
> James,
> 
> This is confusing to me as well. Since cypress took it over, they
> have
> gone and renamed the driver cyfmac instead of brcmfmac. That is in
> what is form cypress on their web pages:
> https://community.cypress.com/gfawx74859/attachments/gfawx74859/resourcelibrary/1030/1/cypress-fmac-v5.4.18-2020_0925.zip
> I was able to patch the kernel from this package and build it and it
> builds the cyfmac* drivers. THese require the cyfmac firmware... IN
> addiiton to building this patched kernel, I was also able to rename
> them and make the firmware work with the (older) official brcmfmac
> drivers. all in all confusing.

Well in any event thanks for this link. The most recent FW I could find
was Nov. 2020, and all the firmware files were 'brcmfmac-*' named. I
will give these a try, maybe SAE offload is actually enabled.

Thanks,
James

Re: [PATCH 6/6] station: disable roaming logic for auto-roaming cards

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 2785 bytes --]

Hi James,

On 3/11/21 6:32 PM, James Prestwood wrote:
> Hi Alvin,
> 
> <snip>
> 
>> So to answer your question, I don't know directly, but then I did
>> some
>> searching online and found this thread[1] where a guy does the same
>> grep:
>>
>> $ strings /lib/firmware/brcm/brcmfmac43455-sdio.bin | grep Version
>> 43455c0-roml/43455_sdio-pno-aoe-pktfilter-pktctx-wfds-mfp-dfsradar-
>> wowlpf-idsup-idauth-noclminc-clm_min-obss-obssdump-swdiv-gtkoe-
>> roamprof-txbf-ve-sae-sr-okc-bpd
>> Version: ...
>>
>> And there I can see 'sae', so there's a good chance that this
>> firmware
>> supports SAE offload. For WPA3 and FT offload I don't know what the
>> corresponding string is but I believe (firmware driven) FT is a
>> standard
>> feature so it might not have its own string. More on that later...
> 
> As a side note here on SAE, would you mind just doing 'iw list' and
> seeing if SAE_OFFLOAD is listed in the extended feature list (for your
> card). I also see 'sae' when I grep the FW, but its still not included
> in ext features.

Sure - I just checked and `iw list | grep SAE` doesn't show anything.

> 
> I have trying this on a Raspberry Pi, which is the brcmfac43455-
> sdio.bin FW. I see you mentioned "cyfmac43455-sdio.bin", which I'm
> guessing is a non-released version or something? I got my FW from an
> official cypress release, Nov. 2020 I believe it was.

Hmm ok, so maybe the 'sae' string doesn't mean anything after all... 
sorry for the noise.

Regarding the filename stuff: Cypress decided to change the FW filenames 
at some point last year, which is a bit maddening because I believe this 
stuff is also considered ABI. But the brcmfmac* and cyfmac* 
.bin/clm_blob/txt files are pretty much interchangeable AFAIK, so long 
as the numerical part of the chipsets matche, regardless of the BCM or 
CYW prefix. If you use a mainline kernel, I think it expects brcmfmac* 
files. If you use the Cypress release (see link in my last reply), then 
it will want the cyfmac* files.

If you search the internet you will find RPi users also confused by 
this, and that it seems like both Broadcom and Cypress claim to be 
vendors of the RPi wireless chipset. KeithG seems to have the same 
experience. But renaming the files to whatever your driver is expecting 
is all you have to do.

The cyw*.bin files I grepped are from the Cypress FMAC/FW release I 
linked in my last mail. It's the same thing KeithG linked too - just 
that he linked the .zip directly. Another source of firmwares is from 
muRata, who make SoMs based on the Cypress chipsets. They have a 
separate release repository for firmwares[1] and maybe one of those 
might have SAE support. Otherwise I'm out of ideas...

Kind regards,
Alvin

Re: [PATCH 1/6] netdev: use NL80211_STA_INFO_SIGNAL rather than average

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

Hi James,

On 3/10/21 2:27 PM, James Prestwood wrote:
> Since GET_STATION (and in turn GetDiagnostics) gets the most
> current station info this attribute serves as a better indication
> of the current signal strength. In addition full mac cards don't
> appear to always have the average attribute.
> ---
>   src/netdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

This seems fine, so I went ahead an applied it.  I do wonder if we should parse 
SIGNAL_AVG and report it separately (if available).

Regards,
-Denis

Re: [PATCH 1/6] netdev: use NL80211_STA_INFO_SIGNAL rather than average

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On Wed, 2021-03-10 at 15:12 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 3/10/21 2:27 PM, James Prestwood wrote:
> > Since GET_STATION (and in turn GetDiagnostics) gets the most
> > current station info this attribute serves as a better indication
> > of the current signal strength. In addition full mac cards don't
> > appear to always have the average attribute.
> > ---
> >   src/netdev.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> This seems fine, so I went ahead an applied it.  I do wonder if we
> should parse 
> SIGNAL_AVG and report it separately (if available).

Sure I can do that as well. It is probably a better indicator anyways
for what most people would care about.

> 
> Regards,
> -Denis