[PATCH wireless-next 0/8] wifi: TI wilink8 updates

[PATCH wireless-next 0/8] wifi: TI wilink8 updates

[Russell King (Oracle)]

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!

[PATCH wireless-next 1/8] wifi: wlcore: correctness fix fwlog reading

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/event.c b/drivers/net/wireless/ti/wlcore/event.c
index 2499dc908305..6c3a8ea9613e 100644
--- a/drivers/net/wireless/ti/wlcore/event.c
+++ b/drivers/net/wireless/ti/wlcore/event.c
@@ -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

Re: [PATCH wireless-next 1/8] wifi: wlcore: correctness fix fwlog reading

[Kalle Valo]

"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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/E1sBsxi-00E8vQ-5r@rmk-PC.armlinux.org.uk/

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

[PATCH wireless-next 2/8] wifi: wl18xx: make wl18xx_tx_immediate_complete() more efficient

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wl18xx/tx.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ti/wl18xx/tx.c b/drivers/net/wireless/ti/wl18xx/tx.c
index 55d9b0861c53..beef393853ef 100644
--- a/drivers/net/wireless/ti/wl18xx/tx.c
+++ b/drivers/net/wireless/ti/wl18xx/tx.c
@@ -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

[PATCH wireless-next 3/8] wifi: wlcore: improve code in wlcore_fw_status()

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index ef12169f8044..a98b26dc3cb8 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -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

[PATCH wireless-next 4/8] wifi: wlcore: pass "status" to wlcore_hw_convert_fw_status()

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index a98b26dc3cb8..3defe49c5120 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -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

Re: [PATCH wireless-next 4/8] wifi: wlcore: pass "status" to wlcore_hw_convert_fw_status()

[Nemanov, Michael]

On 5/28/2024 12:17 PM, Russell King (Oracle) wrote:
> 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.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>    drivers/net/wireless/ti/wlcore/main.c | 2 +-
>    1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
> index a98b26dc3cb8..3defe49c5120 100644
> --- a/drivers/net/wireless/ti/wlcore/main.c
> +++ b/drivers/net/wireless/ti/wlcore/main.c
> @@ -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.

Re: [PATCH wireless-next 4/8] wifi: wlcore: pass "status" to wlcore_hw_convert_fw_status()

[Russell King (Oracle)]

On Wed, May 29, 2024 at 04:15:13PM +0300, Nemanov, Michael wrote:
> On 5/28/2024 12:17 PM, Russell King (Oracle) wrote:
> > @@ -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.

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!

[PATCH wireless-next 5/8] wifi: wlcore: store AP encryption key type

[Russell King (Oracle)]

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 3defe49c5120..8f82666f379c 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -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

[PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 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(-)

diff --git a/drivers/net/wireless/ti/wlcore/cmd.c b/drivers/net/wireless/ti/wlcore/cmd.c
index a939fd89a7f5..0d1fcdca3869 100644
--- a/drivers/net/wireless/ti/wlcore/cmd.c
+++ b/drivers/net/wireless/ti/wlcore/cmd.c
@@ -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);
 
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index 8f82666f379c..35d1114a28aa 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -379,6 +379,8 @@ static void wl12xx_irq_update_links_status(struct wl1271 *wl,
 
 static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
 {
+	struct wl12xx_vif *wlvifsta;
+	struct wl12xx_vif *wlvifap;
 	struct wl12xx_vif *wlvif;
 	u32 old_tx_blk_count = wl->tx_blocks_available;
 	int avail, freed_blocks;
@@ -410,23 +412,100 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
 		wl->tx_pkts_freed[i] = status->counters.tx_released_pkts[i];
 	}
 
+	/* Find an authorized STA vif */
+	wlvifsta = NULL;
+	wl12xx_for_each_wlvif_sta(wl, wlvif) {
+		if (wlvif->sta.hlid != WL12XX_INVALID_LINK_ID &&
+		    test_bit(WLVIF_FLAG_STA_AUTHORIZED, &wlvif->flags)) {
+			wlvifsta = wlvif;
+			break;
+		}
+	}
+
+	/* Find a started AP vif */
+	wlvifap = NULL;
+	wl12xx_for_each_wlvif(wl, wlvif) {
+		if (wlvif->bss_type == BSS_TYPE_AP_BSS &&
+		    wlvif->inconn_count == 0 &&
+		    test_bit(WLVIF_FLAG_AP_STARTED, &wlvif->flags)) {
+			wlvifap = wlvif;
+			break;
+		}
+	}
 
 	for_each_set_bit(i, wl->links_map, wl->num_links) {
+		u16 diff16, sec_pn16;
 		u8 diff, tx_lnk_free_pkts;
+
 		lnk = &wl->links[i];
 
 		/* prevent wrap-around in freed-packets counter */
 		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
 		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
 
-		if (diff == 0)
+		if (diff) {
+			lnk->allocated_pkts -= diff;
+			lnk->prev_freed_pkts = tx_lnk_free_pkts;
+		}
+
+		/* Get the current sec_pn16 value if present */
+		if (status->counters.tx_lnk_sec_pn16)
+			sec_pn16 = __le16_to_cpu(status->counters.tx_lnk_sec_pn16[i]);
+		else
+			sec_pn16 = 0;
+		/* prevent wrap-around in pn16 counter */
+		diff16 = (sec_pn16 - lnk->prev_sec_pn16) & 0xffff;
+
+		/* FIXME: since free_pkts is a 8-bit counter of packets that
+		 * rolls over, it can become zero. If it is zero, then we
+		 * omit processing below. Is that really correct?
+		 */
+		if (tx_lnk_free_pkts <= 0)
 			continue;
 
-		lnk->allocated_pkts -= diff;
-		lnk->prev_freed_pkts = tx_lnk_free_pkts;
+		/* For a station that has an authorized link: */
+		if (wlvifsta && wlvifsta->sta.hlid == i) {
+			if (wlvifsta->encryption_type == KEY_TKIP ||
+			    wlvifsta->encryption_type == KEY_AES) {
+				if (diff16) {
+					lnk->prev_sec_pn16 = sec_pn16;
+					/* accumulate the prev_freed_pkts
+					 * counter according to the PN from
+					 * firmware
+					 */
+					lnk->total_freed_pkts += diff16;
+				}
+			} else {
+				if (diff)
+					/* accumulate the prev_freed_pkts
+					 * counter according to the free packets
+					 * count from firmware
+					 */
+					lnk->total_freed_pkts += diff;
+			}
+		}
 
-		/* accumulate the prev_freed_pkts counter */
-		lnk->total_freed_pkts += diff;
+		/* For an AP that has been started */
+		if (wlvifap && test_bit(i, wlvifap->ap.sta_hlid_map)) {
+			if (wlvifap->encryption_type == KEY_TKIP ||
+			    wlvifap->encryption_type == KEY_AES) {
+				if (diff16) {
+					lnk->prev_sec_pn16 = sec_pn16;
+					/* accumulate the prev_freed_pkts
+					 * counter according to the PN from
+					 * firmware
+					 */
+					lnk->total_freed_pkts += diff16;
+				}
+			} else {
+				if (diff)
+					/* accumulate the prev_freed_pkts
+					 * counter according to the free packets
+					 * count from firmware
+					 */
+					lnk->total_freed_pkts += diff;
+			}
+		}
 	}
 
 	/* prevent wrap-around in total blocks counter */
diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h
index eefae3f867b9..5fbed64302f1 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore_i.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h
@@ -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

Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Nemanov, Michael]

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

[...]

