[PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Fedor Pchelkin]

The first two patches concern memory leak issues found during testing.

The third and the fourth one do some extra small changes.

The other ones implement TX completion functionality missing for the USB
part of rtw89 driver, suggested by Bitterblue Smith [1].  This will allow
handling TX wait skbs and the ones flagged with IEEE80211_TX_CTL_REQ_TX_STATUS
correctly.

rtw89 has several ways of handling TX status report events.  The first one
is based on RPP feature which is used by PCIe HCI.  The other one depends
on firmware sending a corresponding C2H message, quite similar to what
rtw88 has.  RTL8851BU vendor driver [2] was taken for reference.

[1]: https://lore.kernel.org/linux-wireless/0cb4d19b-94c7-450e-ac56-8b0d4a1d889f@gmail.com/
[2]: https://github.com/fofajardo/rtl8851bu.git

Series has been tested to work with RTL8851BU (USB) and RTL8852BE (PCIe)
devices.


Changelog.

v2: - add new 3/7 and 4/7 patches prepared due feedback to previous comments
      or developed in process
    - further changelog below --- in the patches

v1: https://lore.kernel.org/linux-wireless/20250920132614.277719-1-pchelkin@ispras.ru/

Fedor Pchelkin (7):
  wifi: rtw89: usb: use common error path for skbs in
    rtw89_usb_rx_handler()
  wifi: rtw89: usb: fix leak in rtw89_usb_write_port()
  wifi: rtw89: usb: use ieee80211_free_txskb() where appropriate
  wifi: rtw89: refine rtw89_core_tx_wait_complete()
  wifi: rtw89: implement C2H TX report handler
  wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB
  wifi: rtw89: process TX wait skbs for USB via C2H handler

 drivers/net/wireless/realtek/rtw89/core.c | 41 ++++++++++++---
 drivers/net/wireless/realtek/rtw89/core.h | 45 +++++++++++++----
 drivers/net/wireless/realtek/rtw89/fw.h   | 14 ++++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 46 +++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  | 61 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/pci.c  |  2 +-
 drivers/net/wireless/realtek/rtw89/pci.h  |  4 --
 drivers/net/wireless/realtek/rtw89/txrx.h |  6 ++-
 drivers/net/wireless/realtek/rtw89/usb.c  | 41 +++++++++++----
 9 files changed, 228 insertions(+), 32 deletions(-)

-- 
2.51.0

[PATCH rtw-next v2 1/7] wifi: rtw89: usb: use common error path for skbs in rtw89_usb_rx_handler()

[Fedor Pchelkin]

Allow adding rx_skb to rx_free_queue for later reuse on the common error
handling path, otherwise free it.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

v2: - do goto 'free_or_reuse' (Ping-Ke)

 drivers/net/wireless/realtek/rtw89/usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 6cf89aee252e..e8e064cf7e0a 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -410,8 +410,7 @@ static void rtw89_usb_rx_handler(struct work_struct *work)
 
 		if (skb_queue_len(&rtwusb->rx_queue) >= RTW89_USB_MAX_RXQ_LEN) {
 			rtw89_warn(rtwdev, "rx_queue overflow\n");
-			dev_kfree_skb_any(rx_skb);
-			continue;
+			goto free_or_reuse;
 		}
 
 		memset(&desc_info, 0, sizeof(desc_info));
@@ -422,7 +421,7 @@ static void rtw89_usb_rx_handler(struct work_struct *work)
 			rtw89_debug(rtwdev, RTW89_DBG_HCI,
 				    "failed to allocate RX skb of size %u\n",
 				    desc_info.pkt_size);
-			continue;
+			goto free_or_reuse;
 		}
 
 		pkt_offset = desc_info.offset + desc_info.rxd_len;
@@ -432,6 +431,7 @@ static void rtw89_usb_rx_handler(struct work_struct *work)
 
 		rtw89_core_rx(rtwdev, &desc_info, skb);
 
+free_or_reuse:
 		if (skb_queue_len(&rtwusb->rx_free_queue) >= RTW89_USB_RX_SKB_NUM)
 			dev_kfree_skb_any(rx_skb);
 		else
-- 
2.51.0

[PATCH rtw-next v2 2/7] wifi: rtw89: usb: fix leak in rtw89_usb_write_port()

[Fedor Pchelkin]

When there is an attempt to write data and RTW89_FLAG_UNPLUGGED is set,
this means device is disconnected and no urb is submitted.  Return
appropriate error code to the caller to properly free the allocated
resources.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/usb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index e8e064cf7e0a..512a46dd9d06 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -256,7 +256,7 @@ static int rtw89_usb_write_port(struct rtw89_dev *rtwdev, u8 ch_dma,
 	int ret;
 
 	if (test_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags))
-		return 0;
+		return -ENODEV;
 
 	urb = usb_alloc_urb(0, GFP_ATOMIC);
 	if (!urb)
@@ -305,8 +305,9 @@ static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 		ret = rtw89_usb_write_port(rtwdev, txch, skb->data, skb->len,
 					   txcb);
 		if (ret) {
-			rtw89_err(rtwdev, "write port txch %d failed: %d\n",
-				  txch, ret);
+			if (ret != -ENODEV)
+				rtw89_err(rtwdev, "write port txch %d failed: %d\n",
+					  txch, ret);
 
 			skb_dequeue(&txcb->tx_ack_queue);
 			kfree(txcb);
-- 
2.51.0

[PATCH rtw-next v2 3/7] wifi: rtw89: usb: use ieee80211_free_txskb() where appropriate

[Fedor Pchelkin]

rtw89_usb_ops_tx_kick_off() may need to release skb if a failure occurs.
It operates mainly on skbs coming from the core wireless stack and the
ones containing firmware commands.

Use ieee80211_free_txskb() for the former case.

Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

 drivers/net/wireless/realtek/rtw89/usb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 512a46dd9d06..655e8437d62e 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -278,6 +278,15 @@ static int rtw89_usb_write_port(struct rtw89_dev *rtwdev, u8 ch_dma,
 	return ret;
 }
 
+static void rtw89_usb_tx_free_skb(struct rtw89_dev *rtwdev, u8 txch,
+				  struct sk_buff *skb)
+{
+	if (txch == RTW89_TXCH_CH12)
+		dev_kfree_skb_any(skb);
+	else
+		ieee80211_free_txskb(rtwdev->hw, skb);
+}
+
 static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 {
 	struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
@@ -292,7 +301,7 @@ static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 
 		txcb = kmalloc(sizeof(*txcb), GFP_ATOMIC);
 		if (!txcb) {
-			dev_kfree_skb_any(skb);
+			rtw89_usb_tx_free_skb(rtwdev, txch, skb);
 			continue;
 		}
 
@@ -311,7 +320,7 @@ static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 
 			skb_dequeue(&txcb->tx_ack_queue);
 			kfree(txcb);
-			dev_kfree_skb_any(skb);
+			rtw89_usb_tx_free_skb(rtwdev, txch, skb);
 		}
 	}
 }
-- 
2.51.0

[PATCH rtw-next v2 4/7] wifi: rtw89: refine rtw89_core_tx_wait_complete()

[Fedor Pchelkin]

Pass TX status value directly into rtw89_core_tx_wait_complete().  This
will make it a bit in sync with further patches and will give flexibility
in future work.  Also use scope based RCU locking which simplifies the
code of the function.

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/core.h | 20 ++++++++++----------
 drivers/net/wireless/realtek/rtw89/pci.c  |  2 +-
 drivers/net/wireless/realtek/rtw89/pci.h  |  4 ----
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 928c8c84c964..60e32894d8b4 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -3507,6 +3507,11 @@ struct rtw89_phy_rate_pattern {
 	bool enable;
 };
 
+#define RTW89_TX_DONE			0x0
+#define RTW89_TX_RETRY_LIMIT		0x1
+#define RTW89_TX_LIFE_TIME		0x2
+#define RTW89_TX_MACID_DROP		0x3
+
 #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
 struct rtw89_tx_wait_info {
 	struct rcu_head rcu_head;
@@ -7374,25 +7379,20 @@ static inline struct sk_buff *rtw89_alloc_skb_for_rx(struct rtw89_dev *rtwdev,
 
 static inline bool rtw89_core_tx_wait_complete(struct rtw89_dev *rtwdev,
 					       struct rtw89_tx_skb_data *skb_data,
-					       bool tx_done)
+					       u8 tx_status)
 {
 	struct rtw89_tx_wait_info *wait;
-	bool ret = false;
 
-	rcu_read_lock();
+	guard(rcu)();
 
 	wait = rcu_dereference(skb_data->wait);
 	if (!wait)
-		goto out;
+		return false;
 
-	ret = true;
-	wait->tx_done = tx_done;
+	wait->tx_done = tx_status == RTW89_TX_DONE;
 	/* Don't access skb anymore after completion */
 	complete_all(&wait->completion);
-
-out:
-	rcu_read_unlock();
-	return ret;
+	return true;
 }
 
 static inline bool rtw89_is_mlo_1_1(struct rtw89_dev *rtwdev)
diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index 0ee5f8579447..b1985193a18f 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -464,7 +464,7 @@ static void rtw89_pci_tx_status(struct rtw89_dev *rtwdev,
 	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
 	struct ieee80211_tx_info *info;
 
-	if (rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status == RTW89_TX_DONE))
+	if (rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
 		return;
 
 	info = IEEE80211_SKB_CB(skb);
diff --git a/drivers/net/wireless/realtek/rtw89/pci.h b/drivers/net/wireless/realtek/rtw89/pci.h
index cb05c83dfd56..16dfb0e79d77 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.h
+++ b/drivers/net/wireless/realtek/rtw89/pci.h
@@ -1487,10 +1487,6 @@ struct rtw89_pci_tx_addr_info_32_v1 {
 #define RTW89_PCI_RPP_POLLUTED		BIT(31)
 #define RTW89_PCI_RPP_SEQ		GENMASK(30, 16)
 #define RTW89_PCI_RPP_TX_STATUS		GENMASK(15, 13)
-#define RTW89_TX_DONE			0x0
-#define RTW89_TX_RETRY_LIMIT		0x1
-#define RTW89_TX_LIFE_TIME		0x2
-#define RTW89_TX_MACID_DROP		0x3
 #define RTW89_PCI_RPP_QSEL		GENMASK(12, 8)
 #define RTW89_PCI_RPP_MACID		GENMASK(7, 0)
 
-- 
2.51.0

[PATCH rtw-next v2 5/7] wifi: rtw89: implement C2H TX report handler

[Fedor Pchelkin]

rtw89 has several ways of handling TX status report events.  The first one
is based on RPP feature which is used by PCIe HCI.  The other one depends
on firmware sending a corresponding C2H message, quite similar to what
rtw88 has.

Toggle a bit in the TX descriptor and place skb in a queue to wait for a
message from the firmware.  rtw89 has an extra feature providing TX
reports for multiple retry transmission attempts.  When there is a failed
TX status reported by the firmware, the report is ignored until the limit
is reached or success status appears.  Do all this according to the vendor
driver for RTL8851BU.

It seems the only way to implement TX status reporting for rtw89 USB.
This will allow handling TX wait skbs and the ones flagged with
IEEE80211_TX_CTL_REQ_TX_STATUS correctly.

Found by Linux Verification Center (linuxtesting.org).

Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

v2: - fix bit masks and consider TX transmission retry limit for TX
      reporting (Bitterblue)
    - use newer style for C2H message type definitions and drop
      unimplemented funcs from 'enum rtw89_mac_c2h_misc_func' (Ping-Ke)
    - modify rtw89_core_fill_txdesc_v1() and rtw89_core_fill_txdesc_v2()
      accordingly (Ping-Ke)
      
      Well, it took me awhile to figure out how those V0, V1 and V2 parts
      are structured in rtw89/txrx.h and match them with definitions from
      the vendor driver.  Probably BE_TXD_* section should be marked as V2
      by a comment..

      Anyway, I've decided to activate TX report bits for V1 and V2 as well
      though can't experiment with them, firmware on my side follows V0.

 drivers/net/wireless/realtek/rtw89/core.c | 28 +++++++++++++++++++----
 drivers/net/wireless/realtek/rtw89/core.h |  4 ++++
 drivers/net/wireless/realtek/rtw89/fw.h   | 14 ++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 26 +++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  |  7 ++++++
 drivers/net/wireless/realtek/rtw89/txrx.h |  6 ++++-
 6 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 917b2adede61..49ecc248464b 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1396,7 +1396,10 @@ static __le32 rtw89_build_txwd_info1(struct rtw89_tx_desc_info *desc_info)
 	u32 dword = FIELD_PREP(RTW89_TXWD_INFO1_MAX_AGGNUM, desc_info->ampdu_num) |
 		    FIELD_PREP(RTW89_TXWD_INFO1_A_CTRL_BSR, desc_info->a_ctrl_bsr) |
 		    FIELD_PREP(RTW89_TXWD_INFO1_DATA_RTY_LOWEST_RATE,
-			       desc_info->data_retry_lowest_rate);
+			       desc_info->data_retry_lowest_rate) |
+		    FIELD_PREP(RTW89_TXWD_INFO1_DATA_TXCNT_LMT_SEL,
+			       desc_info->tx_cnt_lmt_en) |
+		    FIELD_PREP(RTW89_TXWD_INFO1_DATA_TXCNT_LMT, desc_info->tx_cnt_lmt);
 
 	return cpu_to_le32(dword);
 }
@@ -1420,11 +1423,19 @@ static __le32 rtw89_build_txwd_info2_v1(struct rtw89_tx_desc_info *desc_info)
 	return cpu_to_le32(dword);
 }
 
