mt76: add hash lookup for skb on TXS status_list

From: Ben Greear <[email protected]>

This will let us update stats and wcid.rate for every TXS callback we receive for a particular wcid.

For now, the TXS is not requested for NO_SKB frames, however. That will be allowed in next patch.

Signed-off-by Ben Greear <[email protected]>

---
 .../net/wireless/mediatek/mt76/mt7915/mac.c    | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

drivers/net/wireless/mediatek/mt76/mt7915/mac.c +0 -0

index 6f92e207680f..2228dad71657 100644

From: Ben Greear <[email protected]>

This lets user turn on/off this feature. Enabling gives better tx-rate related stats, but will cause extra driver and (maybe) firmware work. Not sure if it actually affects performance or not.

Signed-off-by Ben Greear <[email protected]>

---
 .../wireless/mediatek/mt76/mt7915/debugfs.c   | 24 +++++++++++++++++++
 .../net/wireless/mediatek/mt76/mt7915/mac.c   |  3 ++-
 .../wireless/mediatek/mt76/mt7915/mt7915.h    |  5 ++++
 3 files changed, 31 insertions(+), 1 deletion(-)

drivers/net/wireless/mediatek/mt76/mt7915/debugfs.c +0 -0

index 91664ac63a8d..6be194f16548 100644

From: Ben Greear <[email protected]>

This lets user turn on/off this feature. Enabling gives better tx-rate related stats, but will cause extra driver and (maybe) firmware work. Not sure if it actually affects performance or not.

Signed-off-by Ben Greear <[email protected]>

Quoted 7 lines · [email protected] writes:

From: Ben Greear <[email protected]>

This lets user turn on/off this feature. Enabling gives better tx-rate related stats, but will cause extra driver and (maybe) firmware work. Not sure if it actually affects performance or not.

Quoted 1 line

Signed-off-by Ben Greear <[email protected]>

This is grey area, debugfs is not really meant to be used for users enabling driver features.

Quoted 9 lines · Kalle Valo

Quoted 7 lines · [email protected] writes:

From: Ben Greear <[email protected]>

This lets user turn on/off this feature. Enabling gives better tx-rate related stats, but will cause extra driver and (maybe) firmware work. Not sure if it actually affects performance or not.

Quoted 5 lines

Quoted 1 line

Signed-off-by Ben Greear <[email protected]>

This is grey area, debugfs is not really meant to be used for users enabling driver features.

What method do you suggest? Surely not trying to drive something down through netlink for something this chipset specific?

Quoted 10 lines · Ben Greear

Quoted 9 lines · Kalle Valo

Quoted 7 lines · [email protected] writes:

From: Ben Greear <[email protected]>

This lets user turn on/off this feature. Enabling gives better tx-rate related stats, but will cause extra driver and (maybe) firmware work. Not sure if it actually affects performance or not.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Signed-off-by Ben Greear <[email protected]>

This is grey area, debugfs is not really meant to be used for users enabling driver features.

What method do you suggest? Surely not trying to drive something down through netlink for something this chipset specific?

I think a module parameter would be a good choise for enabling this kind of feature.

Of course the downside of a module parater is that when it's not possible to configure this per device, only per driver. Like discussed many times in the past, we would really need some kind more advanced module parameters which can be per device.

From: Ben Greear <[email protected]>

Allow setting fix rate on transmit without using full testmode logic.

txpower, dynbw, retry count is not currently supported. And, probably later need additional logic to not apply this txo to non-data frames and to smaller frames, to allow ARP and such to go through while also forcing test data frames to arbitrary tx-rates (rates which very well may not be received by peer.)

Quoted 4 lines · [email protected] writes:

From: Ben Greear <[email protected]>

Allow setting fix rate on transmit without using full testmode logic.

Why?

Quoted 7 lines

txpower, dynbw, retry count is not currently supported. And, probably later need additional logic to not apply this txo to non-data frames and to smaller frames, to allow ARP and such to go through while also forcing test data frames to arbitrary tx-rates (rates which very well may not be received by peer.)

Please include an example how to use the debugfs file.