>    
>    static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
>    {
> +	struct wl12xx_vif *wlvifsta;
> +	struct wl12xx_vif *wlvifap;
>    	struct wl12xx_vif *wlvif;
>    	u32 old_tx_blk_count = wl->tx_blocks_available;
>    	int avail, freed_blocks;
> @@ -410,23 +412,100 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
>    		wl->tx_pkts_freed[i] = status->counters.tx_released_pkts[i];
>    	}
>    
[...]
>    	for_each_set_bit(i, wl->links_map, wl->num_links) {
> +		u16 diff16, sec_pn16;
>    		u8 diff, tx_lnk_free_pkts;
> +
>    		lnk = &wl->links[i];
>    
>    		/* prevent wrap-around in freed-packets counter */
>    		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
>    		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
>    
> -		if (diff == 0)
> +		if (diff) {
> +			lnk->allocated_pkts -= diff;
> +			lnk->prev_freed_pkts = tx_lnk_free_pkts;
> +		}
> +
> +		/* Get the current sec_pn16 value if present */
> +		if (status->counters.tx_lnk_sec_pn16)
> +			sec_pn16 = __le16_to_cpu(status->counters.tx_lnk_sec_pn16[i]);
> +		else
> +			sec_pn16 = 0;
> +		/* prevent wrap-around in pn16 counter */
> +		diff16 = (sec_pn16 - lnk->prev_sec_pn16) & 0xffff;
> +
> +		/* FIXME: since free_pkts is a 8-bit counter of packets that
> +		 * rolls over, it can become zero. If it is zero, then we
> +		 * omit processing below. Is that really correct?
> +		 */
> +		if (tx_lnk_free_pkts <= 0)
>    			continue;
>    
The original code was
         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;

I wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? 
This is monotonously incremented counter so 0 is not significant, unlike 
the diff.
Have I missed something?

Michael.

Re: [EXTERNAL] [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Russell King (Oracle)]

On Thu, May 30, 2024 at 11:06:40AM +0300, Nemanov, Michael wrote:
> 
> On 5/28/2024 12:18 PM, Russell King (Oracle) wrote:
> 
> [...]
> 
> >    static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
> >    {
> > +	struct wl12xx_vif *wlvifsta;
> > +	struct wl12xx_vif *wlvifap;
> >    	struct wl12xx_vif *wlvif;
> >    	u32 old_tx_blk_count = wl->tx_blocks_available;
> >    	int avail, freed_blocks;
> > @@ -410,23 +412,100 @@ static int wlcore_fw_status(struct wl1271 *wl, struct wl_fw_status *status)
> >    		wl->tx_pkts_freed[i] = status->counters.tx_released_pkts[i];
> >    	}
> [...]
> >    	for_each_set_bit(i, wl->links_map, wl->num_links) {
> > +		u16 diff16, sec_pn16;
> >    		u8 diff, tx_lnk_free_pkts;
> > +
> >    		lnk = &wl->links[i];
> >    		/* prevent wrap-around in freed-packets counter */
> >    		tx_lnk_free_pkts = status->counters.tx_lnk_free_pkts[i];
> >    		diff = (tx_lnk_free_pkts - lnk->prev_freed_pkts) & 0xff;
> > -		if (diff == 0)
> > +		if (diff) {
> > +			lnk->allocated_pkts -= diff;
> > +			lnk->prev_freed_pkts = tx_lnk_free_pkts;
> > +		}
> > +
> > +		/* Get the current sec_pn16 value if present */
> > +		if (status->counters.tx_lnk_sec_pn16)
> > +			sec_pn16 = __le16_to_cpu(status->counters.tx_lnk_sec_pn16[i]);
> > +		else
> > +			sec_pn16 = 0;
> > +		/* prevent wrap-around in pn16 counter */
> > +		diff16 = (sec_pn16 - lnk->prev_sec_pn16) & 0xffff;
> > +
> > +		/* FIXME: since free_pkts is a 8-bit counter of packets that
> > +		 * rolls over, it can become zero. If it is zero, then we
> > +		 * omit processing below. Is that really correct?
> > +		 */
> > +		if (tx_lnk_free_pkts <= 0)
> >    			continue;
> The original code was
>         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;
> 
> I wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? This is
> monotonously incremented counter so 0 is not significant, unlike the diff.
> Have I missed something?

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!

Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Nemanov, Michael]

On 5/30/2024 11:20 AM, Russell King (Oracle) wrote:
[...]
> > The original code was > 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; > > I 
> wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? 
> This is > monotonously incremented counter so 0 is not significant, 
> unlike the diff. > Have I missed something? 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) ) ... + } }

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

> 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.

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.

Re: [PATCH wireless-next 6/8] wifi: wlcore: add pn16 support

