wifi: TI wilink8 updates

Hi,

This series updates the TI wilink8 driver targetting two issues. First are four driver correctness and/or improvements to the driver.

Patch 1 ensures that if the firmware log pointer somehow ends up beyond the buffer limit, we will wrap back to the start, rather than relying on an exact match for the end of the buffer.

Patch 2 avoids using the modulus operator for the loop over the WL18xx completed packet circular buffer, which can be expensive for some CPUs. Since we only ever increment the index by one, it is trivial to detect when we need to wrap.

Patch 3 improves the code in wlcore_fw_status() to make what's going on with status->counters.tx_lnk_free_pkts[] more clear.

Patch 4 removes a potential aliasing issue - wlcore_fw_status() is passed wl->fw_status as an argument, yet rather than using that for wlcore_hw_convert_fw_status(), we use wl->fw_status, and then go back to using the passed-in status to access the data written by this function.

Patch 5 is taken from one of TI's wilink8 patches adding support for later firmwares. This adds support for storing the AP key type, which will be used in connection with the pn16 changes found in newer firmware.

Patch 6 adds the necessary code for pn16 support, also taken from one of TI's wilink8 patches, augmented in two ways. First, if wlcore_hw_convert_fw_status() did not provide the pn16 array, then this code does nothing (thus maintaining compatibility with existing firmware.) Second, this is an array of 16-bit quantities, and so is subject to endian issues. Use the appropriate type and conversion functions for it.

Patch 7 adds support for parsing the status data structures with the new pn16 field buried in the middle (!). Since mainline has to maintain compatibility with existing firmware, and we can't do TI's silly lock-step "upgrade the firmware at the same time" thing, we maintain support for the old layout, and select the appropriate parsing function. We also resize the raw status array as appropriate.

Patch 8 allows the driver to accept both 8.9.0.x.>58 and 8.9.>=1.x.x firmwares.

 drivers/net/wireless/ti/wl18xx/main.c     |  71 ++++++++++++++++++++-
 drivers/net/wireless/ti/wl18xx/tx.c       |  13 +++-
 drivers/net/wireless/ti/wl18xx/wl18xx.h   |  62 +++++++++++++++++-
 drivers/net/wireless/ti/wlcore/cmd.c      |   9 +++
 drivers/net/wireless/ti/wlcore/event.c    |   2 +-
 drivers/net/wireless/ti/wlcore/main.c     | 101 +++++++++++++++++++++++++++---
 drivers/net/wireless/ti/wlcore/wlcore_i.h |   4 ++
 7 files changed, 246 insertions(+), 16 deletions(-)

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Fix the calculation of clear_offset, which may overflow the end of the buffer. However, this is harmless if it does because in that case it will be recalculated when we copy the chunk of messages at the start of the buffer.

---
 drivers/net/wireless/ti/wlcore/event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2499dc908305..6c3a8ea9613e 100644
@@ -83,7 +83,7 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	/* Copy initial part up to the end of ring buffer */
 	len = min(actual_len, available_len);
 	wl12xx_copy_fwlog(wl, &buffer[start_loc], len);
-	clear_ptr = addr_ptr + start_loc + actual_len;
+	clear_ptr = addr_ptr + start_loc + len;
 	if (clear_ptr == buff_end_ptr)
 		clear_ptr = buff_start_ptr;
 

2.30.2

8 patches applied to wireless-next.git, thanks.

64ff013ce098 wifi: wlcore: correctness fix fwlog reading b734d8830f70 wifi: wl18xx: make wl18xx_tx_immediate_complete() more efficient 97715e29cebc wifi: wlcore: improve code in wlcore_fw_status() dd265a7415f8 wifi: wlcore: pass "status" to wlcore_hw_convert_fw_status() 81271c2bc59e wifi: wlcore: store AP encryption key type bb8edd900fd6 wifi: wlcore: add pn16 support
9685262b5e5d wifi: wl18xx: add support for reading 8.9.1 fw status 8c58f972219e wifi: wl18xx: allow firmwares > 8.9.0.x.58

https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