IIRC there was a similar rtw88 patch adding a debugfs to force setting the tx rate. So what's the consensus, are we going to allow each driver have their own custom interfaces setting tx rates? In my opinion this should go via a generic nl80211 command, but if people think having custom tx rate interfaces is ok I guess I need to reconsider.

As this patch needs more discussion, please separate it from rest of the series so that it can be applied separately.

From: Ben Greear <[email protected]>

This improves performance when sending lots of frames that are requesting being mapped to a TXS callback.

Add comments to help next person understood the tx path better.

Signed-off-by Ben Greear <[email protected]>

---

v5: Rebased on top of previous series.

 drivers/net/wireless/mediatek/mt76/mt76.h     | 48 +++++++---
 .../net/wireless/mediatek/mt76/mt7603/mac.c   |  2 +-
 .../net/wireless/mediatek/mt76/mt7615/mac.c   |  2 +-
 .../net/wireless/mediatek/mt76/mt76x02_mac.c  |  2 +-
 .../net/wireless/mediatek/mt76/mt7915/mac.c   |  8 +-
 .../net/wireless/mediatek/mt76/mt7921/mac.c   |  9 +-
 drivers/net/wireless/mediatek/mt76/tx.c       | 90 ++++++++++++++++---
 7 files changed, 132 insertions(+), 29 deletions(-)

drivers/net/wireless/mediatek/mt76/mt76.h +8 -0

index 436bf2b8e2cd..016f563fec39 100644
@@ -235,6 +235,14 @@ DECLARE_EWMA(signal, 10, 8);
 #define MT_WCID_TX_INFO_TXPWR_ADJ	GENMASK(25, 18)
 #define MT_WCID_TX_INFO_SET		BIT(31)
 