+static __le32 rtw89_build_txwd_info3(struct rtw89_tx_desc_info *desc_info)
+{
+	u32 dword = FIELD_PREP(RTW89_TXWD_INFO3_SPE_RPT, desc_info->report);
+
+	return cpu_to_le32(dword);
+}
+
 static __le32 rtw89_build_txwd_info4(struct rtw89_tx_desc_info *desc_info)
 {
 	bool rts_en = !desc_info->is_bmc;
 	u32 dword = FIELD_PREP(RTW89_TXWD_INFO4_RTS_EN, rts_en) |
-		    FIELD_PREP(RTW89_TXWD_INFO4_HW_RTS_EN, 1);
+		    FIELD_PREP(RTW89_TXWD_INFO4_HW_RTS_EN, 1) |
+		    FIELD_PREP(RTW89_TXWD_INFO4_SW_DEFINE, desc_info->sn);
 
 	return cpu_to_le32(dword);
 }
@@ -1447,6 +1458,7 @@ void rtw89_core_fill_txdesc(struct rtw89_dev *rtwdev,
 	txwd_info->dword0 = rtw89_build_txwd_info0(desc_info);
 	txwd_info->dword1 = rtw89_build_txwd_info1(desc_info);
 	txwd_info->dword2 = rtw89_build_txwd_info2(desc_info);
+	txwd_info->dword3 = rtw89_build_txwd_info3(desc_info);
 	txwd_info->dword4 = rtw89_build_txwd_info4(desc_info);
 
 }
@@ -1476,6 +1488,7 @@ void rtw89_core_fill_txdesc_v1(struct rtw89_dev *rtwdev,
 	txwd_info->dword0 = rtw89_build_txwd_info0_v1(desc_info);
 	txwd_info->dword1 = rtw89_build_txwd_info1(desc_info);
 	txwd_info->dword2 = rtw89_build_txwd_info2_v1(desc_info);
+	txwd_info->dword3 = rtw89_build_txwd_info3(desc_info);
 	txwd_info->dword4 = rtw89_build_txwd_info4(desc_info);
 }
 EXPORT_SYMBOL(rtw89_core_fill_txdesc_v1);
@@ -1561,7 +1574,10 @@ static __le32 rtw89_build_txwd_info0_v2(struct rtw89_tx_desc_info *desc_info)
 	u32 dword = FIELD_PREP(BE_TXD_INFO0_DATA_STBC, desc_info->stbc) |
 		    FIELD_PREP(BE_TXD_INFO0_DATA_LDPC, desc_info->ldpc) |
 		    FIELD_PREP(BE_TXD_INFO0_DISDATAFB, desc_info->dis_data_fb) |
-		    FIELD_PREP(BE_TXD_INFO0_MULTIPORT_ID, desc_info->port);
+		    FIELD_PREP(BE_TXD_INFO0_MULTIPORT_ID, desc_info->port) |
+		    FIELD_PREP(BE_TXD_INFO0_DATA_TXCNT_LMT_SEL,
+			       desc_info->tx_cnt_lmt_en) |
+		    FIELD_PREP(BE_TXD_INFO0_DATA_TXCNT_LMT, desc_info->tx_cnt_lmt);
 
 	return cpu_to_le32(dword);
 }
@@ -1571,7 +1587,8 @@ static __le32 rtw89_build_txwd_info1_v2(struct rtw89_tx_desc_info *desc_info)
 	u32 dword = FIELD_PREP(BE_TXD_INFO1_MAX_AGG_NUM, desc_info->ampdu_num) |
 		    FIELD_PREP(BE_TXD_INFO1_A_CTRL_BSR, desc_info->a_ctrl_bsr) |
 		    FIELD_PREP(BE_TXD_INFO1_DATA_RTY_LOWEST_RATE,
-			       desc_info->data_retry_lowest_rate);
+			       desc_info->data_retry_lowest_rate) |
+		    FIELD_PREP(BE_TXD_INFO1_SW_DEFINE, desc_info->sn);
 
 	return cpu_to_le32(dword);
 }
@@ -1580,7 +1597,8 @@ static __le32 rtw89_build_txwd_info2_v2(struct rtw89_tx_desc_info *desc_info)
 {
 	u32 dword = FIELD_PREP(BE_TXD_INFO2_AMPDU_DENSITY, desc_info->ampdu_density) |
 		    FIELD_PREP(BE_TXD_INFO2_FORCE_KEY_EN, desc_info->sec_en) |
-		    FIELD_PREP(BE_TXD_INFO2_SEC_CAM_IDX, desc_info->sec_cam_idx);
+		    FIELD_PREP(BE_TXD_INFO2_SEC_CAM_IDX, desc_info->sec_cam_idx) |
+		    FIELD_PREP(BE_TXD_INFO2_SPE_RPT_V1, desc_info->report);
 
 	return cpu_to_le32(dword);
 }
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 60e32894d8b4..66b7bfa5902e 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -1167,6 +1167,10 @@ struct rtw89_tx_desc_info {
 	u8 ampdu_density;
 	u8 ampdu_num;
 	bool sec_en;
+	bool report;
+	bool tx_cnt_lmt_en;
+	u8 sn: 4;
+	u8 tx_cnt_lmt: 6;
 	u8 addr_info_nr;
 	u8 sec_keyid;
 	u8 sec_type;
diff --git a/drivers/net/wireless/realtek/rtw89/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h
index ddebf7972068..6d2f374244c4 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.h
+++ b/drivers/net/wireless/realtek/rtw89/fw.h
@@ -3747,6 +3747,20 @@ struct rtw89_c2h_scanofld {
 #define RTW89_GET_MAC_C2H_MCC_REQ_ACK_H2C_FUNC(c2h) \
 	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(15, 8))
 
+struct rtw89_c2h_mac_tx_rpt {
+	struct rtw89_c2h_hdr hdr;
+	__le32 w2;
+	__le32 w3;
+	__le32 w4;
+	__le32 w5;
+	__le32 w6;
+	__le32 w7;
+};
+
+#define RTW89_C2H_MAC_TX_RPT_W2_TX_STATE GENMASK(7, 6)
+#define RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE GENMASK(11, 8)
+#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT GENMASK(13, 8)
+
 struct rtw89_mac_mcc_tsf_rpt {
 	u32 macid_x;
 	u32 macid_y;
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index fd11b8fb3c89..10c2a39e544b 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5457,6 +5457,20 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
 	rtw89_complete_cond(&rtwdev->mcc.wait, cond, &data);
 }
 
+static void
+rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
+{
+	const struct rtw89_c2h_mac_tx_rpt *rpt =
+		(const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
+	u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
+	u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
+	u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
+
+	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+		    "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
+		    sw_define, tx_status, data_txcnt);
+}
+
 static void
 rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 {
@@ -5691,6 +5705,12 @@ void (* const rtw89_mac_c2h_mcc_handler[])(struct rtw89_dev *rtwdev,
 	[RTW89_MAC_C2H_FUNC_MCC_STATUS_RPT] = rtw89_mac_c2h_mcc_status_rpt,
 };
 
+static
+void (* const rtw89_mac_c2h_misc_handler[])(struct rtw89_dev *rtwdev,
+					    struct sk_buff *c2h, u32 len) = {
+	[RTW89_MAC_C2H_FUNC_TX_REPORT] = rtw89_mac_c2h_tx_rpt,
+};
+
 static
 void (* const rtw89_mac_c2h_mlo_handler[])(struct rtw89_dev *rtwdev,
 					   struct sk_buff *c2h, u32 len) = {
@@ -5777,6 +5797,8 @@ bool rtw89_mac_c2h_chk_atomic(struct rtw89_dev *rtwdev, struct sk_buff *c2h,
 		}
 	case RTW89_MAC_C2H_CLASS_MCC:
 		return true;
+	case RTW89_MAC_C2H_CLASS_MISC:
+		return true;
 	case RTW89_MAC_C2H_CLASS_MLO:
 		return true;
 	case RTW89_MAC_C2H_CLASS_MRC:
@@ -5812,6 +5834,10 @@ void rtw89_mac_c2h_handle(struct rtw89_dev *rtwdev, struct sk_buff *skb,
 		if (func < NUM_OF_RTW89_MAC_C2H_FUNC_MCC)
 			handler = rtw89_mac_c2h_mcc_handler[func];
 		break;
+	case RTW89_MAC_C2H_CLASS_MISC:
+		if (func < NUM_OF_RTW89_MAC_C2H_FUNC_MISC)
+			handler = rtw89_mac_c2h_misc_handler[func];
+		break;
 	case RTW89_MAC_C2H_CLASS_MLO:
 		if (func < NUM_OF_RTW89_MAC_C2H_FUNC_MLO)
 			handler = rtw89_mac_c2h_mlo_handler[func];
diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
index 25fe5e5c8a97..15c5c7e4033c 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -432,6 +432,12 @@ enum rtw89_mac_c2h_mcc_func {
 	NUM_OF_RTW89_MAC_C2H_FUNC_MCC,
 };
 
+enum rtw89_mac_c2h_misc_func {
+	RTW89_MAC_C2H_FUNC_TX_REPORT = 1,
+
+	NUM_OF_RTW89_MAC_C2H_FUNC_MISC,
+};
+
 enum rtw89_mac_c2h_mlo_func {
 	RTW89_MAC_C2H_FUNC_MLO_GET_TBL			= 0x0,
 	RTW89_MAC_C2H_FUNC_MLO_EMLSR_TRANS_DONE		= 0x1,
@@ -470,6 +476,7 @@ enum rtw89_mac_c2h_class {
 	RTW89_MAC_C2H_CLASS_WOW = 0x3,
 	RTW89_MAC_C2H_CLASS_MCC = 0x4,
 	RTW89_MAC_C2H_CLASS_FWDBG = 0x5,
+	RTW89_MAC_C2H_CLASS_MISC = 0x9,
 	RTW89_MAC_C2H_CLASS_MLO = 0xc,
 	RTW89_MAC_C2H_CLASS_MRC = 0xe,
 	RTW89_MAC_C2H_CLASS_AP = 0x18,
diff --git a/drivers/net/wireless/realtek/rtw89/txrx.h b/drivers/net/wireless/realtek/rtw89/txrx.h
index 984c9fdbb018..b37dbac7b790 100644
--- a/drivers/net/wireless/realtek/rtw89/txrx.h
+++ b/drivers/net/wireless/realtek/rtw89/txrx.h
@@ -127,6 +127,8 @@ static inline u8 rtw89_get_data_nss(struct rtw89_dev *rtwdev, u16 hw_rate)
 #define RTW89_TXWD_INFO0_MULTIPORT_ID GENMASK(6, 4)
 
 /* TX WD INFO DWORD 1 */
+#define RTW89_TXWD_INFO1_DATA_TXCNT_LMT_SEL BIT(31)
+#define RTW89_TXWD_INFO1_DATA_TXCNT_LMT GENMASK(30, 25)
 #define RTW89_TXWD_INFO1_DATA_RTY_LOWEST_RATE GENMASK(24, 16)
 #define RTW89_TXWD_INFO1_A_CTRL_BSR BIT(14)
 #define RTW89_TXWD_INFO1_MAX_AGGNUM GENMASK(7, 0)
@@ -139,10 +141,12 @@ static inline u8 rtw89_get_data_nss(struct rtw89_dev *rtwdev, u16 hw_rate)
 #define RTW89_TXWD_INFO2_SEC_CAM_IDX GENMASK(7, 0)
 
 /* TX WD INFO DWORD 3 */
+#define RTW89_TXWD_INFO3_SPE_RPT BIT(10)
 
 /* TX WD INFO DWORD 4 */
-#define RTW89_TXWD_INFO4_RTS_EN BIT(27)
 #define RTW89_TXWD_INFO4_HW_RTS_EN BIT(31)
+#define RTW89_TXWD_INFO4_RTS_EN BIT(27)
+#define RTW89_TXWD_INFO4_SW_DEFINE GENMASK(3, 0)
 
 /* TX WD INFO DWORD 5 */
 
-- 
2.51.0

[PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Fedor Pchelkin]

Frames flagged with IEEE80211_TX_CTL_REQ_TX_STATUS mean the driver has to
report to mac80211 stack whether AP sent ACK for the null frame/probe
request or not.  It's not implemented in USB part of the driver yet.

PCIe HCI has its own way of getting TX status incorporated into RPP
feature, and it's always enabled there.  Other HCIs need a different
scheme based on processing C2H messages.

Thus define a .tx_rpt_enable flag defining which HCIs would need a TX
report feature.  Currently it is USB only.

Do skb handling / queueing part quite similar to what rtw88 has.  Store
the flagged skbs in a queue for further processing in a C2H handler.
Flush the queue on HCI reset and on timeout via delayed work handler used
for TX waits - it's convenient since the further changes will pass TX wait
skbs management to the same TX report queue.

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

v2: - update TX rpt description in rtw89_core_tx_update_desc_info()
    - add a flag in rtw89_hci_info to determine if we should enable TX report (Ping-Ke)
    - refine rtw89_tx_rpt_queue_purge() - it's called on USB HCI reset
      and at rtw89_tx_wait_work.  Queueing delayed rtw89_tx_wait_work may be
      suboptimal but basically it's what rtw88 does with timer stuff.
      We can drop it and rely only on HCI reset to free remaining buffers
      in case firmware didn't send any TX status report.  I'd like to know
      your thoughts on this.

 drivers/net/wireless/realtek/rtw89/core.c |  9 ++++-
 drivers/net/wireless/realtek/rtw89/core.h |  6 ++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 19 ++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  | 43 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/usb.c  | 14 +++++++-
 5 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 49ecc248464b..214924f8bee0 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1107,6 +1107,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 						 tx_req->rtwsta_link);
 		if (addr_cam->valid && desc_info->mlo)
 			upd_wlan_hdr = true;
+
+		if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+			rtw89_tx_rpt_enable(rtwdev, tx_req);
 	}
 	is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
 		  is_multicast_ether_addr(hdr->addr1));
@@ -1140,7 +1143,10 @@ static void rtw89_tx_wait_work(struct wiphy *wiphy, struct wiphy_work *work)
 	struct rtw89_dev *rtwdev = container_of(work, struct rtw89_dev,
 						tx_wait_work.work);
 
-	rtw89_tx_wait_list_clear(rtwdev);
+	if (!rtwdev->hci.tx_rpt_enable)
+		rtw89_tx_wait_list_clear(rtwdev);
+	else
+		rtw89_tx_rpt_queue_purge(rtwdev);
 }
 
 void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
@@ -5847,6 +5853,7 @@ int rtw89_core_init(struct rtw89_dev *rtwdev)
 	wiphy_work_init(&rtwdev->cancel_6ghz_probe_work, rtw89_cancel_6ghz_probe_work);
 	INIT_WORK(&rtwdev->load_firmware_work, rtw89_load_firmware_work);
 
+	skb_queue_head_init(&rtwdev->tx_rpt_queue);
 	skb_queue_head_init(&rtwdev->c2h_queue);
 	rtw89_core_ppdu_sts_init(rtwdev);
 	rtw89_traffic_stats_init(rtwdev, &rtwdev->stats);
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 66b7bfa5902e..3940e54353d3 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -3527,6 +3527,8 @@ struct rtw89_tx_wait_info {
 
 struct rtw89_tx_skb_data {
 	struct rtw89_tx_wait_info __rcu *wait;
+	u8 tx_rpt_sn;
+	u8 tx_pkt_cnt_lmt;
 	u8 hci_priv[];
 };
 
@@ -3696,6 +3698,7 @@ struct rtw89_hci_info {
 	u32 rpwm_addr;
 	u32 cpwm_addr;
 	bool paused;
+	bool tx_rpt_enable;
 };
 
 struct rtw89_chip_ops {
@@ -6015,6 +6018,9 @@ struct rtw89_dev {
 	struct list_head tx_waits;
 	struct wiphy_delayed_work tx_wait_work;
 
+	atomic_t sn;
+	struct sk_buff_head tx_rpt_queue;
+
 	struct rtw89_cam_info cam_info;
 
 	struct sk_buff_head c2h_queue;
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 10c2a39e544b..75d9efac452b 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5465,10 +5465,29 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 	u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
 	u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
 	u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
+	struct rtw89_tx_skb_data *skb_data;
+	struct sk_buff *cur, *tmp;
+	unsigned long flags;
 
 	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
 		    "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
 		    sw_define, tx_status, data_txcnt);
+
+	spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
+	skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
+		skb_data = RTW89_TX_SKB_CB(cur);
+
+		if (sw_define != skb_data->tx_rpt_sn)
+			continue;
+		if (tx_status != RTW89_TX_DONE &&
+		    data_txcnt != skb_data->tx_pkt_cnt_lmt)
+			continue;
+
+		__skb_unlink(cur, &rtwdev->tx_rpt_queue);
+		rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
+		break;
+	}
+	spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
 }
 
 static void
diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
index 15c5c7e4033c..1f7d3734d15f 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -1616,4 +1616,47 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
 
 	return ret;
 }
+
+static inline
+void rtw89_tx_rpt_enable(struct rtw89_dev *rtwdev,
+			 struct rtw89_core_tx_request *tx_req)
+{
+	if (!rtwdev->hci.tx_rpt_enable)
+		return;
+
+	tx_req->desc_info.report = true;
+	tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
+	tx_req->desc_info.tx_cnt_lmt_en = true;
+	tx_req->desc_info.tx_cnt_lmt = 8;
+}
+
+static inline
+void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+	ieee80211_tx_info_clear_status(info);
+	if (tx_status == RTW89_TX_DONE)
+		info->flags |= IEEE80211_TX_STAT_ACK;
+	else
+		info->flags &= ~IEEE80211_TX_STAT_ACK;
+
+	ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
+}
+
+static inline
+void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
+{
+	struct sk_buff_head q;
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	__skb_queue_head_init(&q);
+	spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
+	skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
+	spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
+
+	while ((skb = __skb_dequeue(&q)))
+		rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
+}
 #endif
diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 655e8437d62e..f53ab676e9a8 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -216,6 +216,15 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
 		skb_pull(skb, txdesc_size);
 
 		info = IEEE80211_SKB_CB(skb);
+		if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
+			/* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
+			skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
+			wiphy_delayed_work_queue(rtwdev->hw->wiphy,
+						 &rtwdev->tx_wait_work,
+						 RTW89_TX_WAIT_WORK_TIMEOUT);
+			continue;
+		}
+
 		ieee80211_tx_info_clear_status(info);
 
 		if (urb->status == 0) {
@@ -396,6 +405,8 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
 	memset(txdesc, 0, txdesc_size);
 	rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
 
+	RTW89_TX_SKB_CB(skb)->tx_rpt_sn = tx_req->desc_info.sn;
+
 	le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
 
 	skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
@@ -678,7 +689,7 @@ static void rtw89_usb_deinit_tx(struct rtw89_dev *rtwdev)
 
 static void rtw89_usb_ops_reset(struct rtw89_dev *rtwdev)
 {
-	/* TODO: anything to do here? */
+	rtw89_tx_rpt_queue_purge(rtwdev);
 }
 
 static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
@@ -962,6 +973,7 @@ int rtw89_usb_probe(struct usb_interface *intf,
 
 	rtwdev->hci.ops = &rtw89_usb_ops;
 	rtwdev->hci.type = RTW89_HCI_TYPE_USB;
+	rtwdev->hci.tx_rpt_enable = true;
 
 	ret = rtw89_usb_intf_init(rtwdev, intf);
 	if (ret) {
-- 
2.51.0

[PATCH rtw-next v2 7/7] wifi: rtw89: process TX wait skbs for USB via C2H handler

[Fedor Pchelkin]

TX wait skbs need to be completed when they are done.  PCIe part does this
inside rtw89_pci_tx_status() during RPP processing.  Other HCIs use a
mechanism based on C2H firmware messages.

Store TX wait skbs inside TX report queue so that it'll be possible to
identify completed items inside C2H handler via private driver data of
skb.

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

v2: store TX wait skbs in tx_rpt_queue (Ping-Ke)

 drivers/net/wireless/realtek/rtw89/core.c |  6 ++++--
 drivers/net/wireless/realtek/rtw89/core.h | 15 +++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c  |  3 ++-
 drivers/net/wireless/realtek/rtw89/mac.h  | 15 +++++++++++++--
 drivers/net/wireless/realtek/rtw89/usb.c  |  3 ++-
 5 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 214924f8bee0..1457a5fe7320 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1108,7 +1108,7 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 		if (addr_cam->valid && desc_info->mlo)
 			upd_wlan_hdr = true;
 
-		if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+		if (tx_req->wait || (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS))
 			rtw89_tx_rpt_enable(rtwdev, tx_req);
 	}
 	is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
@@ -1173,7 +1173,8 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
 
 	if (time_left == 0) {
 		ret = -ETIMEDOUT;
-		list_add_tail(&wait->list, &rtwdev->tx_waits);
+		if (!rtwdev->hci.tx_rpt_enable)
+			list_add_tail(&wait->list, &rtwdev->tx_waits);
 		wiphy_delayed_work_queue(rtwdev->hw->wiphy, &rtwdev->tx_wait_work,
 					 RTW89_TX_WAIT_WORK_TIMEOUT);
 	} else {
@@ -1242,6 +1243,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
 	tx_req.skb = skb;
 	tx_req.vif = vif;
 	tx_req.sta = sta;
+	tx_req.wait = wait;
 	tx_req.rtwvif_link = rtwvif_link;
 	tx_req.rtwsta_link = rtwsta_link;
 	tx_req.desc_info.sw_mld = sw_mld;
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 3940e54353d3..c13465e2730a 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -1201,6 +1201,7 @@ struct rtw89_core_tx_request {
 	struct sk_buff *skb;
 	struct ieee80211_vif *vif;
 	struct ieee80211_sta *sta;
+	struct rtw89_tx_wait_info *wait;
 	struct rtw89_vif_link *rtwvif_link;
 	struct rtw89_sta_link *rtwsta_link;
 	struct rtw89_tx_desc_info desc_info;
@@ -7387,6 +7388,20 @@ static inline struct sk_buff *rtw89_alloc_skb_for_rx(struct rtw89_dev *rtwdev,
 	return dev_alloc_skb(length);
 }
 
+static inline bool rtw89_core_is_tx_wait(struct rtw89_dev *rtwdev,
+					 struct rtw89_tx_skb_data *skb_data)
+{
+	struct rtw89_tx_wait_info *wait;
+
+	guard(rcu)();
+
+	wait = rcu_dereference(skb_data->wait);
+	if (!wait)
+		return false;
+
+	return true;
+}
+
 static inline bool rtw89_core_tx_wait_complete(struct rtw89_dev *rtwdev,
 					       struct rtw89_tx_skb_data *skb_data,
 					       u8 tx_status)
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 75d9efac452b..3406c8b01eb8 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5484,7 +5484,8 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 			continue;
 
 		__skb_unlink(cur, &rtwdev->tx_rpt_queue);
-		rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
+		if (!rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
+			rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
 		break;
 	}
 	spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
index 1f7d3734d15f..2d647d3b0852 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -1647,16 +1647,27 @@ void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx
 static inline
 void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
 {
+	struct rtw89_tx_skb_data *skb_data;
+	struct rtw89_tx_wait_info *wait;
 	struct sk_buff_head q;
 	struct sk_buff *skb;
 	unsigned long flags;
 
+	lockdep_assert_wiphy(rtwdev->hw->wiphy);
+
 	__skb_queue_head_init(&q);
 	spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
 	skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
 	spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
 
-	while ((skb = __skb_dequeue(&q)))
-		rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
+	while ((skb = __skb_dequeue(&q))) {
+		skb_data = RTW89_TX_SKB_CB(skb);
+		wait = wiphy_dereference(rtwdev->hw->wiphy, skb_data->wait);
+
+		if (wait)
+			rtw89_tx_wait_release(wait);
+		else
+			rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
+	}
 }
 #endif
diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index f53ab676e9a8..adbadb2783f0 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -216,7 +216,8 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
 		skb_pull(skb, txdesc_size);
 
 		info = IEEE80211_SKB_CB(skb);
-		if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
+		if (rtw89_core_is_tx_wait(rtwdev, RTW89_TX_SKB_CB(skb)) ||
+		    (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)) {
 			/* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
 			skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
 			wiphy_delayed_work_queue(rtwdev->hw->wiphy,
-- 
2.51.0

RE: [PATCH rtw-next v2 7/7] wifi: rtw89: process TX wait skbs for USB via C2H handler

[Zong-Zhe Yang]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> 
> TX wait skbs need to be completed when they are done.  PCIe part does this inside
> rtw89_pci_tx_status() during RPP processing.  Other HCIs use a mechanism based on C2H
> firmware messages.
> 
> Store TX wait skbs inside TX report queue so that it'll be possible to identify completed items
> inside C2H handler via private driver data of skb.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> 
> v2: store TX wait skbs in tx_rpt_queue (Ping-Ke)
> 
>  drivers/net/wireless/realtek/rtw89/core.c |  6 ++++--
> drivers/net/wireless/realtek/rtw89/core.h | 15 +++++++++++++++
> drivers/net/wireless/realtek/rtw89/mac.c  |  3 ++-
> drivers/net/wireless/realtek/rtw89/mac.h  | 15 +++++++++++++--
> drivers/net/wireless/realtek/rtw89/usb.c  |  3 ++-
>  5 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> b/drivers/net/wireless/realtek/rtw89/core.c
> index 214924f8bee0..1457a5fe7320 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1108,7 +1108,7 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
>                 if (addr_cam->valid && desc_info->mlo)
>                         upd_wlan_hdr = true;
> 
> -               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> +               if (tx_req->wait || (info->flags &
> + IEEE80211_TX_CTL_REQ_TX_STATUS))
>                         rtw89_tx_rpt_enable(rtwdev, tx_req);
>         }
>         is_bmc = (is_broadcast_ether_addr(hdr->addr1) || @@ -1173,7 +1173,8 @@ int
> rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
> 
>         if (time_left == 0) {
>                 ret = -ETIMEDOUT;
> -               list_add_tail(&wait->list, &rtwdev->tx_waits);
> +               if (!rtwdev->hci.tx_rpt_enable)
> +                       list_add_tail(&wait->list, &rtwdev->tx_waits);
>                 wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> &rtwdev->tx_wait_work,
>                                          RTW89_TX_WAIT_WORK_TIMEOUT);
>         } else {
> @@ -1242,6 +1243,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
>         tx_req.skb = skb;
>         tx_req.vif = vif;
>         tx_req.sta = sta;
> +       tx_req.wait = wait;
>         tx_req.rtwvif_link = rtwvif_link;
>         tx_req.rtwsta_link = rtwsta_link;
>         tx_req.desc_info.sw_mld = sw_mld; diff --git
> a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 3940e54353d3..c13465e2730a 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -1201,6 +1201,7 @@ struct rtw89_core_tx_request {
>         struct sk_buff *skb;
>         struct ieee80211_vif *vif;
>         struct ieee80211_sta *sta;
> +       struct rtw89_tx_wait_info *wait;
>         struct rtw89_vif_link *rtwvif_link;
>         struct rtw89_sta_link *rtwsta_link;
>         struct rtw89_tx_desc_info desc_info; @@ -7387,6 +7388,20 @@ static inline struct
> sk_buff *rtw89_alloc_skb_for_rx(struct rtw89_dev *rtwdev,
>         return dev_alloc_skb(length);
>  }
> 
> +static inline bool rtw89_core_is_tx_wait(struct rtw89_dev *rtwdev,
> +                                        struct rtw89_tx_skb_data
> +*skb_data) {
> +       struct rtw89_tx_wait_info *wait;
> +
> +       guard(rcu)();
> +
> +       wait = rcu_dereference(skb_data->wait);
> +       if (!wait)
> +               return false;
> +
> +       return true;
> +}
> +

Look like using rcu_access_pointer() is enough for here.

>  static inline bool rtw89_core_tx_wait_complete(struct rtw89_dev *rtwdev,
>                                                struct rtw89_tx_skb_data
> *skb_data,
>                                                u8 tx_status) diff --git
> a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 75d9efac452b..3406c8b01eb8 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5484,7 +5484,8 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff
> *c2h, u32 len)
>                         continue;
> 
>                 __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> -               rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
> +               if (!rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
> +                       rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
>                 break;
>         }
>         spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags); diff --git
> a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 1f7d3734d15f..2d647d3b0852 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1647,16 +1647,27 @@ void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct
> sk_buff *skb, u8 tx  static inline  void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
> {
> +       struct rtw89_tx_skb_data *skb_data;
> +       struct rtw89_tx_wait_info *wait;
>         struct sk_buff_head q;
>         struct sk_buff *skb;
>         unsigned long flags;
> 
> +       lockdep_assert_wiphy(rtwdev->hw->wiphy);
> +
>         __skb_queue_head_init(&q);
>         spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
>         skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
>         spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> 
> -       while ((skb = __skb_dequeue(&q)))
> -               rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +       while ((skb = __skb_dequeue(&q))) {
> +               skb_data = RTW89_TX_SKB_CB(skb);
> +               wait = wiphy_dereference(rtwdev->hw->wiphy,
> + skb_data->wait);
> +
> +               if (wait)
> +                       rtw89_tx_wait_release(wait);
> +               else
> +                       rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +       }
>  }
>  #endif
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c
> b/drivers/net/wireless/realtek/rtw89/usb.c
> index f53ab676e9a8..adbadb2783f0 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,7 +216,8 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 skb_pull(skb, txdesc_size);
> 
>                 info = IEEE80211_SKB_CB(skb);
> -               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> +               if (rtw89_core_is_tx_wait(rtwdev, RTW89_TX_SKB_CB(skb)) ||
> +                   (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)) {
>                         /* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
>                         skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
>                         wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> --
> 2.51.0
> 

Re: [PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Bitterblue Smith]

On 02/10/2025 23:08, Fedor Pchelkin wrote:
> The first two patches concern memory leak issues found during testing.
> 
> The third and the fourth one do some extra small changes.
> 
> The other ones implement TX completion functionality missing for the USB
> part of rtw89 driver, suggested by Bitterblue Smith [1].  This will allow
> handling TX wait skbs and the ones flagged with IEEE80211_TX_CTL_REQ_TX_STATUS
> correctly.
> 
> rtw89 has several ways of handling TX status report events.  The first one
> is based on RPP feature which is used by PCIe HCI.  The other one depends
> on firmware sending a corresponding C2H message, quite similar to what
> rtw88 has.  RTL8851BU vendor driver [2] was taken for reference.
> 
> [1]: https://lore.kernel.org/linux-wireless/0cb4d19b-94c7-450e-ac56-8b0d4a1d889f@gmail.com/
> [2]: https://github.com/fofajardo/rtl8851bu.git
> 
> Series has been tested to work with RTL8851BU (USB) and RTL8852BE (PCIe)
> devices.
> 
> 
> Changelog.
> 
> v2: - add new 3/7 and 4/7 patches prepared due feedback to previous comments
>       or developed in process
>     - further changelog below --- in the patches
> 
> v1: https://lore.kernel.org/linux-wireless/20250920132614.277719-1-pchelkin@ispras.ru/
> 
> Fedor Pchelkin (7):
>   wifi: rtw89: usb: use common error path for skbs in
>     rtw89_usb_rx_handler()
>   wifi: rtw89: usb: fix leak in rtw89_usb_write_port()
>   wifi: rtw89: usb: use ieee80211_free_txskb() where appropriate
>   wifi: rtw89: refine rtw89_core_tx_wait_complete()
>   wifi: rtw89: implement C2H TX report handler
>   wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB
>   wifi: rtw89: process TX wait skbs for USB via C2H handler
> 
>  drivers/net/wireless/realtek/rtw89/core.c | 41 ++++++++++++---
>  drivers/net/wireless/realtek/rtw89/core.h | 45 +++++++++++++----
>  drivers/net/wireless/realtek/rtw89/fw.h   | 14 ++++++
>  drivers/net/wireless/realtek/rtw89/mac.c  | 46 +++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.h  | 61 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/pci.c  |  2 +-
>  drivers/net/wireless/realtek/rtw89/pci.h  |  4 --
>  drivers/net/wireless/realtek/rtw89/txrx.h |  6 ++-
>  drivers/net/wireless/realtek/rtw89/usb.c  | 41 +++++++++++----
>  9 files changed, 228 insertions(+), 32 deletions(-)
> 

I tested these patches with RTL8851BU, RTL8832AU, RTL8832BU, RTL8832CU, and
RTL8912AU. They all work, with a few additions.

Before these patches RTL8851BU and RTL8832AU would remain "connected" when
I power off the router. That's because they don't have beacon filtering in
the firmware and the null frames sent by mac80211 were always marked with
IEEE80211_TX_STAT_ACK. With these patches they disconnect immediately when
I power off the router. So that works nicely.

What doesn't work is TX reports for management frames. Currently rtw89
doesn't configure the firmware to provide TX reports for the management
queue. That can be enabled with SET_CMC_TBL_MGQ_RPT_EN for the wifi 6 chips
and with CCTLINFO_G7_W0_MGQ_RPT_EN for RTL8922AU.

The other thing that doesn't work is the TX reports are different for
RTL8852CU and RTL8922AU. It's only a small difference for RTL8852CU:

#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)

RTL8922AU is more strange. It needs something like this:

#define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
#define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
#define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)

The C2H is 80 bytes here (header included).

I think that's everything.

RE: [PATCH rtw-next v2 1/7] wifi: rtw89: usb: use common error path for skbs in rtw89_usb_rx_handler()

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> Allow adding rx_skb to rx_free_queue for later reuse on the common error
> handling path, otherwise free it.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

RE: [PATCH rtw-next v2 3/7] wifi: rtw89: usb: use ieee80211_free_txskb() where appropriate

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> rtw89_usb_ops_tx_kick_off() may need to release skb if a failure occurs.
> It operates mainly on skbs coming from the core wireless stack and the
> ones containing firmware commands.
> 
> Use ieee80211_free_txskb() for the former case.
> 
> Suggested-by: Ping-Ke Shih <pkshih@realtek.com>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

RE: [PATCH rtw-next v2 4/7] wifi: rtw89: refine rtw89_core_tx_wait_complete()

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> Pass TX status value directly into rtw89_core_tx_wait_complete().  This
> will make it a bit in sync with further patches and will give flexibility
> in future work.  Also use scope based RCU locking which simplifies the
> code of the function.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

RE: [PATCH rtw-next v2 5/7] wifi: rtw89: implement C2H TX report handler

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> rtw89 has several ways of handling TX status report events.  The first one
> is based on RPP feature which is used by PCIe HCI.  The other one depends
> on firmware sending a corresponding C2H message, quite similar to what
> rtw88 has.
> 
> Toggle a bit in the TX descriptor and place skb in a queue to wait for a
> message from the firmware.  rtw89 has an extra feature providing TX
> reports for multiple retry transmission attempts.  When there is a failed
> TX status reported by the firmware, the report is ignored until the limit
> is reached or success status appears.  Do all this according to the vendor
> driver for RTL8851BU.
> 
> It seems the only way to implement TX status reporting for rtw89 USB.
> This will allow handling TX wait skbs and the ones flagged with
> IEEE80211_TX_CTL_REQ_TX_STATUS correctly.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index fd11b8fb3c89..10c2a39e544b 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,6 +5457,20 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
>         rtw89_complete_cond(&rtwdev->mcc.wait, cond, &data);
>  }
> 
> +static void
> +rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> +{
> +       const struct rtw89_c2h_mac_tx_rpt *rpt =
> +               (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
> +       u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
> +       u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
> +       u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);

Since we'd like reverse X'mas tree order, I'd like separate declarations and
assignments. Like

u8 sw_define, tx_status, data_txcnt;

sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);

Otherwise, looks good to me.

> +
> +       rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> +                   "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
> +                   sw_define, tx_status, data_txcnt);
> +}
> +

[...]

RE: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> Frames flagged with IEEE80211_TX_CTL_REQ_TX_STATUS mean the driver has to
> report to mac80211 stack whether AP sent ACK for the null frame/probe
> request or not.  It's not implemented in USB part of the driver yet.
> 
> PCIe HCI has its own way of getting TX status incorporated into RPP
> feature, and it's always enabled there.  Other HCIs need a different
> scheme based on processing C2H messages.
> 
> Thus define a .tx_rpt_enable flag defining which HCIs would need a TX
> report feature.  Currently it is USB only.
> 
> Do skb handling / queueing part quite similar to what rtw88 has.  Store
> the flagged skbs in a queue for further processing in a C2H handler.
> Flush the queue on HCI reset and on timeout via delayed work handler used
> for TX waits - it's convenient since the further changes will pass TX wait
> skbs management to the same TX report queue.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> 
> v2: - update TX rpt description in rtw89_core_tx_update_desc_info()
>     - add a flag in rtw89_hci_info to determine if we should enable TX report (Ping-Ke)
>     - refine rtw89_tx_rpt_queue_purge() - it's called on USB HCI reset
>       and at rtw89_tx_wait_work.  Queueing delayed rtw89_tx_wait_work may be
>       suboptimal but basically it's what rtw88 does with timer stuff.
>       We can drop it and rely only on HCI reset to free remaining buffers
>       in case firmware didn't send any TX status report.  I'd like to know
>       your thoughts on this.
> 
>  drivers/net/wireless/realtek/rtw89/core.c |  9 ++++-
>  drivers/net/wireless/realtek/rtw89/core.h |  6 ++++
>  drivers/net/wireless/realtek/rtw89/mac.c  | 19 ++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.h  | 43 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/usb.c  | 14 +++++++-
>  5 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 49ecc248464b..214924f8bee0 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1107,6 +1107,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
>                                                  tx_req->rtwsta_link);
>                 if (addr_cam->valid && desc_info->mlo)
>                         upd_wlan_hdr = true;
> +
> +               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> +                       rtw89_tx_rpt_enable(rtwdev, tx_req);

During review of previous round, I add a patch [1] internally to adjust
this function, because we only need to fill few fields for FWCMD type.

Please add my patch into your patchset before this patch.

[1] https://lore.kernel.org/linux-wireless/20251007032656.13189-1-pkshih@realtek.com/T/#u

>         }
>         is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
>                   is_multicast_ether_addr(hdr->addr1));
> @@ -1140,7 +1143,10 @@ static void rtw89_tx_wait_work(struct wiphy *wiphy, struct wiphy_work *work)
>         struct rtw89_dev *rtwdev = container_of(work, struct rtw89_dev,
>                                                 tx_wait_work.work);
> 
> -       rtw89_tx_wait_list_clear(rtwdev);
> +       if (!rtwdev->hci.tx_rpt_enable)
> +               rtw89_tx_wait_list_clear(rtwdev);
> +       else
> +               rtw89_tx_rpt_queue_purge(rtwdev);

I feel rtw89_tx_wait_work() should only free list of 'rtwdev->tx_waits'.
To mix TX report queue ('rtwdev->tx_rpt_queue') looks hard to understand.
A skb exists in two queues looks not easy to handle...

Also, I see rtw89_hci_reset() calls rtw89_tx_wait_list_clear(). Is it still
correct after this patch?

>  }
> 
>  void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
> @@ -5847,6 +5853,7 @@ int rtw89_core_init(struct rtw89_dev *rtwdev)
>         wiphy_work_init(&rtwdev->cancel_6ghz_probe_work, rtw89_cancel_6ghz_probe_work);
>         INIT_WORK(&rtwdev->load_firmware_work, rtw89_load_firmware_work);
> 
> +       skb_queue_head_init(&rtwdev->tx_rpt_queue);
>         skb_queue_head_init(&rtwdev->c2h_queue);
>         rtw89_core_ppdu_sts_init(rtwdev);
>         rtw89_traffic_stats_init(rtwdev, &rtwdev->stats);
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 66b7bfa5902e..3940e54353d3 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3527,6 +3527,8 @@ struct rtw89_tx_wait_info {
> 
>  struct rtw89_tx_skb_data {
>         struct rtw89_tx_wait_info __rcu *wait;
> +       u8 tx_rpt_sn;
> +       u8 tx_pkt_cnt_lmt;
>         u8 hci_priv[];
>  };
> 
> @@ -3696,6 +3698,7 @@ struct rtw89_hci_info {
>         u32 rpwm_addr;
>         u32 cpwm_addr;
>         bool paused;
> +       bool tx_rpt_enable;

tx_rpt_enabled?

>  };
> 
>  struct rtw89_chip_ops {
> @@ -6015,6 +6018,9 @@ struct rtw89_dev {
>         struct list_head tx_waits;
>         struct wiphy_delayed_work tx_wait_work;
> 
> +       atomic_t sn;
> +       struct sk_buff_head tx_rpt_queue;
> +
>         struct rtw89_cam_info cam_info;
> 
>         struct sk_buff_head c2h_queue;
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 10c2a39e544b..75d9efac452b 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5465,10 +5465,29 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>         u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
>         u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
>         u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> +       struct rtw89_tx_skb_data *skb_data;
> +       struct sk_buff *cur, *tmp;
> +       unsigned long flags;
> 
>         rtw89_debug(rtwdev, RTW89_DBG_TXRX,
>                     "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
>                     sw_define, tx_status, data_txcnt);
> +
> +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> +       skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> +               skb_data = RTW89_TX_SKB_CB(cur);

Prefer just 'skb' instead of 'cur'. 
I don't need look back to understand 'cur' is a sk_buff.

> +
> +               if (sw_define != skb_data->tx_rpt_sn)
> +                       continue;
> +               if (tx_status != RTW89_TX_DONE &&
> +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)

As commit message of previous patch, "When there is a failed
TX status reported by the firmware, the report is ignored until the limit
is reached or success status appears."

Do you still need to check data_txcnt for failed cases? 


> +                       continue;
> +
> +               __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> +               rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
> +               break;
> +       }
> +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
>  }
> 
>  static void
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 15c5c7e4033c..1f7d3734d15f 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1616,4 +1616,47 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
> 
>         return ret;
>  }
> +
> +static inline
> +void rtw89_tx_rpt_enable(struct rtw89_dev *rtwdev,
> +                        struct rtw89_core_tx_request *tx_req)
> +{
> +       if (!rtwdev->hci.tx_rpt_enable)
> +               return;
> +
> +       tx_req->desc_info.report = true;
> +       tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;

Since you have assigned 4 bits for 'sn', no need '& 0xF'. 
If you still want to emphasize this, maybe add a comment.

> +       tx_req->desc_info.tx_cnt_lmt_en = true;
> +       tx_req->desc_info.tx_cnt_lmt = 8;

Question about '8'. Can it be larger?

I mean if hardware retry over 8 (e.g. 10), and succeed to send at 9th retry.
Would it be a problem?


> +}
> +
> +static inline
> +void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
> +{
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> +       ieee80211_tx_info_clear_status(info);
> +       if (tx_status == RTW89_TX_DONE)
> +               info->flags |= IEEE80211_TX_STAT_ACK;
> +       else
> +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> +
> +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> +}
> +
> +static inline
> +void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
> +{
> +       struct sk_buff_head q;
> +       struct sk_buff *skb;
> +       unsigned long flags;
> +
> +       __skb_queue_head_init(&q);

With a blank line, the lock would be clear. 

> +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> +       skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
> +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> +
> +       while ((skb = __skb_dequeue(&q)))
> +               rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +}
>  #endif
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index 655e8437d62e..f53ab676e9a8 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,6 +216,15 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 skb_pull(skb, txdesc_size);
> 
>                 info = IEEE80211_SKB_CB(skb);
> +               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> +                       /* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
> +                       skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
> +                       wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> +                                                &rtwdev->tx_wait_work,
> +                                                RTW89_TX_WAIT_WORK_TIMEOUT);
> +                       continue;
> +               }
> +
>                 ieee80211_tx_info_clear_status(info);
> 
>                 if (urb->status == 0) {
> @@ -396,6 +405,8 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
>         memset(txdesc, 0, txdesc_size);
>         rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
> 
> +       RTW89_TX_SKB_CB(skb)->tx_rpt_sn = tx_req->desc_info.sn;

Prefer a local variable 'struct rtw89_tx_skb_data *skb_data;'.

> +
>         le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);

This is also to fill tx desc, so prefer filling tx_rpt_sn after this. 

> 
>         skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> @@ -678,7 +689,7 @@ static void rtw89_usb_deinit_tx(struct rtw89_dev *rtwdev)
> 
>  static void rtw89_usb_ops_reset(struct rtw89_dev *rtwdev)
>  {
> -       /* TODO: anything to do here? */
> +       rtw89_tx_rpt_queue_purge(rtwdev);

No sure if we can ensure no TX packets still on the fly --
which have been submitted, but not call rtw89_usb_write_port_complete() yet.

But I'm also not sure if it is a problem that newly TX completion happens
after calling rtw89_tx_rpt_queue_purge().

>  }
> 
>  static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> @@ -962,6 +973,7 @@ int rtw89_usb_probe(struct usb_interface *intf,
> 
>         rtwdev->hci.ops = &rtw89_usb_ops;
>         rtwdev->hci.type = RTW89_HCI_TYPE_USB;
> +       rtwdev->hci.tx_rpt_enable = true;
> 
>         ret = rtw89_usb_intf_init(rtwdev, intf);
>         if (ret) {
> --
> 2.51.0
> 

RE: [PATCH rtw-next v2 7/7] wifi: rtw89: process TX wait skbs for USB via C2H handler

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> TX wait skbs need to be completed when they are done.  PCIe part does this
> inside rtw89_pci_tx_status() during RPP processing.  Other HCIs use a
> mechanism based on C2H firmware messages.
> 
> Store TX wait skbs inside TX report queue so that it'll be possible to
> identify completed items inside C2H handler via private driver data of
> skb.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> 
> v2: store TX wait skbs in tx_rpt_queue (Ping-Ke)
> 
>  drivers/net/wireless/realtek/rtw89/core.c |  6 ++++--
>  drivers/net/wireless/realtek/rtw89/core.h | 15 +++++++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.c  |  3 ++-
>  drivers/net/wireless/realtek/rtw89/mac.h  | 15 +++++++++++++--
>  drivers/net/wireless/realtek/rtw89/usb.c  |  3 ++-
>  5 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 214924f8bee0..1457a5fe7320 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1108,7 +1108,7 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
>                 if (addr_cam->valid && desc_info->mlo)
>                         upd_wlan_hdr = true;
> 
> -               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> +               if (tx_req->wait || (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS))
>                         rtw89_tx_rpt_enable(rtwdev, tx_req);
>         }
>         is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
> @@ -1173,7 +1173,8 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
> 
>         if (time_left == 0) {
>                 ret = -ETIMEDOUT;
> -               list_add_tail(&wait->list, &rtwdev->tx_waits);
> +               if (!rtwdev->hci.tx_rpt_enable)
> +                       list_add_tail(&wait->list, &rtwdev->tx_waits);

Oh. You avoid using rtwdev->tx_waits for USB. But I'd like to have the same
behavior as PCIE.

        rtw89_pci_release_txwd_skb() -->
            rtw89_pci_tx_status() -->
		        rtw89_core_tx_wait_complete()
		        ieee80211_tx_status_ni()

    rtw89_usb_write_port_complete() (rtwdev->tx_rpt_queue) -->
        rtw89_mac_c2h_tx_rpt() -->
            rtw89_tx_rpt_tx_status() -->
                rtw89_core_tx_wait_complete()
                ieee80211_tx_status_irqsafe()


As noted by Zong-Zhe before, an important point is that rtwdev->hci.ops->reset()
must release all TX packets (rtwdev->tx_rpt_queue), and then it will be safe
to call rtw89_tx_wait_list_clear().

>                 wiphy_delayed_work_queue(rtwdev->hw->wiphy, &rtwdev->tx_wait_work,
>                                          RTW89_TX_WAIT_WORK_TIMEOUT);
>         } else {
> @@ -1242,6 +1243,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
>         tx_req.skb = skb;
>         tx_req.vif = vif;
>         tx_req.sta = sta;
> +       tx_req.wait = wait;

Can we move ' rcu_assign_pointer(skb_data->wait, wait);' here, and
callee use rtw89_core_is_tx_wait()? Then, no need rtw89_core_tx_request::wait.


>         tx_req.rtwvif_link = rtwvif_link;
>         tx_req.rtwsta_link = rtwsta_link;
>         tx_req.desc_info.sw_mld = sw_mld;
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 3940e54353d3..c13465e2730a 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -1201,6 +1201,7 @@ struct rtw89_core_tx_request {
>         struct sk_buff *skb;
>         struct ieee80211_vif *vif;
>         struct ieee80211_sta *sta;
> +       struct rtw89_tx_wait_info *wait;
>         struct rtw89_vif_link *rtwvif_link;
>         struct rtw89_sta_link *rtwsta_link;
>         struct rtw89_tx_desc_info desc_info;
> @@ -7387,6 +7388,20 @@ static inline struct sk_buff *rtw89_alloc_skb_for_rx(struct rtw89_dev *rtwdev,
>         return dev_alloc_skb(length);
>  }
> 
> +static inline bool rtw89_core_is_tx_wait(struct rtw89_dev *rtwdev,
> +                                        struct rtw89_tx_skb_data *skb_data)
> +{
> +       struct rtw89_tx_wait_info *wait;
> +
> +       guard(rcu)();
> +
> +       wait = rcu_dereference(skb_data->wait);
> +       if (!wait)
> +               return false;
> +
> +       return true;
> +}
> +
>  static inline bool rtw89_core_tx_wait_complete(struct rtw89_dev *rtwdev,
>                                                struct rtw89_tx_skb_data *skb_data,
>                                                u8 tx_status)
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 75d9efac452b..3406c8b01eb8 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5484,7 +5484,8 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>                         continue;
> 
>                 __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> -               rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
> +               if (!rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
> +                       rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);

Can't we use the same style as PCIE?

rtw89_pci_tx_status()
{
	if (rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
		return;

   ...
}

>                 break;
>         }
>         spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 1f7d3734d15f..2d647d3b0852 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1647,16 +1647,27 @@ void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx
>  static inline
>  void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
>  {
> +       struct rtw89_tx_skb_data *skb_data;
> +       struct rtw89_tx_wait_info *wait;
>         struct sk_buff_head q;
>         struct sk_buff *skb;
>         unsigned long flags;
> 
> +       lockdep_assert_wiphy(rtwdev->hw->wiphy);
> +
>         __skb_queue_head_init(&q);
>         spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
>         skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
>         spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> 
> -       while ((skb = __skb_dequeue(&q)))
> -               rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +       while ((skb = __skb_dequeue(&q))) {
> +               skb_data = RTW89_TX_SKB_CB(skb);
> +               wait = wiphy_dereference(rtwdev->hw->wiphy, skb_data->wait);
> +
> +               if (wait)
> +                       rtw89_tx_wait_release(wait);

As mentioned above, can we just use the same flow as PCIE. Here only call
rtw89_tx_rpt_tx_status(), and callers or rtwdev->tx_wait_work call
rtw89_tx_wait_release().

> +               else
> +                       rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +       }
>  }
>  #endif
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index f53ab676e9a8..adbadb2783f0 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,7 +216,8 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 skb_pull(skb, txdesc_size);
> 
>                 info = IEEE80211_SKB_CB(skb);
> -               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> +               if (rtw89_core_is_tx_wait(rtwdev, RTW89_TX_SKB_CB(skb)) ||
> +                   (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)) {
>                         /* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
>                         skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
>                         wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> --
> 2.51.0
> 

Re: [PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Fedor Pchelkin]

On Sat, 04. Oct 20:37, Bitterblue Smith wrote:
> I tested these patches with RTL8851BU, RTL8832AU, RTL8832BU, RTL8832CU, and
> RTL8912AU. They all work, with a few additions.
> 
> Before these patches RTL8851BU and RTL8832AU would remain "connected" when
> I power off the router. That's because they don't have beacon filtering in
> the firmware and the null frames sent by mac80211 were always marked with
> IEEE80211_TX_STAT_ACK. With these patches they disconnect immediately when
> I power off the router. So that works nicely.
> 

Glad to hear, thanks for the insight.

> What doesn't work is TX reports for management frames. Currently rtw89
> doesn't configure the firmware to provide TX reports for the management
> queue. That can be enabled with SET_CMC_TBL_MGQ_RPT_EN for the wifi 6 chips
> and with CCTLINFO_G7_W0_MGQ_RPT_EN for RTL8922AU.

I'll investigate. Looks like the enabling of the management part should go
to rtw89_fw_h2c_default_cmac_tbl().

Btw, could you give a quick hint please on how I can check during testing
that the reporting facility works properly for all cases needed?  By far
I've dealt with iw utility and debugging printks incorporated into rtw89
but it doesn't look sufficient anymore..

> 
> The other thing that doesn't work is the TX reports are different for
> RTL8852CU and RTL8922AU. It's only a small difference for RTL8852CU:
> 
> #define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
> 
> RTL8922AU is more strange. It needs something like this:
> 
> #define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
> #define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
> #define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)
> 
> The C2H is 80 bytes here (header included).

rtw89_mac_c2h_tx_rpt() needs to account for different types of C2H report
formats, bah.  Will add this missing part.

Re: [PATCH rtw-next v2 7/7] wifi: rtw89: process TX wait skbs for USB via C2H handler

[Fedor Pchelkin]

On Tue, 07. Oct 08:07, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > @@ -1173,7 +1173,8 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
> > 
> >         if (time_left == 0) {
> >                 ret = -ETIMEDOUT;
> > -               list_add_tail(&wait->list, &rtwdev->tx_waits);
> > +               if (!rtwdev->hci.tx_rpt_enable)
> > +                       list_add_tail(&wait->list, &rtwdev->tx_waits);
> 
> Oh. You avoid using rtwdev->tx_waits for USB. But I'd like to have the same
> behavior as PCIE.

I may be confused but doesn't it conflict with the comment [1] you've
posted to the previous version?  I've treated that as we should use
rtwdev->tx_rpt_queue for both TX wait and IEEE80211_TX_CTL_REQ_TX_STATUS
frames...

I'm all for following the PCIe-style as possible, too, but then initial
comment [1] becomes irrelevant, right?

[1]: https://lore.kernel.org/linux-wireless/c2c40bed311c4f05948cf2541c64ea30@realtek.com/

Re: [PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Bitterblue Smith]

On 11/10/2025 17:57, Fedor Pchelkin wrote:
> On Sat, 04. Oct 20:37, Bitterblue Smith wrote:
>> I tested these patches with RTL8851BU, RTL8832AU, RTL8832BU, RTL8832CU, and
>> RTL8912AU. They all work, with a few additions.
>>
>> Before these patches RTL8851BU and RTL8832AU would remain "connected" when
>> I power off the router. That's because they don't have beacon filtering in
>> the firmware and the null frames sent by mac80211 were always marked with
>> IEEE80211_TX_STAT_ACK. With these patches they disconnect immediately when
>> I power off the router. So that works nicely.
>>
> 
> Glad to hear, thanks for the insight.
> 
>> What doesn't work is TX reports for management frames. Currently rtw89
>> doesn't configure the firmware to provide TX reports for the management
>> queue. That can be enabled with SET_CMC_TBL_MGQ_RPT_EN for the wifi 6 chips
>> and with CCTLINFO_G7_W0_MGQ_RPT_EN for RTL8922AU.
> 
> I'll investigate. Looks like the enabling of the management part should go
> to rtw89_fw_h2c_default_cmac_tbl().
> 

Yes, and rtw89_fw_h2c_default_cmac_tbl_g7().

> Btw, could you give a quick hint please on how I can check during testing
> that the reporting facility works properly for all cases needed?  By far
> I've dealt with iw utility and debugging printks incorporated into rtw89
> but it doesn't look sufficient anymore..
> 

I enabled RTW89_DBG_TXRX, which let me see that no TX reports appeared
during authentication and association. I also added a printk where the
IEEE80211_TX_CTL_REQ_TX_STATUS flag is checked. Then I just use the
driver normally, with wpa_supplicant and NetworkManager.

>>
>> The other thing that doesn't work is the TX reports are different for
>> RTL8852CU and RTL8922AU. It's only a small difference for RTL8852CU:
>>
>> #define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
>>
>> RTL8922AU is more strange. It needs something like this:
>>
>> #define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
>> #define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
>> #define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)
>>
>> The C2H is 80 bytes here (header included).
> 
> rtw89_mac_c2h_tx_rpt() needs to account for different types of C2H report
> formats, bah.  Will add this missing part.

RE: [PATCH rtw-next v2 7/7] wifi: rtw89: process TX wait skbs for USB via C2H handler

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> On Tue, 07. Oct 08:07, Ping-Ke Shih wrote:
> > Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > @@ -1173,7 +1173,8 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
> > >
> > >         if (time_left == 0) {
> > >                 ret = -ETIMEDOUT;
> > > -               list_add_tail(&wait->list, &rtwdev->tx_waits);
> > > +               if (!rtwdev->hci.tx_rpt_enable)
> > > +                       list_add_tail(&wait->list, &rtwdev->tx_waits);
> >
> > Oh. You avoid using rtwdev->tx_waits for USB. But I'd like to have the same
> > behavior as PCIE.
> 
> I may be confused but doesn't it conflict with the comment [1] you've
> posted to the previous version?  I've treated that as we should use
> rtwdev->tx_rpt_queue for both TX wait and IEEE80211_TX_CTL_REQ_TX_STATUS
> frames...

Yes. I got this thought after reviewing whole v2.

> 
> I'm all for following the PCIe-style as possible, too, but then initial
> comment [1] becomes irrelevant, right?
> 
> [1]: https://lore.kernel.org/linux-wireless/c2c40bed311c4f05948cf2541c64ea30@realtek.com/

At the [1], I wanted to iterate skb's in rtwdev->tx_rpt_queue, and then call
rtw89_core_tx_wait_complete() if 'wait' existing in RTW89_TX_SKB_CB().

I think the idea is similar, but I might not have clear picture at v1 review.
If you feel they are conflict, just check and discuss with new comments.

Re: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Fedor Pchelkin]

Hi,

thanks for detailed review!

On Tue, 07. Oct 06:31, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > Frames flagged with IEEE80211_TX_CTL_REQ_TX_STATUS mean the driver has to
> > report to mac80211 stack whether AP sent ACK for the null frame/probe
> > request or not.  It's not implemented in USB part of the driver yet.
> > 
> > PCIe HCI has its own way of getting TX status incorporated into RPP
> > feature, and it's always enabled there.  Other HCIs need a different
> > scheme based on processing C2H messages.
> > 
> > Thus define a .tx_rpt_enable flag defining which HCIs would need a TX
> > report feature.  Currently it is USB only.
> > 
> > Do skb handling / queueing part quite similar to what rtw88 has.  Store
> > the flagged skbs in a queue for further processing in a C2H handler.
> > Flush the queue on HCI reset and on timeout via delayed work handler used
> > for TX waits - it's convenient since the further changes will pass TX wait
> > skbs management to the same TX report queue.
> > 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > ---
> > 
> > v2: - update TX rpt description in rtw89_core_tx_update_desc_info()
> >     - add a flag in rtw89_hci_info to determine if we should enable TX report (Ping-Ke)
> >     - refine rtw89_tx_rpt_queue_purge() - it's called on USB HCI reset
> >       and at rtw89_tx_wait_work.  Queueing delayed rtw89_tx_wait_work may be
> >       suboptimal but basically it's what rtw88 does with timer stuff.
> >       We can drop it and rely only on HCI reset to free remaining buffers
> >       in case firmware didn't send any TX status report.  I'd like to know
> >       your thoughts on this.
> > 
> >  drivers/net/wireless/realtek/rtw89/core.c |  9 ++++-
> >  drivers/net/wireless/realtek/rtw89/core.h |  6 ++++
> >  drivers/net/wireless/realtek/rtw89/mac.c  | 19 ++++++++++
> >  drivers/net/wireless/realtek/rtw89/mac.h  | 43 +++++++++++++++++++++++
> >  drivers/net/wireless/realtek/rtw89/usb.c  | 14 +++++++-
> >  5 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> > index 49ecc248464b..214924f8bee0 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1107,6 +1107,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
> >                                                  tx_req->rtwsta_link);
> >                 if (addr_cam->valid && desc_info->mlo)
> >                         upd_wlan_hdr = true;
> > +
> > +               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> > +                       rtw89_tx_rpt_enable(rtwdev, tx_req);
> 
> During review of previous round, I add a patch [1] internally to adjust
> this function, because we only need to fill few fields for FWCMD type.
> 
> Please add my patch into your patchset before this patch.
> 
> [1] https://lore.kernel.org/linux-wireless/20251007032656.13189-1-pkshih@realtek.com/T/#u

Okay.

> 
> >         }
> >         is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
> >                   is_multicast_ether_addr(hdr->addr1));
> > @@ -1140,7 +1143,10 @@ static void rtw89_tx_wait_work(struct wiphy *wiphy, struct wiphy_work *work)
> >         struct rtw89_dev *rtwdev = container_of(work, struct rtw89_dev,
> >                                                 tx_wait_work.work);
> > 
> > -       rtw89_tx_wait_list_clear(rtwdev);
> > +       if (!rtwdev->hci.tx_rpt_enable)
> > +               rtw89_tx_wait_list_clear(rtwdev);
> > +       else
> > +               rtw89_tx_rpt_queue_purge(rtwdev);
> 
> I feel rtw89_tx_wait_work() should only free list of 'rtwdev->tx_waits'.
> To mix TX report queue ('rtwdev->tx_rpt_queue') looks hard to understand.
> A skb exists in two queues looks not easy to handle...
> 
> Also, I see rtw89_hci_reset() calls rtw89_tx_wait_list_clear(). Is it still
> correct after this patch?

TX wait list is currently empty when TX reports are used but, yeah, this
code will most probably be reworked due to the need to separate tx_rpt_queue
and TX wait_list and unify PCIe and USB reset-handlers.  I'm pondering over
these patches and will send out v3 in a day or two.

> 
> >  }
> > 
> >  void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
> > @@ -5847,6 +5853,7 @@ int rtw89_core_init(struct rtw89_dev *rtwdev)
> >         wiphy_work_init(&rtwdev->cancel_6ghz_probe_work, rtw89_cancel_6ghz_probe_work);
> >         INIT_WORK(&rtwdev->load_firmware_work, rtw89_load_firmware_work);
> > 
> > +       skb_queue_head_init(&rtwdev->tx_rpt_queue);
> >         skb_queue_head_init(&rtwdev->c2h_queue);
> >         rtw89_core_ppdu_sts_init(rtwdev);
> >         rtw89_traffic_stats_init(rtwdev, &rtwdev->stats);
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> > index 66b7bfa5902e..3940e54353d3 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.h
> > +++ b/drivers/net/wireless/realtek/rtw89/core.h
> > @@ -3527,6 +3527,8 @@ struct rtw89_tx_wait_info {
> > 
> >  struct rtw89_tx_skb_data {
> >         struct rtw89_tx_wait_info __rcu *wait;
> > +       u8 tx_rpt_sn;
> > +       u8 tx_pkt_cnt_lmt;
> >         u8 hci_priv[];
> >  };
> > 
> > @@ -3696,6 +3698,7 @@ struct rtw89_hci_info {
> >         u32 rpwm_addr;
> >         u32 cpwm_addr;
> >         bool paused;
> > +       bool tx_rpt_enable;
> 
> tx_rpt_enabled?
> 
> >  };
> > 
> >  struct rtw89_chip_ops {
> > @@ -6015,6 +6018,9 @@ struct rtw89_dev {
> >         struct list_head tx_waits;
> >         struct wiphy_delayed_work tx_wait_work;
> > 
> > +       atomic_t sn;
> > +       struct sk_buff_head tx_rpt_queue;
> > +
> >         struct rtw89_cam_info cam_info;
> > 
> >         struct sk_buff_head c2h_queue;
> > diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> > index 10c2a39e544b..75d9efac452b 100644
> > --- a/drivers/net/wireless/realtek/rtw89/mac.c
> > +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> > @@ -5465,10 +5465,29 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> >         u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
> >         u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
> >         u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> > +       struct rtw89_tx_skb_data *skb_data;
> > +       struct sk_buff *cur, *tmp;
> > +       unsigned long flags;
> > 
> >         rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> >                     "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
> >                     sw_define, tx_status, data_txcnt);
> > +
> > +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> > +       skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> > +               skb_data = RTW89_TX_SKB_CB(cur);
> 
> Prefer just 'skb' instead of 'cur'. 
> I don't need look back to understand 'cur' is a sk_buff.
> 
> > +
> > +               if (sw_define != skb_data->tx_rpt_sn)
> > +                       continue;
> > +               if (tx_status != RTW89_TX_DONE &&
> > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> 
> As commit message of previous patch, "When there is a failed
> TX status reported by the firmware, the report is ignored until the limit
> is reached or success status appears."
> 
> Do you still need to check data_txcnt for failed cases? 

The question also concerns

  tx_req->desc_info.tx_cnt_lmt = 8;

line in rtw89_tx_rpt_enable().  'tx_cnt_lmt' is written to TX descriptor
and processed by firmware.  The value defines how many times the firmware
will retry transmission attempts, it will not retry more times than that.

'data_txcnt' C2H field determines the retry attempt counter for the frame
returned by the firmware.  If it reaches the limit, this means we got
the last report from the firmware and there would be no other firmware
reports for the sent frame.  So a final tx_status should be taken
uncondionally in this case.

E.g. if 'tx_cnt_lmt' is set to 1, the firmware will try only once,
'data_txcnt' will be 1, too.  The limit is reached and we should take
tx_status immediately as is.  So there's a higher chance of getting a
failed status eventually.

I set it currently to 8 as the vendor driver does.  In local testing it
looks more than enough.  I've seen maximum of 5 retry attempts for the
same frame (usually there are no retries at all) though my network radio
environment is quite noisy.

I'll add the tx_cnt_lmt related info to commit message for clarity.

> 
> 
> > +                       continue;
> > +
> > +               __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> > +               rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
> > +               break;
> > +       }
> > +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> >  }
> > 
> >  static void
> > diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> > index 15c5c7e4033c..1f7d3734d15f 100644
> > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > @@ -1616,4 +1616,47 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
> > 
> >         return ret;
> >  }
> > +
> > +static inline
> > +void rtw89_tx_rpt_enable(struct rtw89_dev *rtwdev,
> > +                        struct rtw89_core_tx_request *tx_req)
> > +{
> > +       if (!rtwdev->hci.tx_rpt_enable)
> > +               return;
> > +
> > +       tx_req->desc_info.report = true;
> > +       tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
> 
> Since you have assigned 4 bits for 'sn', no need '& 0xF'. 
> If you still want to emphasize this, maybe add a comment.
> 
> > +       tx_req->desc_info.tx_cnt_lmt_en = true;
> > +       tx_req->desc_info.tx_cnt_lmt = 8;
> 
> Question about '8'. Can it be larger?
> 
> I mean if hardware retry over 8 (e.g. 10), and succeed to send at 9th retry.
> Would it be a problem?
> 
> 
> > +}
> > +
> > +static inline
> > +void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
> > +{
> > +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> > +
> > +       ieee80211_tx_info_clear_status(info);
> > +       if (tx_status == RTW89_TX_DONE)
> > +               info->flags |= IEEE80211_TX_STAT_ACK;
> > +       else
> > +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> > +
> > +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> > +}
> > +
> > +static inline
> > +void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
> > +{
> > +       struct sk_buff_head q;
> > +       struct sk_buff *skb;
> > +       unsigned long flags;
> > +
> > +       __skb_queue_head_init(&q);
> 
> With a blank line, the lock would be clear. 
> 
> > +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> > +       skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
> > +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> > +
> > +       while ((skb = __skb_dequeue(&q)))
> > +               rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> > +}
> >  #endif
> > diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> > index 655e8437d62e..f53ab676e9a8 100644
> > --- a/drivers/net/wireless/realtek/rtw89/usb.c
> > +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> > @@ -216,6 +216,15 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
> >                 skb_pull(skb, txdesc_size);
> > 
> >                 info = IEEE80211_SKB_CB(skb);
> > +               if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> > +                       /* sn is passed to rtw89_mac_c2h_tx_rpt() via driver data */
> > +                       skb_queue_tail(&rtwdev->tx_rpt_queue, skb);
> > +                       wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> > +                                                &rtwdev->tx_wait_work,
> > +                                                RTW89_TX_WAIT_WORK_TIMEOUT);
> > +                       continue;
> > +               }
> > +
> >                 ieee80211_tx_info_clear_status(info);
> > 
> >                 if (urb->status == 0) {
> > @@ -396,6 +405,8 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> >         memset(txdesc, 0, txdesc_size);
> >         rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
> > 
> > +       RTW89_TX_SKB_CB(skb)->tx_rpt_sn = tx_req->desc_info.sn;
> 
> Prefer a local variable 'struct rtw89_tx_skb_data *skb_data;'.
> 
> > +
> >         le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
> 
> This is also to fill tx desc, so prefer filling tx_rpt_sn after this. 
> 
> > 
> >         skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> > @@ -678,7 +689,7 @@ static void rtw89_usb_deinit_tx(struct rtw89_dev *rtwdev)
> > 
> >  static void rtw89_usb_ops_reset(struct rtw89_dev *rtwdev)
> >  {
> > -       /* TODO: anything to do here? */
> > +       rtw89_tx_rpt_queue_purge(rtwdev);
> 
> No sure if we can ensure no TX packets still on the fly --
> which have been submitted, but not call rtw89_usb_write_port_complete() yet.

It may race, agree.

> 
> But I'm also not sure if it is a problem that newly TX completion happens
> after calling rtw89_tx_rpt_queue_purge().

I can't imagine of scenario where that would be a problem, too.

> 
> >  }
> > 
> >  static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> > @@ -962,6 +973,7 @@ int rtw89_usb_probe(struct usb_interface *intf,
> > 
> >         rtwdev->hci.ops = &rtw89_usb_ops;
> >         rtwdev->hci.type = RTW89_HCI_TYPE_USB;
> > +       rtwdev->hci.tx_rpt_enable = true;
> > 
> >         ret = rtw89_usb_intf_init(rtwdev, intf);
> >         if (ret) {
> > --
> > 2.51.0
> > 
> 

Re: [PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Fedor Pchelkin]

On Sun, 12. Oct 01:49, Bitterblue Smith wrote:
> On 11/10/2025 17:57, Fedor Pchelkin wrote:
> > On Sat, 04. Oct 20:37, Bitterblue Smith wrote:
> >> I tested these patches with RTL8851BU, RTL8832AU, RTL8832BU, RTL8832CU, and
> >> RTL8912AU. They all work, with a few additions.
> >>
> >> Before these patches RTL8851BU and RTL8832AU would remain "connected" when
> >> I power off the router. That's because they don't have beacon filtering in
> >> the firmware and the null frames sent by mac80211 were always marked with
> >> IEEE80211_TX_STAT_ACK. With these patches they disconnect immediately when
> >> I power off the router. So that works nicely.
> >>
> > 
> > Glad to hear, thanks for the insight.
> > 
> >> What doesn't work is TX reports for management frames. Currently rtw89
> >> doesn't configure the firmware to provide TX reports for the management
> >> queue. That can be enabled with SET_CMC_TBL_MGQ_RPT_EN for the wifi 6 chips
> >> and with CCTLINFO_G7_W0_MGQ_RPT_EN for RTL8922AU.
> > 
> > I'll investigate. Looks like the enabling of the management part should go
> > to rtw89_fw_h2c_default_cmac_tbl().
> > 
> 
> Yes, and rtw89_fw_h2c_default_cmac_tbl_g7().
> 
> > Btw, could you give a quick hint please on how I can check during testing
> > that the reporting facility works properly for all cases needed?  By far
> > I've dealt with iw utility and debugging printks incorporated into rtw89
> > but it doesn't look sufficient anymore..
> > 
> 
> I enabled RTW89_DBG_TXRX, which let me see that no TX reports appeared
> during authentication and association. I also added a printk where the
> IEEE80211_TX_CTL_REQ_TX_STATUS flag is checked. Then I just use the
> driver normally, with wpa_supplicant and NetworkManager.

Thanks, Bitterblue!

By the way, do you see lots of "parse phy sts failed\n" messages printed
when RTW89_DBG_TXRX is enabled?  (it's with RTL8851BU in my case)

I wonder whether this is kind of a normal failure case or an indicator of
a firmware bug.

Just to point out, I've activated your workarounds from [1], otherwise
the device is unusable due to firmware unresponding during scan and
crashing eventually.

[1]: https://lore.kernel.org/linux-wireless/0abbda91-c5c2-4007-84c8-215679e652e1@gmail.com/

> 
> >>
> >> The other thing that doesn't work is the TX reports are different for
> >> RTL8852CU and RTL8922AU. It's only a small difference for RTL8852CU:
> >>
> >> #define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
> >>
> >> RTL8922AU is more strange. It needs something like this:
> >>
> >> #define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
> >> #define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
> >> #define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)
> >>
> >> The C2H is 80 bytes here (header included).
> > 
> > rtw89_mac_c2h_tx_rpt() needs to account for different types of C2H report
> > formats, bah.  Will add this missing part.
> 

RE: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Ping-Ke Shih]

> > > +
> > > +               if (sw_define != skb_data->tx_rpt_sn)
> > > +                       continue;
> > > +               if (tx_status != RTW89_TX_DONE &&
> > > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> >
> > As commit message of previous patch, "When there is a failed
> > TX status reported by the firmware, the report is ignored until the limit
> > is reached or success status appears."
> >
> > Do you still need to check data_txcnt for failed cases?
> 
> The question also concerns
> 
>   tx_req->desc_info.tx_cnt_lmt = 8;
> 
> line in rtw89_tx_rpt_enable().  'tx_cnt_lmt' is written to TX descriptor
> and processed by firmware.  The value defines how many times the firmware
> will retry transmission attempts, it will not retry more times than that.
> 
> 'data_txcnt' C2H field determines the retry attempt counter for the frame
> returned by the firmware.  If it reaches the limit, this means we got
> the last report from the firmware and there would be no other firmware
> reports for the sent frame.  So a final tx_status should be taken
> uncondionally in this case.
> 
> E.g. if 'tx_cnt_lmt' is set to 1, the firmware will try only once,
> 'data_txcnt' will be 1, too.  The limit is reached and we should take
> tx_status immediately as is.  So there's a higher chance of getting a
> failed status eventually.
> 
> I set it currently to 8 as the vendor driver does.  In local testing it
> looks more than enough.  I've seen maximum of 5 retry attempts for the
> same frame (usually there are no retries at all) though my network radio
> environment is quite noisy.
> 
> I'll add the tx_cnt_lmt related info to commit message for clarity.
> 

Thanks for the detail. 

    1 2 3 4 5 6 7 8
(a) x x x x x x x x  ==> retry 8 times, but all failure. Report at 8th C2H.
(b) x x x o          ==> retry 3 times, and 4th done. Report at 4th C2H.
(c) o                ==> just done at first one. Report at first C2H.

For every attempt, firmware reports a C2H with tx_status, right?
Can I say (a) case is why we should check data_txcnt? 
For cases (b)/(c), they rely on 'tx_status == RTW89_TX_DONE'.

Re: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Fedor Pchelkin]

On Wed, 15. Oct 01:43, Ping-Ke Shih wrote:
> > > > +
> > > > +               if (sw_define != skb_data->tx_rpt_sn)
> > > > +                       continue;
> > > > +               if (tx_status != RTW89_TX_DONE &&
> > > > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> > >
> > > As commit message of previous patch, "When there is a failed
> > > TX status reported by the firmware, the report is ignored until the limit
> > > is reached or success status appears."
> > >
> > > Do you still need to check data_txcnt for failed cases?
> > 
> > The question also concerns
> > 
> >   tx_req->desc_info.tx_cnt_lmt = 8;
> > 
> > line in rtw89_tx_rpt_enable().  'tx_cnt_lmt' is written to TX descriptor
> > and processed by firmware.  The value defines how many times the firmware
> > will retry transmission attempts, it will not retry more times than that.
> > 
> > 'data_txcnt' C2H field determines the retry attempt counter for the frame
> > returned by the firmware.  If it reaches the limit, this means we got
> > the last report from the firmware and there would be no other firmware
> > reports for the sent frame.  So a final tx_status should be taken
> > uncondionally in this case.
> > 
> > E.g. if 'tx_cnt_lmt' is set to 1, the firmware will try only once,
> > 'data_txcnt' will be 1, too.  The limit is reached and we should take
> > tx_status immediately as is.  So there's a higher chance of getting a
> > failed status eventually.
> > 
> > I set it currently to 8 as the vendor driver does.  In local testing it
> > looks more than enough.  I've seen maximum of 5 retry attempts for the
> > same frame (usually there are no retries at all) though my network radio
> > environment is quite noisy.
> > 
> > I'll add the tx_cnt_lmt related info to commit message for clarity.
> > 
> 
> Thanks for the detail. 
> 
>     1 2 3 4 5 6 7 8
> (a) x x x x x x x x  ==> retry 8 times, but all failure. Report at 8th C2H.
> (b) x x x o          ==> retry 3 times, and 4th done. Report at 4th C2H.
> (c) o                ==> just done at first one. Report at first C2H.
> 
> For every attempt, firmware reports a C2H with tx_status, right?

Yes.

> Can I say (a) case is why we should check data_txcnt? 
> For cases (b)/(c), they rely on 'tx_status == RTW89_TX_DONE'.

We should somehow determine in case (a) when those 8 attempts for the
frame have passed and then promptly give the report with a failed status
up to the wireless stack.  To my mind, without checking data_txcnt
rtw89_mac_c2h_tx_rpt() can't determine the time when to do an actual
report if every retry attempt has failed.

Otherwise skb would remain in the queue being unreported until HCI reset
takes place, though we already had a chance to report it as failed.

RE: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> On Wed, 15. Oct 01:43, Ping-Ke Shih wrote:
> > > > > +
> > > > > +               if (sw_define != skb_data->tx_rpt_sn)
> > > > > +                       continue;
> > > > > +               if (tx_status != RTW89_TX_DONE &&
> > > > > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> > > >
> > > > As commit message of previous patch, "When there is a failed
> > > > TX status reported by the firmware, the report is ignored until the limit
> > > > is reached or success status appears."
> > > >
> > > > Do you still need to check data_txcnt for failed cases?
> > >
> > > The question also concerns
> > >
> > >   tx_req->desc_info.tx_cnt_lmt = 8;
> > >
> > > line in rtw89_tx_rpt_enable().  'tx_cnt_lmt' is written to TX descriptor
> > > and processed by firmware.  The value defines how many times the firmware
> > > will retry transmission attempts, it will not retry more times than that.
> > >
> > > 'data_txcnt' C2H field determines the retry attempt counter for the frame
> > > returned by the firmware.  If it reaches the limit, this means we got
> > > the last report from the firmware and there would be no other firmware
> > > reports for the sent frame.  So a final tx_status should be taken
> > > uncondionally in this case.
> > >
> > > E.g. if 'tx_cnt_lmt' is set to 1, the firmware will try only once,
> > > 'data_txcnt' will be 1, too.  The limit is reached and we should take
> > > tx_status immediately as is.  So there's a higher chance of getting a
> > > failed status eventually.
> > >
> > > I set it currently to 8 as the vendor driver does.  In local testing it
> > > looks more than enough.  I've seen maximum of 5 retry attempts for the
> > > same frame (usually there are no retries at all) though my network radio
> > > environment is quite noisy.
> > >
> > > I'll add the tx_cnt_lmt related info to commit message for clarity.
> > >
> >
> > Thanks for the detail.
> >
> >     1 2 3 4 5 6 7 8
> > (a) x x x x x x x x  ==> retry 8 times, but all failure. Report at 8th C2H.
> > (b) x x x o          ==> retry 3 times, and 4th done. Report at 4th C2H.
> > (c) o                ==> just done at first one. Report at first C2H.
> >
> > For every attempt, firmware reports a C2H with tx_status, right?
> 
> Yes.
> 
> > Can I say (a) case is why we should check data_txcnt?
> > For cases (b)/(c), they rely on 'tx_status == RTW89_TX_DONE'.
> 
> We should somehow determine in case (a) when those 8 attempts for the
> frame have passed and then promptly give the report with a failed status
> up to the wireless stack.  To my mind, without checking data_txcnt
> rtw89_mac_c2h_tx_rpt() can't determine the time when to do an actual
> report if every retry attempt has failed.
> 
> Otherwise skb would remain in the queue being unreported until HCI reset
> takes place, though we already had a chance to report it as failed.

Got it.

By the way, I'd list case (d) which TX done at 8th retry and can be handled
properly as well. 

    1 2 3 4 5 6 7 8
(d) x x x x x x x o  ==> retry 8 times, and finally done. Report at 8th C2H.

Re: [PATCH rtw-next v2 6/7] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Fedor Pchelkin]

On Thu, 16. Oct 00:54, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > On Wed, 15. Oct 01:43, Ping-Ke Shih wrote:
> > > > > > +
> > > > > > +               if (sw_define != skb_data->tx_rpt_sn)
> > > > > > +                       continue;
> > > > > > +               if (tx_status != RTW89_TX_DONE &&
> > > > > > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> > > > >
> > > > > As commit message of previous patch, "When there is a failed
> > > > > TX status reported by the firmware, the report is ignored until the limit
> > > > > is reached or success status appears."
> > > > >
> > > > > Do you still need to check data_txcnt for failed cases?
> > > >
> > > > The question also concerns
> > > >
> > > >   tx_req->desc_info.tx_cnt_lmt = 8;
> > > >
> > > > line in rtw89_tx_rpt_enable().  'tx_cnt_lmt' is written to TX descriptor
> > > > and processed by firmware.  The value defines how many times the firmware
> > > > will retry transmission attempts, it will not retry more times than that.
> > > >
> > > > 'data_txcnt' C2H field determines the retry attempt counter for the frame
> > > > returned by the firmware.  If it reaches the limit, this means we got
> > > > the last report from the firmware and there would be no other firmware
> > > > reports for the sent frame.  So a final tx_status should be taken
> > > > uncondionally in this case.
> > > >
> > > > E.g. if 'tx_cnt_lmt' is set to 1, the firmware will try only once,
> > > > 'data_txcnt' will be 1, too.  The limit is reached and we should take
> > > > tx_status immediately as is.  So there's a higher chance of getting a
> > > > failed status eventually.
> > > >
> > > > I set it currently to 8 as the vendor driver does.  In local testing it
> > > > looks more than enough.  I've seen maximum of 5 retry attempts for the
> > > > same frame (usually there are no retries at all) though my network radio
> > > > environment is quite noisy.
> > > >
> > > > I'll add the tx_cnt_lmt related info to commit message for clarity.
> > > >
> > >
> > > Thanks for the detail.
> > >
> > >     1 2 3 4 5 6 7 8
> > > (a) x x x x x x x x  ==> retry 8 times, but all failure. Report at 8th C2H.
> > > (b) x x x o          ==> retry 3 times, and 4th done. Report at 4th C2H.
> > > (c) o                ==> just done at first one. Report at first C2H.
> > >
> > > For every attempt, firmware reports a C2H with tx_status, right?
> > 
> > Yes.
> > 
> > > Can I say (a) case is why we should check data_txcnt?
> > > For cases (b)/(c), they rely on 'tx_status == RTW89_TX_DONE'.
> > 
> > We should somehow determine in case (a) when those 8 attempts for the
> > frame have passed and then promptly give the report with a failed status
> > up to the wireless stack.  To my mind, without checking data_txcnt
> > rtw89_mac_c2h_tx_rpt() can't determine the time when to do an actual
> > report if every retry attempt has failed.
> > 
> > Otherwise skb would remain in the queue being unreported until HCI reset
> > takes place, though we already had a chance to report it as failed.
> 
> Got it.
> 
> By the way, I'd list case (d) which TX done at 8th retry and can be handled
> properly as well. 
> 
>     1 2 3 4 5 6 7 8
> (d) x x x x x x x o  ==> retry 8 times, and finally done. Report at 8th C2H.

This is covered by `tx_status != RTW89_TX_DONE` check that's the first
half of the && expression.

Re: [PATCH rtw-next v2 0/7] wifi: rtw89: improvements for USB part

[Bitterblue Smith]

On 15/10/2025 00:33, Fedor Pchelkin wrote:
> On Sun, 12. Oct 01:49, Bitterblue Smith wrote:
>> On 11/10/2025 17:57, Fedor Pchelkin wrote:
>>> On Sat, 04. Oct 20:37, Bitterblue Smith wrote:
>>>> I tested these patches with RTL8851BU, RTL8832AU, RTL8832BU, RTL8832CU, and
>>>> RTL8912AU. They all work, with a few additions.
>>>>
>>>> Before these patches RTL8851BU and RTL8832AU would remain "connected" when
>>>> I power off the router. That's because they don't have beacon filtering in
>>>> the firmware and the null frames sent by mac80211 were always marked with
>>>> IEEE80211_TX_STAT_ACK. With these patches they disconnect immediately when
>>>> I power off the router. So that works nicely.
>>>>
>>>
>>> Glad to hear, thanks for the insight.
>>>
>>>> What doesn't work is TX reports for management frames. Currently rtw89
>>>> doesn't configure the firmware to provide TX reports for the management
>>>> queue. That can be enabled with SET_CMC_TBL_MGQ_RPT_EN for the wifi 6 chips
>>>> and with CCTLINFO_G7_W0_MGQ_RPT_EN for RTL8922AU.
>>>
>>> I'll investigate. Looks like the enabling of the management part should go
>>> to rtw89_fw_h2c_default_cmac_tbl().
>>>
>>
>> Yes, and rtw89_fw_h2c_default_cmac_tbl_g7().
>>
>>> Btw, could you give a quick hint please on how I can check during testing
>>> that the reporting facility works properly for all cases needed?  By far
>>> I've dealt with iw utility and debugging printks incorporated into rtw89
>>> but it doesn't look sufficient anymore..
>>>
>>
>> I enabled RTW89_DBG_TXRX, which let me see that no TX reports appeared
>> during authentication and association. I also added a printk where the
>> IEEE80211_TX_CTL_REQ_TX_STATUS flag is checked. Then I just use the
>> driver normally, with wpa_supplicant and NetworkManager.
> 
> Thanks, Bitterblue!
> 
> By the way, do you see lots of "parse phy sts failed\n" messages printed
> when RTW89_DBG_TXRX is enabled?  (it's with RTL8851BU in my case)
> 
> I wonder whether this is kind of a normal failure case or an indicator of
> a firmware bug.
> 

Yes, I see that with RTL8851BU, RTL8832BU, RTL8832AU. I didn't
investigate.

> Just to point out, I've activated your workarounds from [1], otherwise
> the device is unusable due to firmware unresponding during scan and
> crashing eventually.
> 
> [1]: https://lore.kernel.org/linux-wireless/0abbda91-c5c2-4007-84c8-215679e652e1@gmail.com/
> 

Maybe I should send a patch for that. I thought it was an easy problem
and the firmware will be fixed before 6.17. Then I forgot about it.

>>
>>>>
>>>> The other thing that doesn't work is the TX reports are different for
>>>> RTL8852CU and RTL8922AU. It's only a small difference for RTL8852CU:
>>>>
>>>> #define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
>>>>
>>>> RTL8922AU is more strange. It needs something like this:
>>>>
>>>> #define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
>>>> #define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
>>>> #define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)
>>>>
>>>> The C2H is 80 bytes here (header included).
>>>
>>> rtw89_mac_c2h_tx_rpt() needs to account for different types of C2H report
>>> formats, bah.  Will add this missing part.
>>