wl18xx_tx_immediate_complete() iterates through the completed transmit descriptors in a circular fashion, and in doing so uses a modulus operation that is not a power of two. This leads to inefficient code generation, which can be easily solved by providing a helper to increment to the next descriptor. Use this more efficient solution.

---
 drivers/net/wireless/ti/wl18xx/tx.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

index 55d9b0861c53..beef393853ef 100644
@@ -129,6 +129,14 @@ static void wl18xx_tx_complete_packet(struct wl1271 *wl, u8 tx_stat_byte)
 	wl1271_free_tx_id(wl, id);
 }
 
+static u8 wl18xx_next_tx_idx(u8 idx)
+{
+	if (++idx >= WL18XX_FW_MAX_TX_STATUS_DESC)
+		idx = 0;
+
+	return idx;
+}
+
 void wl18xx_tx_immediate_complete(struct wl1271 *wl)
 {
 	struct wl18xx_fw_status_priv *status_priv =

@@ -161,9 +169,8 @@ void wl18xx_tx_immediate_complete(struct wl1271 *wl)
 		return;
 	}
 
-	for (i = priv->last_fw_rls_idx;
-	     i != status_priv->fw_release_idx;
-	     i = (i + 1) % WL18XX_FW_MAX_TX_STATUS_DESC) {
+	for (i = priv->last_fw_rls_idx; i != status_priv->fw_release_idx;
+	     i = wl18xx_next_tx_idx(i)) {
 		wl18xx_tx_complete_packet(wl,
 			status_priv->released_tx_desc[i]);
 

2.30.2

Referring to status->counters.tx_lnk_free_pkts[i] multiple times leads to less efficient code. Cache this value in a local variable. This also makes the code clearer.

---
 drivers/net/wireless/ti/wlcore/main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index ef12169f8044..a98b26dc3cb8 100644
@@ -412,18 +412,18 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
 
 
 	for_each_set_bit(i, wl->links_map, wl->num_links) {
-		u8 diff;
+		u8 diff, tx_lnk_free_pkts;
 		lnk = &wl->links[i];
 
 		/* prevent wrap-around in freed-packets counter */
-		diff = (status->counters.tx_lnk_free_pkts[i] -
-		       lnk->prev_freed_pkts) & 0xff;
+		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
+		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
 
 		if (diff == 0)
 			continue;
 
 		lnk->allocated_pkts -= diff;
-		lnk->prev_freed_pkts = status->counters.tx_lnk_free_pkts[i];
+		lnk->prev_freed_pkts = tx_lnk_free_pkts;
 
 		/* accumulate the prev_freed_pkts counter */
 		lnk->total_freed_pkts += diff;

2.30.2

wlcore_fw_status() is passed a pointer to the struct wl_fw_status to decode the status into, which is always wl->fw_status. Rather than referencing wl->fw_status within wlcore_fw_status(), use the supplied argument so that we access this member in a consistent manner.

---
 drivers/net/wireless/ti/wlcore/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a98b26dc3cb8..3defe49c5120 100644
@@ -392,7 +392,7 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
 	if (ret < 0)
 		return ret;
 
-	wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, wl->fw_status);
+	wlcore_hw_convert_fw_status(wl, wl->raw_fw_status, status);
 
 	wl1271_debug(DEBUG_IRQ, "intr: 0x%x (fw_rx_counter = %d, "
 		     "drv_rx_counter = %d, tx_results_counter = %d)",

2.30.2

Agree this is more consistent. Maybe *status shouldn't be an argument to wlcore_fw_status at all? It's called only in one place with wl->fw_status anyway.

Michael.

I did consider that, and if we removed the argument, it would make sense to add a local "status" variable at the top of this function anyway, otherwise endlessly referring to wl->fw_status.foo instead of status->foo becomes quite tiring and needlessly verbose (which means less readable.)

That's something which could be done as a separate patch.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Updates for WL18xx firmware 8.9.1.x.x need to know the AP encryption key type. Store this when a new key is set. Patch extracted from:

https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0023-wlcore-Fixing-PN-drift-on-encrypted-link-after-recov.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7

---
 drivers/net/wireless/ti/wlcore/main.c | 4 ++++
 1 file changed, 4 insertions(+)

index 3defe49c5120..8f82666f379c 100644
@@ -3537,6 +3537,10 @@ int wlcore_set_key(struct wl1271 *wl, enum set_key_cmd cmd,
 			return ret;
 		}
 
+		/* Store AP encryption key type */
+		if (wlvif->bss_type == BSS_TYPE_AP_BSS)
+			wlvif->encryption_type = key_type;
+
 		/*
 		 * reconfiguring arp response if the unicast (or common)
 		 * encryption key type was changed

2.30.2

TI Wl18xx firmware adds a "pn16" field for AES and TKIP keys as per their patch:

https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0023-wlcore-Fixing-PN-drift-on-encrypted-link-after-recov.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7

Add support for this, but rather than requiring the field to be present (which would break existing firmwares), make it optional.

---
 drivers/net/wireless/ti/wlcore/cmd.c      |  9 +++
 drivers/net/wireless/ti/wlcore/main.c     | 89 +++++++++++++++++++++--
 drivers/net/wireless/ti/wlcore/wlcore_i.h |  4 +
 3 files changed, 97 insertions(+), 5 deletions(-)

index a939fd89a7f5..0d1fcdca3869 100644
@@ -332,6 +332,14 @@ int wl12xx_allocate_link(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 *hlid)
 			wl->fw_status->counters.tx_lnk_free_pkts[link];
 	wl->links[link].wlvif = wlvif;
 
+	/*
+	 * Take the last sec_pn16 value from the current FW status. On recovery,
+	 * we might not have fw_status yet, and tx_lnk_sec_pn16[] will be NULL.
+	 */
+	if (wl->fw_status->counters.tx_lnk_sec_pn16)
+		wl->links[link].prev_sec_pn16 =
+			le16_to_cpu(wl->fw_status->counters.tx_lnk_sec_pn16[link]);
+
 	/*
 	 * Take saved value for total freed packets from wlvif, in case this is
 	 * recovery/resume

@@ -360,6 +368,7 @@ void wl12xx_free_link(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 *hlid)
 
 	wl->links[*hlid].allocated_pkts = 0;
 	wl->links[*hlid].prev_freed_pkts = 0;
+	wl->links[*hlid].prev_sec_pn16 = 0;
 	wl->links[*hlid].ba_bitmap = 0;
 	eth_zero_addr(wl->links[*hlid].addr);
 

index eefae3f867b9..5fbed64302f1 100644
@@ -151,6 +151,9 @@ struct wl_fw_status {
 		 */
 		u8 *tx_lnk_free_pkts;
 
+		/* PN16 of last TKIP/AES seq-num per HLID */
+		__le16 *tx_lnk_sec_pn16;
+
 		/* Cumulative counter of released Voice memory blocks */
 		u8 tx_voice_released_blks;
 

@@ -259,6 +262,7 @@ struct wl1271_link {
 	/* accounting for allocated / freed packets in FW */
 	u8 allocated_pkts;
 	u8 prev_freed_pkts;
+	u16 prev_sec_pn16;
 
 	u8 addr[ETH_ALEN];
 

2.30.2

On 5/28/2024 12:18 PM, Russell King (Oracle) wrote:

[...]

[...]

You are... While you're correct about the original code, your quote is somewhat incomplete.

+ if ( (isSta == true) && (i == wlvifSta->sta.hlid) && (test_bit(WLVIF_FLAG_STA_AUTHORIZED, &wlvifSta->flags)) && (status->counters.tx_lnk_free_pkts[i] > 0) )
...
+ }

+ if ( (isAp == true) && (test_bit(i, &wlvifAp->ap.sta_hlid_map[0])) && (test_bit(WLVIF_FLAG_AP_STARTED, &wlvifAp->flags)) && (wlvifAp->inconn_count == 0) && (status->counters.tx_lnk_free_pkts[i] > 0) )
...
+ }
}

Note that both of these if() conditions can only be executed if the final condition in each is true. Both check for the same thing, which is:

		status->counters.tx_lnk_free_pkts[i] > 0

In my patch, tx_lnk_free_pkts is status->counters.tx_lnk_free_pkts.

Therefore, there is no point in evaluating either of these excessively long if() conditions in the original code when tx_lnk_free_pkts is less than zero or zero - and thus the logic between TI's original patch and my change is preserved.

Whether that condition in the original patch is correct or not is the subject of that FIXME comment - I believe TI's code is incorrect, since it is possible that tx_lnk_free_pkts, which is a u8 that is incremented by the number of free packets, will hit zero at some point just as a matter of one extra packet being freed when the counter was 255.

Moving it out of those two if() statements makes the issue very obvious. It would be nice to get a view from TI on whether the original patch is actually correct in this regard. I believe TI's original patch is buggy.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On 5/30/2024 11:20 AM, Russell King (Oracle) wrote:
[...]

Sorry, considered only the diff with base branch, not the original patch.

After consulting with the author I do not believe it is buggy. It was the most painless way to prevent issues with the recovery flow.
Indeed there will be case where tx_lnk_free_pkts is 0 again in which case the internal counters will not be updated. This was considered OK as this is usually a transient state and the counter will advance eventually. For the unlikely case where FW crashed just after update was skipped, well, there will be a small rollback in the PN after recovery which means a few frames will get lost. This as considered acceptable.

Michael.

On 5/30/2024 3:25 PM, Nemanov, Michael wrote:
[...]

[...]

And sorry for making a mess of the quoted text on the previous message, got it sorted now.

Michael.

Add the necessary code to read the 8.9.1 firmware status into the driver private status structure, augmenting the 8.9.0 firmware status code. New structure layout taken from:

https://git.ti.com/cgit/wilink8-wlan/build-utilites/tree/patches/kernel_patches/4.19.38/0023-wlcore-Fixing-PN-drift-on-encrypted-link-after-recov.patch?h=r8.9&id=a2ee50aa5190ed3b334373d6cd09b1bff56ffcf7

---
 drivers/net/wireless/ti/wl18xx/main.c   | 71 ++++++++++++++++++++++++-
 drivers/net/wireless/ti/wl18xx/wl18xx.h | 60 +++++++++++++++++++++
 2 files changed, 129 insertions(+), 2 deletions(-)

index 2ccac1cdec01..39d8eebb9b6e 100644
@@ -1177,8 +1177,49 @@ static int wl18xx_hw_init(struct wl1271 *wl)
 	return ret;
 }
 
