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!
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
---
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
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) <[email protected]>
> ---
> 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.
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!
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.
--
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:
[...]
> > 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.