| Message ID | E1sBsy7-00E8vu-Nc@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bb8edd900fd69e2d9865e29e4682e5289e8a2345 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: TI wilink8 updates | expand |
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.
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.
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.
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 --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];
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(-)