+#define MT_PACKET_ID_MASK		GENMASK(6, 0)
+#define MT_PACKET_ID_NO_ACK		0
+/* Request TXS, but don't try to match with skb. */
+#define MT_PACKET_ID_NO_SKB		1
+#define MT_PACKET_ID_FIRST		2
+#define MT_PACKET_ID_HAS_RATE		BIT(7)
+#define MT_PACKET_ID_MAX		(GENMASK(7, 0) - 1)
+
 struct mt76_wcid {
 	struct mt76_rx_tid __rcu *aggr[IEEE80211_NUM_TIDS];
 

drivers/net/wireless/mediatek/mt76/mt76.h +0 -0

 	u16 idx;
 	u8 hw_key_idx;
 	u8 hw_key_idx2;

@@ -302,13 +312,8 @@ struct mt76_rx_tid {
 #define MT_TX_CB_TXS_DONE		BIT(1)
 #define MT_TX_CB_TXS_FAILED		BIT(2)

drivers/net/wireless/mediatek/mt76/mt76.h +13 -3

@@ -1297,13 +1303,33 @@ mt76_token_put(struct mt76_dev *dev, int token)
 }
 
 static inline int
-mt76_get_next_pkt_id(struct mt76_wcid *wcid)
+mt76_get_next_pkt_id(struct mt76_dev *dev, struct mt76_wcid *wcid,
+		     struct sk_buff *skb)
 {
+	struct sk_buff *qskb;
+
+	lockdep_assert_held(&dev->status_list.lock);
+
 	wcid->packet_id = (wcid->packet_id + 1) & MT_PACKET_ID_MASK;
-	if (wcid->packet_id == MT_PACKET_ID_NO_ACK ||
-	    wcid->packet_id == MT_PACKET_ID_NO_SKB)
+	if (wcid->packet_id < MT_PACKET_ID_FIRST)
 		wcid->packet_id = MT_PACKET_ID_FIRST;
 
+	qskb = wcid->skb_status_array[wcid->packet_id];
+	if (qskb) {
+		/* bummer, already waiting on this pid.  See if it is stale. */
+		struct mt76_tx_cb *cb = mt76_tx_skb_cb(qskb);
+
+		if (!time_after(jiffies, cb->jiffies + MT_TX_STATUS_SKB_TIMEOUT)) {

drivers/net/wireless/mediatek/mt76/tx.c +2 -0

index 6f302acb6e69..4c8504d3c904 100644
@@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
 			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
 		return MT_PACKET_ID_NO_SKB;
 
+	/* due to limited range of the pktid (7 bits), we can only
+	 * have a limited number of outstanding frames.  I think it is OK to

Quoted 43 lines · On 2021-08-04 15:44, [email protected] wrote:

From: Ben Greear <[email protected]>

This improves performance when sending lots of frames that are requesting being mapped to a TXS callback.

Add comments to help next person understood the tx path better.

Signed-off-by Ben Greear <[email protected]>

---

v5: Rebased on top of previous series.

  drivers/net/wireless/mediatek/mt76/mt76.h     | 48 +++++++---
  .../net/wireless/mediatek/mt76/mt7603/mac.c   |  2 +-
  .../net/wireless/mediatek/mt76/mt7615/mac.c   |  2 +-
  .../net/wireless/mediatek/mt76/mt76x02_mac.c  |  2 +-
  .../net/wireless/mediatek/mt76/mt7915/mac.c   |  8 +-
  .../net/wireless/mediatek/mt76/mt7921/mac.c   |  9 +-
  drivers/net/wireless/mediatek/mt76/tx.c       | 90 ++++++++++++++++---
  7 files changed, 132 insertions(+), 29 deletions(-)

drivers/net/wireless/mediatek/mt76/mt76.h +8 -0

index 436bf2b8e2cd..016f563fec39 100644
@@ -235,6 +235,14 @@ DECLARE_EWMA(signal, 10, 8);
  #define MT_WCID_TX_INFO_TXPWR_ADJ	GENMASK(25, 18)
  #define MT_WCID_TX_INFO_SET		BIT(31)
 
+#define MT_PACKET_ID_MASK		GENMASK(6, 0)
+#define MT_PACKET_ID_NO_ACK		0
+/* Request TXS, but don't try to match with skb. */
+#define MT_PACKET_ID_NO_SKB		1
+#define MT_PACKET_ID_FIRST		2
+#define MT_PACKET_ID_HAS_RATE		BIT(7)
+#define MT_PACKET_ID_MAX		(GENMASK(7, 0) - 1)
+
  struct mt76_wcid {
  	struct mt76_rx_tid __rcu *aggr[IEEE80211_NUM_TIDS];
 

@@ -246,6 +254,8 @@ struct mt76_wcid {
 

drivers/net/wireless/mediatek/mt76/mt76.h struct mt76_wcid { · line 257

Quoted 3 lines

drivers/net/wireless/mediatek/mt76/mt76.h +1 -0

  	struct rate_info rate;
 
+	struct sk_buff *skb_status_array[MT_PACKET_ID_MAX + 1];

You could add this to reduce the struct size:

#define MT_NUM_STATUS_PACKETS \
	(MT_PACKET_ID_MAX + 1 - MT_PACKET_ID_FIRST)

And then subtract MT_PACKET_ID_FIRST from cache entries.

Quoted 7 lines

drivers/net/wireless/mediatek/mt76/mt76.h +0 -0

  	u16 idx;
  	u8 hw_key_idx;
  	u8 hw_key_idx2;

@@ -302,13 +312,8 @@ struct mt76_rx_tid {
  #define MT_TX_CB_TXS_DONE		BIT(1)
  #define MT_TX_CB_TXS_FAILED		BIT(2)
 

drivers/net/wireless/mediatek/mt76/mt76.h struct mt76_rx_tid { · line 316

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/mt76.h +2 -7

-#define MT_PACKET_ID_MASK		GENMASK(6, 0)
-#define MT_PACKET_ID_NO_ACK		0
-#define MT_PACKET_ID_NO_SKB		1
-#define MT_PACKET_ID_FIRST		2
-#define MT_PACKET_ID_HAS_RATE		BIT(7)
-
-#define MT_TX_STATUS_SKB_TIMEOUT	HZ
+/* This is timer for when to give up when waiting for TXS callback. */
+#define MT_TX_STATUS_SKB_TIMEOUT	(HZ / 8)

I think the way timeouts are checked now, HZ/8 is way too short. I would recommend checking timeout only for packets where MT_TX_CB_DMA_DONE is already set, and setting cb->jiffies from within __mt76_tx_status_skb_done on DMA completion. That should make it possible to keep the timeout short without running into it in cases where significant congestion adds huge completion latency.

Quoted 25 lines

drivers/net/wireless/mediatek/mt76/mt76.h +13 -3

@@ -1297,13 +1303,33 @@ mt76_token_put(struct mt76_dev *dev, int token)
  }
 
  static inline int
-mt76_get_next_pkt_id(struct mt76_wcid *wcid)
+mt76_get_next_pkt_id(struct mt76_dev *dev, struct mt76_wcid *wcid,
+		     struct sk_buff *skb)
  {
+	struct sk_buff *qskb;
+
+	lockdep_assert_held(&dev->status_list.lock);
+
  	wcid->packet_id = (wcid->packet_id + 1) & MT_PACKET_ID_MASK;
-	if (wcid->packet_id == MT_PACKET_ID_NO_ACK ||
-	    wcid->packet_id == MT_PACKET_ID_NO_SKB)
+	if (wcid->packet_id < MT_PACKET_ID_FIRST)
  		wcid->packet_id = MT_PACKET_ID_FIRST;
 
+	qskb = wcid->skb_status_array[wcid->packet_id];
+	if (qskb) {
+		/* bummer, already waiting on this pid.  See if it is stale. */
+		struct mt76_tx_cb *cb = mt76_tx_skb_cb(qskb);
+
+		if (!time_after(jiffies, cb->jiffies + MT_TX_STATUS_SKB_TIMEOUT)) {

drivers/net/wireless/mediatek/mt76/mt76.h mt76_token_put(struct mt76_dev *dev, int token) · line 1332

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/mt76.h +10 -0

+			/* ok, not stale.  Increment pid anyway, will try next
+			 * slot next time
+			 */
+			return MT_PACKET_ID_NO_SKB;
+		}
+	}
+
+	/* cache this skb for fast lookup by packet-id */
+	wcid->skb_status_array[wcid->packet_id] = skb;
+

I think mt76_get_next_pkt_id is not a good place for caching the skb. Better cache it in the same place that also puts the skb in the status
list: mt76_tx_status_skb_add

That way you can drop your (possibly broken) changes to mt7921, which calls mt76_get_next_pkt_id directly, but does not support tx status tracking for skbs.

drivers/net/wireless/mediatek/mt76/mt7915/mac.c mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid, · line 1322

Quoted 11 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c +2 -0

index d9f52e2611a7..8f5702981900 100644
@@ -1318,6 +1318,8 @@ mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid,
 
  	mt76_tx_status_lock(mdev, &list);
  	skb = mt76_tx_status_skb_get(mdev, wcid, pid, &list);
+
+	/* TODO:  Gather stats anyway, even if we are not matching on an skb. */

Please drop this comment, since you're deleting in another patch in this series anyway.

drivers/net/wireless/mediatek/mt76/mt7915/mac.c mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid, · line 1426

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c +4 -0

@@ -1417,10 +1419,14 @@ mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid,
  		stats->tx_bw[0]++;
  		break;
  	}
+
+	/* Cache rate for packets that don't get a TXS callback for some
+	 * reason.
+	 */
  	wcid->rate = rate;

That comment is wrong, wcid->rate is cached because HE rates can't be reported via skb->cb due to lack of space.

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/tx.c +0 -0

index 6f302acb6e69..4c8504d3c904 100644
@@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
  			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
  		return MT_PACKET_ID_NO_SKB;
 

drivers/net/wireless/mediatek/mt76/tx.c mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid, · line 168

Quoted 12 lines

drivers/net/wireless/mediatek/mt76/tx.c +12 -0

+	/* due to limited range of the pktid (7 bits), we can only
+	 * have a limited number of outstanding frames.  I think it is OK to
+	 * check the length outside of a lock since it doesn't matter too much
+	 * if we read wrong data here.
+	 * The TX-status callbacks don't always return a callback for an SKB,
+	 * so the status_list may contain some stale skbs.  Those will be cleaned
+	 * out periodically, see MT_TX_STATUS_SKB_TIMEOUT.
+	 */
+
+	qlen = skb_queue_len(&dev->status_list);
+	if (qlen > 120)
+		return MT_PACKET_ID_NO_SKB;

Checking the length of the per-device status list doesn't make sense, since pktid allocation is per-wcid.

- Felix

Quoted 52 lines · Felix Fietkau

Quoted 42 lines · On 2021-08-04 15:44, [email protected] wrote:

From: Ben Greear <[email protected]>

This improves performance when sending lots of frames that are requesting being mapped to a TXS callback.

Add comments to help next person understood the tx path better.

Signed-off-by Ben Greear <[email protected]>

---

v5: Rebased on top of previous series.

  drivers/net/wireless/mediatek/mt76/mt76.h     | 48 +++++++---
  .../net/wireless/mediatek/mt76/mt7603/mac.c   |  2 +-
  .../net/wireless/mediatek/mt76/mt7615/mac.c   |  2 +-
  .../net/wireless/mediatek/mt76/mt76x02_mac.c  |  2 +-
  .../net/wireless/mediatek/mt76/mt7915/mac.c   |  8 +-
  .../net/wireless/mediatek/mt76/mt7921/mac.c   |  9 +-
  drivers/net/wireless/mediatek/mt76/tx.c       | 90 ++++++++++++++++---
  7 files changed, 132 insertions(+), 29 deletions(-)

drivers/net/wireless/mediatek/mt76/mt76.h +8 -0

index 436bf2b8e2cd..016f563fec39 100644
@@ -235,6 +235,14 @@ DECLARE_EWMA(signal, 10, 8);
  #define MT_WCID_TX_INFO_TXPWR_ADJ	GENMASK(25, 18)
  #define MT_WCID_TX_INFO_SET		BIT(31)
 
+#define MT_PACKET_ID_MASK		GENMASK(6, 0)
+#define MT_PACKET_ID_NO_ACK		0
+/* Request TXS, but don't try to match with skb. */
+#define MT_PACKET_ID_NO_SKB		1
+#define MT_PACKET_ID_FIRST		2
+#define MT_PACKET_ID_HAS_RATE		BIT(7)
+#define MT_PACKET_ID_MAX		(GENMASK(7, 0) - 1)
+
  struct mt76_wcid {
  	struct mt76_rx_tid __rcu *aggr[IEEE80211_NUM_TIDS];
 

drivers/net/wireless/mediatek/mt76/mt76.h struct mt76_wcid { · line 257

Quoted 4 lines

drivers/net/wireless/mediatek/mt76/mt76.h +1 -0

@@ -246,6 +254,8 @@ struct mt76_wcid {
 
  	struct rate_info rate;
 
+	struct sk_buff *skb_status_array[MT_PACKET_ID_MAX + 1];

You could add this to reduce the struct size:

#define MT_NUM_STATUS_PACKETS \
	(MT_PACKET_ID_MAX + 1 - MT_PACKET_ID_FIRST)

Quoted 1 line

And then subtract MT_PACKET_ID_FIRST from cache entries.

That saves two void* bytes of memory, and complicates the code a bit? I can do the change, just doesn't seem worthwhile to me.

Quoted 7 lines

Quoted 7 lines

drivers/net/wireless/mediatek/mt76/mt76.h +0 -0

  	u16 idx;
  	u8 hw_key_idx;
  	u8 hw_key_idx2;

@@ -302,13 +312,8 @@ struct mt76_rx_tid {
  #define MT_TX_CB_TXS_DONE		BIT(1)
  #define MT_TX_CB_TXS_FAILED		BIT(2)
 

Quoted 15 lines

drivers/net/wireless/mediatek/mt76/mt76.h struct mt76_rx_tid { · line 316

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/mt76.h +2 -7

-#define MT_PACKET_ID_MASK		GENMASK(6, 0)
-#define MT_PACKET_ID_NO_ACK		0
-#define MT_PACKET_ID_NO_SKB		1
-#define MT_PACKET_ID_FIRST		2
-#define MT_PACKET_ID_HAS_RATE		BIT(7)
-
-#define MT_TX_STATUS_SKB_TIMEOUT	HZ
+/* This is timer for when to give up when waiting for TXS callback. */
+#define MT_TX_STATUS_SKB_TIMEOUT	(HZ / 8)

I think the way timeouts are checked now, HZ/8 is way too short. I would recommend checking timeout only for packets where MT_TX_CB_DMA_DONE is already set, and setting cb->jiffies from within __mt76_tx_status_skb_done on DMA completion. That should make it possible to keep the timeout short without running into it in cases where significant congestion adds huge completion latency.

Ok, I like that idea. What is reasonable timeout from time of DMA done before we give up on TXS callback?

Quoted 13 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid, · line 1322

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c +2 -0

index d9f52e2611a7..8f5702981900 100644
@@ -1318,6 +1318,8 @@ mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid,
 
  	mt76_tx_status_lock(mdev, &list);
  	skb = mt76_tx_status_skb_get(mdev, wcid, pid, &list);
+
+	/* TODO:  Gather stats anyway, even if we are not matching on an skb. */

Please drop this comment, since you're deleting in another patch in this series anyway.

Quoted 11 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid, · line 1426

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/mt7915/mac.c +4 -0

@@ -1417,10 +1419,14 @@ mt7915_mac_add_txs_skb(struct mt7915_dev *dev, struct mt76_wcid *wcid, int pid,
  		stats->tx_bw[0]++;
  		break;
  	}
+
+	/* Cache rate for packets that don't get a TXS callback for some
+	 * reason.
+	 */
  	wcid->rate = rate;

That comment is wrong, wcid->rate is cached because HE rates can't be reported via skb->cb due to lack of space.

We can update the rate from txs callback, and and from txfree path, and also from querying the firmware rate-ctrl registers (I think?). TXS is disabled for most frames by default. txfree gives only some info, not enough. And polling rate-ctrl registers is slow.

So I think the comment is OK, but I end up modifying the code later anyway, so I can remove this comment if you prefer.

Quoted 8 lines

Quoted 8 lines

drivers/net/wireless/mediatek/mt76/tx.c +0 -0

index 6f302acb6e69..4c8504d3c904 100644
@@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
  			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
  		return MT_PACKET_ID_NO_SKB;
 

Quoted 14 lines

drivers/net/wireless/mediatek/mt76/tx.c mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid, · line 168

Quoted 12 lines

drivers/net/wireless/mediatek/mt76/tx.c +12 -0

+	/* due to limited range of the pktid (7 bits), we can only
+	 * have a limited number of outstanding frames.  I think it is OK to
+	 * check the length outside of a lock since it doesn't matter too much
+	 * if we read wrong data here.
+	 * The TX-status callbacks don't always return a callback for an SKB,
+	 * so the status_list may contain some stale skbs.  Those will be cleaned
+	 * out periodically, see MT_TX_STATUS_SKB_TIMEOUT.
+	 */
+
+	qlen = skb_queue_len(&dev->status_list);
+	if (qlen > 120)
+		return MT_PACKET_ID_NO_SKB;

Checking the length of the per-device status list doesn't make sense, since pktid allocation is per-wcid.

Ok, so just remove this code, or should I set some other higher limit to bound the list?

Quoted 23 lines

Quoted 22 lines

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/tx.c +2 -0

index 6f302acb6e69..4c8504d3c904 100644
@@ -130,15 +154,30 @@ mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid,
   			     IEEE80211_TX_CTL_RATE_CTRL_PROBE)))
   		return MT_PACKET_ID_NO_SKB;
 
+	/* due to limited range of the pktid (7 bits), we can only
+	 * have a limited number of outstanding frames.  I think it is OK to

drivers/net/wireless/mediatek/mt76/tx.c mt76_tx_status_skb_add(struct mt76_dev *dev, struct mt76_wcid *wcid, · line 168

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/tx.c +10 -0

+	 * check the length outside of a lock since it doesn't matter too much
+	 * if we read wrong data here.
+	 * The TX-status callbacks don't always return a callback for an SKB,
+	 * so the status_list may contain some stale skbs.  Those will be cleaned
+	 * out periodically, see MT_TX_STATUS_SKB_TIMEOUT.
+	 */
+
+	qlen = skb_queue_len(&dev->status_list);
+	if (qlen > 120)
+		return MT_PACKET_ID_NO_SKB;

Checking the length of the per-device status list doesn't make sense, since pktid allocation is per-wcid.

Quoted 2 lines

Ok, so just remove this code, or should I set some other higher limit to bound the list?

You could just check for a duplicate skb_status_array entry.

- Felix