[Nemanov, Michael]

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

> 
> On 5/30/2024 11:20 AM, Russell King (Oracle) wrote:
> [...]
>> > The original code was > 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; > > I 
>> wonder if comparing tx_lnk_free_pkts to 0 was added intentionally? 
>> This is > monotonously incremented counter so 0 is not significant, 
>> unlike the diff. > Have I missed something? You are... While you're 

[...]

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

Michael.

[PATCH wireless-next 7/8] wifi: wl18xx: add support for reading 8.9.1 fw status

[Russell King (Oracle)]

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wl18xx/main.c   | 71 ++++++++++++++++++++++++-
 drivers/net/wireless/ti/wl18xx/wl18xx.h | 60 +++++++++++++++++++++
 2 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 2ccac1cdec01..39d8eebb9b6e 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -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;
 }
 
diff --git a/drivers/net/wireless/ti/wl18xx/wl18xx.h b/drivers/net/wireless/ti/wl18xx/wl18xx.h
index b642e0c437bb..7fed96d71b27 100644
--- a/drivers/net/wireless/ti/wl18xx/wl18xx.h
+++ b/drivers/net/wireless/ti/wl18xx/wl18xx.h
@@ -155,6 +155,66 @@ struct wl18xx_fw_status {
 	struct wl18xx_fw_status_priv priv;
 } __packed;
 
+struct wl18xx_fw_packet_counters_8_9_1 {
+	/* Cumulative counter of released packets per AC */
+	u8 tx_released_pkts[NUM_TX_QUEUES];
+
+	/* Cumulative counter of freed packets per HLID */
+	u8 tx_lnk_free_pkts[WL18XX_MAX_LINKS];
+
+	/* PN16 of last TKIP/AES seq-num per HLID */
+	__le16 tx_lnk_sec_pn16[WL18XX_MAX_LINKS];
+
+	/* Cumulative counter of released Voice memory blocks */
+	u8 tx_voice_released_blks;
+
+	/* Tx rate of the last transmitted packet */
+	u8 tx_last_rate;
+
+	/* Tx rate or Tx rate estimate pre-calculated by fw in mbps units */
+	u8 tx_last_rate_mbps;
+
+	/* hlid for which the rates were reported */
+	u8 hlid;
+} __packed;
+
+/* FW status registers */
+struct wl18xx_fw_status_8_9_1 {
+	__le32 intr;
+	u8  fw_rx_counter;
+	u8  drv_rx_counter;
+	u8  reserved;
+	u8  tx_results_counter;
+	__le32 rx_pkt_descs[WL18XX_NUM_RX_DESCRIPTORS];
+
+	__le32 fw_localtime;
+
+	/*
+	 * A bitmap (where each bit represents a single HLID)
+	 * to indicate if the station is in PS mode.
+	 */
+	__le32 link_ps_bitmap;
+
+	/*
+	 * A bitmap (where each bit represents a single HLID) to indicate
+	 * if the station is in Fast mode
+	 */
+	__le32 link_fast_bitmap;
+
+	/* Cumulative counter of total released mem blocks since FW-reset */
+	__le32 total_released_blks;
+
+	/* Size (in Memory Blocks) of TX pool */
+	__le32 tx_total;
+
+	struct wl18xx_fw_packet_counters_8_9_1 counters;
+
+	__le32 log_start_addr;
+
+	/* Private status to be used by the lower drivers */
+	struct wl18xx_fw_status_priv priv;
+} __packed;
+
 #define WL18XX_PHY_VERSION_MAX_LEN 20
 
 struct wl18xx_static_data_priv {
-- 
2.30.2

[PATCH wireless-next 8/8] wifi: wl18xx: allow firmwares > 8.9.0.x.58

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wl18xx/wl18xx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl18xx/wl18xx.h b/drivers/net/wireless/ti/wl18xx/wl18xx.h
index 7fed96d71b27..de6c671c4be6 100644
--- a/drivers/net/wireless/ti/wl18xx/wl18xx.h
+++ b/drivers/net/wireless/ti/wl18xx/wl18xx.h
@@ -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