diff mbox series

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

Message ID E1sBsy7-00E8vu-Nc@rmk-PC.armlinux.org.uk
State New
Headers show
Series wifi: TI wilink8 updates | expand

Commit Message

Russell King (Oracle) May 28, 2024, 9:18 a.m. UTC
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(-)

Comments

Nemanov, Michael May 30, 2024, 12:25 p.m. UTC | #1
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.
Nemanov, Michael May 30, 2024, 12:46 p.m. UTC | #2
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.
diff mbox series

Patch

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];