-static void wl18xx_convert_fw_status(struct wl1271 *wl, void *raw_fw_status,
-				     struct wl_fw_status *fw_status)
+static void wl18xx_convert_fw_status_8_9_1(struct wl1271 *wl,
+					   void *raw_fw_status,
+					   struct wl_fw_status *fw_status)
+{
+	struct wl18xx_fw_status_8_9_1 *int_fw_status = raw_fw_status;
+
+	fw_status->intr = le32_to_cpu(int_fw_status->intr);
+	fw_status->fw_rx_counter = int_fw_status->fw_rx_counter;
+	fw_status->drv_rx_counter = int_fw_status->drv_rx_counter;
+	fw_status->tx_results_counter = int_fw_status->tx_results_counter;
+	fw_status->rx_pkt_descs = int_fw_status->rx_pkt_descs;
+
+	fw_status->fw_localtime = le32_to_cpu(int_fw_status->fw_localtime);
+	fw_status->link_ps_bitmap = le32_to_cpu(int_fw_status->link_ps_bitmap);
+	fw_status->link_fast_bitmap =
+			le32_to_cpu(int_fw_status->link_fast_bitmap);
+	fw_status->total_released_blks =
+			le32_to_cpu(int_fw_status->total_released_blks);
+	fw_status->tx_total = le32_to_cpu(int_fw_status->tx_total);
+
+	fw_status->counters.tx_released_pkts =
+			int_fw_status->counters.tx_released_pkts;
+	fw_status->counters.tx_lnk_free_pkts =
+			int_fw_status->counters.tx_lnk_free_pkts;
+	fw_status->counters.tx_lnk_sec_pn16 =
+			int_fw_status->counters.tx_lnk_sec_pn16;
+	fw_status->counters.tx_voice_released_blks =
+			int_fw_status->counters.tx_voice_released_blks;
+	fw_status->counters.tx_last_rate =
+			int_fw_status->counters.tx_last_rate;
+	fw_status->counters.tx_last_rate_mbps =
+			int_fw_status->counters.tx_last_rate_mbps;
+	fw_status->counters.hlid =
+			int_fw_status->counters.hlid;
+
+	fw_status->log_start_addr = le32_to_cpu(int_fw_status->log_start_addr);
+
+	fw_status->priv = &int_fw_status->priv;
+}
+
+static void wl18xx_convert_fw_status_8_9_0(struct wl1271 *wl,
+					   void *raw_fw_status,
+					   struct wl_fw_status *fw_status)
 {
 	struct wl18xx_fw_status *int_fw_status = raw_fw_status;
 

@@ -1214,6 +1255,15 @@ static void wl18xx_convert_fw_status(struct wl1271 *wl, void *raw_fw_status,
 	fw_status->priv = &int_fw_status->priv;
 }
 
+static void wl18xx_convert_fw_status(struct wl1271 *wl, void *raw_fw_status,
+				     struct wl_fw_status *fw_status)
+{
+	if (wl->chip.fw_ver[FW_VER_MAJOR] == 0)
+		wl18xx_convert_fw_status_8_9_0(wl, raw_fw_status, fw_status);
+	else
+		wl18xx_convert_fw_status_8_9_1(wl, raw_fw_status, fw_status);
+}
+
 static void wl18xx_set_tx_desc_csum(struct wl1271 *wl,
 				    struct wl1271_tx_hw_descr *desc,
 				    struct sk_buff *skb)

@@ -1515,12 +1565,29 @@ static int wl18xx_handle_static_data(struct wl1271 *wl,
 {
 	struct wl18xx_static_data_priv *static_data_priv =
 		(struct wl18xx_static_data_priv *) static_data->priv;
+	size_t fw_status_len;
 
 	strscpy(wl->chip.phy_fw_ver_str, static_data_priv->phy_version,
 		sizeof(wl->chip.phy_fw_ver_str));
 
 	wl1271_info("PHY firmware version: %s", static_data_priv->phy_version);
 
+	/* Adjust the firmware status size according to the firmware version */
+	if (wl->chip.fw_ver[FW_VER_MAJOR] == 0)
+		fw_status_len = sizeof(struct wl18xx_fw_status);
+	else
+		fw_status_len = sizeof(struct wl18xx_fw_status_8_9_1);
+
+	if (wl->fw_status_len != fw_status_len) {
+		void *new_status = krealloc(wl->raw_fw_status, fw_status_len,
+					    GFP_KERNEL | __GFP_ZERO);
+		if (!new_status)
+			return -ENOMEM;
+
+		wl->raw_fw_status = new_status;
+		wl->fw_status_len = fw_status_len;
+	}
+
 	return 0;
 }
 

2.30.2

wlcore firmware versions are structured thusly:

chip.if-type.major.sub-type.minor
e.g. 8 9 0 0 58

With WL18xx ignoring the major firmware version, looking for a firmware version that conforms to:

	chip >= 8
	if-type >= 9
	major (don't care)
	sub-type (don't care)
	minor >= 58

Each test is satisfied if the value read from the firmware is greater than the minimum, but if it is equal (or we don't care about the field), then the next field is checked.

Thus it doesn't recognise 8.9.1.x.0 as being newer than 8.9.0.x.58 since the major and sub-type numbers are "don't care" and the minor needs to be greater or equal to 58.

We need to change the major version from "ignore" to "0" for this later firmware to be correctly detected, and allow the dual-firmware version support to work.

---
 drivers/net/wireless/ti/wl18xx/wl18xx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7fed96d71b27..de6c671c4be6 100644
@@ -13,7 +13,7 @@
 /* minimum FW required for driver */
 #define WL18XX_CHIP_VER		8
 #define WL18XX_IFTYPE_VER	9
-#define WL18XX_MAJOR_VER	WLCORE_FW_VER_IGNORE
+#define WL18XX_MAJOR_VER	0
 #define WL18XX_SUBTYPE_VER	WLCORE_FW_VER_IGNORE
 #define WL18XX_MINOR_VER	58
 

2.30.2