[PATCH rtw-next v3 0/9] wifi: rtw89: improvements for USB part

[PATCH rtw-next v3 0/9] 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) for TX report
functionality - I've been able to test only V0 format of C2H message, and
RTL8852BE (PCIe) devices for other things (mainly that the changes have
not unexpectedly influenced th PCIe part).

Changelog.

v3: - add new 6/9 and 8/9 patches based on previous feedback
    - further changelog below --- in the patches

v2: https://lore.kernel.org/linux-wireless/20251002200857.657747-1-pchelkin@ispras.ru/
    - 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 (8):
  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: provide TX reports for management frames
  wifi: rtw89: process TX wait skbs for USB via C2H handler

Ping-Ke Shih (1):
  wifi: rtw89: fill TX descriptor of FWCMD in shortcut

 drivers/net/wireless/realtek/rtw89/core.c | 61 +++++++++++++-------
 drivers/net/wireless/realtek/rtw89/core.h | 40 +++++++++----
 drivers/net/wireless/realtek/rtw89/fw.c   |  4 +-
 drivers/net/wireless/realtek/rtw89/fw.h   | 41 ++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 65 +++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  | 69 +++++++++++++++++++++++
 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 +++++++++++---
 10 files changed, 288 insertions(+), 45 deletions(-)

-- 
2.51.0

[PATCH rtw-next v3 1/9] 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>
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
---

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 v3 2/9] 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 v3 3/9] 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>
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
---
 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 v3 4/9] 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>
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
---
 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 v3 5/9] 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 to indicate to the firmware that TX
report for the frame is expected.   This will allow handling TX wait skbs
and the ones flagged with IEEE80211_TX_CTL_REQ_TX_STATUS correctly.

Do the bulk of the patch according to the vendor driver for RTL8851BU.
However, there are slight differences in C2H message format between
different types of chips.  RTL885xB ones follow format V0.  RTL8852C has
format V1, and RTL8922AU has format V2.

Found by Linux Verification Center (linuxtesting.org).

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

v3: - consider V1 and V2 C2H message formats inside rtw89_mac_c2h_tx_rpt() (Bitterblue)
    - use X'mas tree order (Ping-Ke)
    - update changelog considering further patches

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)

 drivers/net/wireless/realtek/rtw89/core.c | 28 +++++++++++++---
 drivers/net/wireless/realtek/rtw89/core.h |  4 +++
 drivers/net/wireless/realtek/rtw89/fw.h   | 41 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 41 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  |  7 ++++
 drivers/net/wireless/realtek/rtw89/txrx.h |  6 +++-
 6 files changed, 121 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..e0e587ec4592 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.h
+++ b/drivers/net/wireless/realtek/rtw89/fw.h
@@ -3747,6 +3747,47 @@ 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)
+#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
+
+struct rtw89_c2h_mac_tx_rpt_v2 {
+	struct rtw89_c2h_hdr hdr;
+	__le32 w2;
+	__le32 w3;
+	__le32 w4;
+	__le32 w5;
+	__le32 w6;
+	__le32 w7;
+	__le32 w8;
+	__le32 w9;
+	__le32 w10;
+	__le32 w11;
+	__le32 w12;
+	__le32 w13;
+	__le32 w14;
+	__le32 w15;
+	__le32 w16;
+	__le32 w17;
+	__le32 w18;
+	__le32 w19;
+};
+
+#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)
+
 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..2fe239f18534 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5457,6 +5457,35 @@ 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)
+{
+	u8 sw_define, tx_status, data_txcnt;
+
+	if (rtwdev->chip->chip_id == RTL8922A) {
+		const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
+
+		rpt_v2 = (const struct rtw89_c2h_mac_tx_rpt_v2 *)c2h->data;
+		sw_define = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2);
+		tx_status = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2);
+		data_txcnt = le32_get_bits(rpt_v2->w14, RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2);
+	} else {
+		const struct rtw89_c2h_mac_tx_rpt *rpt;
+
+		rpt = (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
+		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);
+		if (rtwdev->chip->chip_id == RTL8852C)
+			data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
+		else
+			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 +5720,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 +5812,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 +5849,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 v3 6/9] wifi: rtw89: fill TX descriptor of FWCMD in shortcut

[Fedor Pchelkin]

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

TX type FWCMD is used to download firmware and send H2C commands, and
it only fill few fields of TX description, such as desc_info->pkt_size.
Therefore, early return the TX type FWCMD.

Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/core.c | 26 ++++++++++++-----------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 49ecc248464b..abe8eec1d0f5 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1087,32 +1087,35 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hdr *hdr = (void *)skb->data;
 	struct rtw89_addr_cam_entry *addr_cam;
-	enum rtw89_core_tx_type tx_type;
 	enum btc_pkt_type pkt_type;
 	bool upd_wlan_hdr = false;
 	bool is_bmc;
 	u16 seq;
 
+	desc_info->pkt_size = skb->len;
+
+	if (unlikely(tx_req->tx_type == RTW89_CORE_TX_TYPE_FWCMD)) {
+		rtw89_core_tx_update_h2c_info(rtwdev, tx_req);
+		return;
+	}
+
+	tx_req->tx_type = rtw89_core_get_tx_type(rtwdev, skb);
+
 	if (tx_req->sta)
 		desc_info->mlo = tx_req->sta->mlo;
 	else if (tx_req->vif)
 		desc_info->mlo = ieee80211_vif_is_mld(tx_req->vif);
 
 	seq = (le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_SEQ) >> 4;
-	if (tx_req->tx_type != RTW89_CORE_TX_TYPE_FWCMD) {
-		tx_type = rtw89_core_get_tx_type(rtwdev, skb);
-		tx_req->tx_type = tx_type;
+	addr_cam = rtw89_get_addr_cam_of(tx_req->rtwvif_link,
+					 tx_req->rtwsta_link);
+	if (addr_cam->valid && desc_info->mlo)
+		upd_wlan_hdr = true;
 
-		addr_cam = rtw89_get_addr_cam_of(tx_req->rtwvif_link,
-						 tx_req->rtwsta_link);
-		if (addr_cam->valid && desc_info->mlo)
-			upd_wlan_hdr = true;
-	}
 	is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
 		  is_multicast_ether_addr(hdr->addr1));
 
 	desc_info->seq = seq;
-	desc_info->pkt_size = skb->len;
 	desc_info->is_bmc = is_bmc;
 	desc_info->wd_page = true;
 	desc_info->hiq = info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM;
@@ -1129,8 +1132,7 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 		rtw89_core_tx_update_ampdu_info(rtwdev, tx_req, pkt_type);
 		rtw89_core_tx_update_llc_hdr(rtwdev, desc_info, skb);
 		break;
-	case RTW89_CORE_TX_TYPE_FWCMD:
-		rtw89_core_tx_update_h2c_info(rtwdev, tx_req);
+	default:
 		break;
 	}
 }
-- 
2.51.0

[PATCH rtw-next v3 7/9] 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_enabled flag defining which HCIs need to enable a TX
report feature.  Currently it is USB only.

Toggle a bit in the TX descriptor and place flagged skbs in a queue to
wait for a message from the firmware.  The main part is quite similar to
what rtw88 does.  The difference is that rtw89 has a new feature providing
a TX report for each transmission attempt.  Ignore a failed TX status
reported by the firmware until retry limit is reached or successful status
appears.  When there is no success and the retry limit is reached, report
the frame up to the wireless stack as failed eventually.

Forcefully flush the queue on HCI reset.

Found by Linux Verification Center (linuxtesting.org).

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

v3: - don't mix TX RPT queue with TX wait list processing (Ping-Ke)
    - add struct rtw89_tx_rpt for convenience, that's also like in rtw88
    - update changelog to reflect TX RPT retry logic and add some
      comments inside rtw89_mac_c2h_tx_rpt()

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.

 drivers/net/wireless/realtek/rtw89/core.c |  4 ++
 drivers/net/wireless/realtek/rtw89/core.h | 10 ++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 24 ++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  | 56 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/usb.c  | 15 +++++-
 5 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index abe8eec1d0f5..3aa9a9a28118 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1112,6 +1112,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 	if (addr_cam->valid && desc_info->mlo)
 		upd_wlan_hdr = true;
 
+	if (rtw89_is_tx_rpt_skb(tx_req->skb))
+		rtw89_tx_rpt_init(rtwdev, tx_req);
+
 	is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
 		  is_multicast_ether_addr(hdr->addr1));
 
@@ -5849,6 +5852,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..8641e3a8d36d 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -3516,6 +3516,11 @@ struct rtw89_phy_rate_pattern {
 #define RTW89_TX_LIFE_TIME		0x2
 #define RTW89_TX_MACID_DROP		0x3
 
+struct rtw89_tx_rpt {
+	struct sk_buff_head queue;
+	atomic_t sn;
+};
+
 #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
 struct rtw89_tx_wait_info {
 	struct rcu_head rcu_head;
@@ -3527,6 +3532,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 +3703,7 @@ struct rtw89_hci_info {
 	u32 rpwm_addr;
 	u32 cpwm_addr;
 	bool paused;
+	bool tx_rpt_enabled;
 };
 
 struct rtw89_chip_ops {
@@ -6015,6 +6023,8 @@ struct rtw89_dev {
 	struct list_head tx_waits;
 	struct wiphy_delayed_work tx_wait_work;
 
+	struct rtw89_tx_rpt tx_rpt;
+
 	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 2fe239f18534..26c7476afdec 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5460,7 +5460,11 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
 static void
 rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 {
+	struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
 	u8 sw_define, tx_status, data_txcnt;
+	struct rtw89_tx_skb_data *skb_data;
+	struct sk_buff *skb, *tmp;
+	unsigned long flags;
 
 	if (rtwdev->chip->chip_id == RTL8922A) {
 		const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
@@ -5484,6 +5488,26 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 	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(&tx_rpt->queue.lock, flags);
+	skb_queue_walk_safe(&tx_rpt->queue, skb, tmp) {
+		skb_data = RTW89_TX_SKB_CB(skb);
+
+		/* skip if sequence number doesn't match */
+		if (sw_define != skb_data->tx_rpt_sn)
+			continue;
+		/* skip if TX attempt has failed and retry limit has not been
+		 * reached yet
+		 */
+		if (tx_status != RTW89_TX_DONE &&
+		    data_txcnt != skb_data->tx_pkt_cnt_lmt)
+			continue;
+
+		__skb_unlink(skb, &tx_rpt->queue);
+		rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
+		break;
+	}
+	spin_unlock_irqrestore(&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..e8bd92223497 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -1616,4 +1616,60 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
 
 	return ret;
 }
+
+static inline
+void rtw89_tx_rpt_init(struct rtw89_dev *rtwdev,
+		       struct rtw89_core_tx_request *tx_req)
+{
+	struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
+
+	if (!rtwdev->hci.tx_rpt_enabled)
+		return;
+
+	tx_req->desc_info.report = true;
+	/* firmware maintains a 4-bit sequence number */
+	tx_req->desc_info.sn = atomic_inc_return(&tx_rpt->sn) & 0xF;
+	tx_req->desc_info.tx_cnt_lmt_en = true;
+	tx_req->desc_info.tx_cnt_lmt = 8;
+}
+
+static inline
+bool rtw89_is_tx_rpt_skb(struct sk_buff *skb)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+	return info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
+}
+
+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 rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
+	struct sk_buff_head q;
+	struct sk_buff *skb;
+	unsigned long flags;
+
+	__skb_queue_head_init(&q);
+
+	spin_lock_irqsave(&tx_rpt->queue.lock, flags);
+	skb_queue_splice_init(&tx_rpt->queue, &q);
+	spin_unlock_irqrestore(&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..22994c3501f8 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -216,6 +216,14 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
 		skb_pull(skb, txdesc_size);
 
 		info = IEEE80211_SKB_CB(skb);
+		if (rtw89_is_tx_rpt_skb(skb)) {
+			/* sequence number is passed to rtw89_mac_c2h_tx_rpt() via
+			 * driver data
+			 */
+			skb_queue_tail(&rtwdev->tx_rpt.queue, skb);
+			continue;
+		}
+
 		ieee80211_tx_info_clear_status(info);
 
 		if (urb->status == 0) {
@@ -372,6 +380,7 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
 {
 	struct rtw89_tx_desc_info *desc_info = &tx_req->desc_info;
 	struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
+	struct rtw89_tx_skb_data *skb_data;
 	struct sk_buff *skb = tx_req->skb;
 	struct rtw89_txwd_body *txdesc;
 	u32 txdesc_size;
@@ -398,6 +407,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
 
 	le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
 
+	skb_data = RTW89_TX_SKB_CB(skb);
+	skb_data->tx_rpt_sn = tx_req->desc_info.sn;
+
 	skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
 
 	return 0;
@@ -678,7 +690,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 +974,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_enabled = true;
 
 	ret = rtw89_usb_intf_init(rtwdev, intf);
 	if (ret) {
-- 
2.51.0

[PATCH rtw-next v3 8/9] wifi: rtw89: provide TX reports for management frames

[Fedor Pchelkin]

In order to provide TX reports for the management queue rtw89 should
configure the firmware.  Do this with SET_CMC_TBL_MGQ_RPT_EN() for the
WiFi6 chips and with CCTLINFO_G7_W0_MGQ_RPT_EN flag for the WiFi7 ones.

Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/fw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
index ab904a7def1b..0d4438bf8dcc 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.c
+++ b/drivers/net/wireless/realtek/rtw89/fw.c
@@ -3165,6 +3165,7 @@ int rtw89_fw_h2c_default_cmac_tbl(struct rtw89_dev *rtwdev,
 		SET_CMC_TBL_ANTSEL_C(skb->data, 0);
 		SET_CMC_TBL_ANTSEL_D(skb->data, 0);
 	}
+	SET_CMC_TBL_MGQ_RPT_EN(skb->data, rtwdev->hci.tx_rpt_enabled);
 	SET_CMC_TBL_DOPPLER_CTRL(skb->data, 0);
 	SET_CMC_TBL_TXPWR_TOLERENCE(skb->data, 0);
 	if (rtwvif_link->net_type == RTW89_NET_TYPE_AP_MODE)
@@ -3210,7 +3211,8 @@ int rtw89_fw_h2c_default_cmac_tbl_g7(struct rtw89_dev *rtwdev,
 	h2c->c0 = le32_encode_bits(mac_id, CCTLINFO_G7_C0_MACID) |
 		  le32_encode_bits(1, CCTLINFO_G7_C0_OP);
 
-	h2c->w0 = le32_encode_bits(4, CCTLINFO_G7_W0_DATARATE);
+	h2c->w0 = le32_encode_bits(4, CCTLINFO_G7_W0_DATARATE) |
+		  le32_encode_bits(rtwdev->hci.tx_rpt_enabled, CCTLINFO_G7_W0_MGQ_RPT_EN);
 	h2c->m0 = cpu_to_le32(CCTLINFO_G7_W0_ALL);
 
 	h2c->w1 = le32_encode_bits(4, CCTLINFO_G7_W1_DATA_RTY_LOWEST_RATE) |
-- 
2.51.0

[PATCH rtw-next v3 9/9] 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 the C2H handler via private driver data of
skb.  Try to do this as similar to PCIe path as possible.  When the
corresponding TX wait skb is found inside TX report queue, unlink it from
there and call rtw89_core_tx_wait_complete() to mark the completion.

If the callee waiting for the completion has already timed out, the TX
wait skb is placed into TX wait list (like PCIe part does).

Found by Linux Verification Center (linuxtesting.org).

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

v3: - use rcu_access_pointer() inside rtw89_core_is_tx_wait()  (Zong-Zhe)
    - finally I've hopefully realized what you meant by doing TX wait
      skb handling here similar to PCIe.  IMO this looks just elegant now
      compared to my previous submissions. Thanks, Ping-Ke!

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

 drivers/net/wireless/realtek/rtw89/core.c |  5 ++---
 drivers/net/wireless/realtek/rtw89/core.h |  6 ++++++
 drivers/net/wireless/realtek/rtw89/mac.h  | 10 ++++++++--
 drivers/net/wireless/realtek/rtw89/usb.c  |  2 +-
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 3aa9a9a28118..e28020429692 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1112,7 +1112,7 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
 	if (addr_cam->valid && desc_info->mlo)
 		upd_wlan_hdr = true;
 
-	if (rtw89_is_tx_rpt_skb(tx_req->skb))
+	if (rtw89_is_tx_rpt_skb(rtwdev, tx_req->skb))
 		rtw89_tx_rpt_init(rtwdev, tx_req);
 
 	is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
@@ -1244,14 +1244,13 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
 	tx_req.rtwvif_link = rtwvif_link;
 	tx_req.rtwsta_link = rtwsta_link;
 	tx_req.desc_info.sw_mld = sw_mld;
+	rcu_assign_pointer(skb_data->wait, wait);
 
 	rtw89_traffic_stats_accu(rtwdev, rtwvif, skb, true, true);
 	rtw89_wow_parse_akm(rtwdev, skb);
 	rtw89_core_tx_update_desc_info(rtwdev, &tx_req);
 	rtw89_core_tx_wake(rtwdev, &tx_req);
 
-	rcu_assign_pointer(skb_data->wait, wait);
-
 	ret = rtw89_hci_tx_write(rtwdev, &tx_req);
 	if (ret) {
 		rtw89_err(rtwdev, "failed to transmit skb to HCI\n");
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 8641e3a8d36d..85d25eb316a3 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -7391,6 +7391,12 @@ 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)
+{
+	return rcu_access_pointer(skb_data->wait);
+}
+
 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.h b/drivers/net/wireless/realtek/rtw89/mac.h
index e8bd92223497..fe3573455247 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -1634,18 +1634,24 @@ void rtw89_tx_rpt_init(struct rtw89_dev *rtwdev,
 }
 
 static inline
-bool rtw89_is_tx_rpt_skb(struct sk_buff *skb)
+bool rtw89_is_tx_rpt_skb(struct rtw89_dev *rtwdev, struct sk_buff *skb)
 {
+	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 
-	return info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
+	return rtw89_core_is_tx_wait(rtwdev, skb_data) ||
+	       (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS);
 }
 
 static inline
 void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
 {
+	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 
+	if (rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
+		return;
+
 	ieee80211_tx_info_clear_status(info);
 	if (tx_status == RTW89_TX_DONE)
 		info->flags |= IEEE80211_TX_STAT_ACK;
diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 22994c3501f8..e4077dec28c9 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -216,7 +216,7 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
 		skb_pull(skb, txdesc_size);
 
 		info = IEEE80211_SKB_CB(skb);
-		if (rtw89_is_tx_rpt_skb(skb)) {
+		if (rtw89_is_tx_rpt_skb(rtwdev, skb)) {
 			/* sequence number is passed to rtw89_mac_c2h_tx_rpt() via
 			 * driver data
 			 */
-- 
2.51.0

RE: [PATCH rtw-next v3 5/9] 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 to indicate to the firmware that TX
> report for the frame is expected.   This will allow handling TX wait skbs
> and the ones flagged with IEEE80211_TX_CTL_REQ_TX_STATUS correctly.
> 
> Do the bulk of the patch according to the vendor driver for RTL8851BU.
> However, there are slight differences in C2H message format between
> different types of chips.  RTL885xB ones follow format V0.  RTL8852C has
> format V1, and RTL8922AU has format V2.
> 
> 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/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h
> index ddebf7972068..e0e587ec4592 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.h
> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
> @@ -3747,6 +3747,47 @@ 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;
> +};

__packed

> +
> +#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)
> +#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
> +
> +struct rtw89_c2h_mac_tx_rpt_v2 {
> +       struct rtw89_c2h_hdr hdr;
> +       __le32 w2;
> +       __le32 w3;
> +       __le32 w4;
> +       __le32 w5;
> +       __le32 w6;
> +       __le32 w7;
> +       __le32 w8;
> +       __le32 w9;
> +       __le32 w10;
> +       __le32 w11;
> +       __le32 w12;
> +       __le32 w13;
> +       __le32 w14;
> +       __le32 w15;
> +       __le32 w16;
> +       __le32 w17;
> +       __le32 w18;
> +       __le32 w19;
> +};

__packed

> +
> +#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)
> +
>  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..2fe239f18534 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,6 +5457,35 @@ 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)
> +{
> +       u8 sw_define, tx_status, data_txcnt;
> +
> +       if (rtwdev->chip->chip_id == RTL8922A) {

Add a chip_ops c2h_tx_rpt? Then, no need chip_id checking, and reduce line
length (normally we prefer shorter than 80 or 90 characters; over 100 characters
isn't a good idea).

Maybe this is because you want to store the status into local variables.
With a chip_ops, you should define another struct to store them.

Or, you just keep it as was, but wrap lines to be shorter, and give shorter
naming. For example,
  - rpt_v2 -> v2

  - data_txcnt -> txcnt

		if (rtwdev->chip->chip_id == RTL8852C)
			txcnt = le32_get_bits(rpt->w5,
					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
		else
			txcnt = le32_get_bits(rpt->w5,
					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);


> +               const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> +
> +               rpt_v2 = (const struct rtw89_c2h_mac_tx_rpt_v2 *)c2h->data;
> +               sw_define = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2);
> +               tx_status = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2);
> +               data_txcnt = le32_get_bits(rpt_v2->w14, RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2);
> +       } else {
> +               const struct rtw89_c2h_mac_tx_rpt *rpt;
> +
> +               rpt = (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
> +               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);
> +               if (rtwdev->chip->chip_id == RTL8852C)
> +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> +               else
> +                       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)
>  {

[...]

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

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index abe8eec1d0f5..3aa9a9a28118 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1112,6 +1112,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
>         if (addr_cam->valid && desc_info->mlo)
>                 upd_wlan_hdr = true;
> 
> +       if (rtw89_is_tx_rpt_skb(tx_req->skb))
> +               rtw89_tx_rpt_init(rtwdev, tx_req);
> +
>         is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
>                   is_multicast_ether_addr(hdr->addr1));
> 
> @@ -5849,6 +5852,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);

not sure if it's worth to initialize tx_rpt.sn to zero?

>         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..8641e3a8d36d 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3516,6 +3516,11 @@ struct rtw89_phy_rate_pattern {
>  #define RTW89_TX_LIFE_TIME             0x2
>  #define RTW89_TX_MACID_DROP            0x3
> 
> +struct rtw89_tx_rpt {
> +       struct sk_buff_head queue;
> +       atomic_t sn;
> +};
> +
>  #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
>  struct rtw89_tx_wait_info {
>         struct rcu_head rcu_head;
> @@ -3527,6 +3532,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 +3703,7 @@ struct rtw89_hci_info {
>         u32 rpwm_addr;
>         u32 cpwm_addr;
>         bool paused;
> +       bool tx_rpt_enabled;
>  };
> 
>  struct rtw89_chip_ops {
> @@ -6015,6 +6023,8 @@ struct rtw89_dev {
>         struct list_head tx_waits;
>         struct wiphy_delayed_work tx_wait_work;
> 
> +       struct rtw89_tx_rpt tx_rpt;
> +
>         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 2fe239f18534..26c7476afdec 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5460,7 +5460,11 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
>  static void
>  rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
> +       struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
>         u8 sw_define, tx_status, data_txcnt;
> +       struct rtw89_tx_skb_data *skb_data;
> +       struct sk_buff *skb, *tmp;
> +       unsigned long flags;
> 
>         if (rtwdev->chip->chip_id == RTL8922A) {
>                 const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> @@ -5484,6 +5488,26 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>         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(&tx_rpt->queue.lock, flags);
> +       skb_queue_walk_safe(&tx_rpt->queue, skb, tmp) {
> +               skb_data = RTW89_TX_SKB_CB(skb);
> +
> +               /* skip if sequence number doesn't match */
> +               if (sw_define != skb_data->tx_rpt_sn)
> +                       continue;
> +               /* skip if TX attempt has failed and retry limit has not been
> +                * reached yet
> +                */
> +               if (tx_status != RTW89_TX_DONE &&
> +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> +                       continue;
> +
> +               __skb_unlink(skb, &tx_rpt->queue);
> +               rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);

Would it be better to run rtw89_tx_rpt_tx_status() after this loop outside
spin_lock()?

> +               break;
> +       }
> +       spin_unlock_irqrestore(&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..e8bd92223497 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1616,4 +1616,60 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
> 
>         return ret;
>  }
> +
> +static inline
> +void rtw89_tx_rpt_init(struct rtw89_dev *rtwdev,
> +                      struct rtw89_core_tx_request *tx_req)
> +{
> +       struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> +
> +       if (!rtwdev->hci.tx_rpt_enabled)
> +               return;
> +
> +       tx_req->desc_info.report = true;
> +       /* firmware maintains a 4-bit sequence number */
> +       tx_req->desc_info.sn = atomic_inc_return(&tx_rpt->sn) & 0xF;
> +       tx_req->desc_info.tx_cnt_lmt_en = true;
> +       tx_req->desc_info.tx_cnt_lmt = 8;
> +}
> +
> +static inline
> +bool rtw89_is_tx_rpt_skb(struct sk_buff *skb)
> +{
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> +       return info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
> +}
> +
> +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 rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> +       struct sk_buff_head q;
> +       struct sk_buff *skb;
> +       unsigned long flags;
> +
> +       __skb_queue_head_init(&q);
> +
> +       spin_lock_irqsave(&tx_rpt->queue.lock, flags);
> +       skb_queue_splice_init(&tx_rpt->queue, &q);
> +       spin_unlock_irqrestore(&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..22994c3501f8 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,6 +216,14 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 skb_pull(skb, txdesc_size);
> 
>                 info = IEEE80211_SKB_CB(skb);
> +               if (rtw89_is_tx_rpt_skb(skb)) {
> +                       /* sequence number is passed to rtw89_mac_c2h_tx_rpt() via

nit: The 'via' is over 80 characters a little bit. Move to next line.

> +                        * driver data
> +                        */
> +                       skb_queue_tail(&rtwdev->tx_rpt.queue, skb);
> +                       continue;
> +               }
> +
>                 ieee80211_tx_info_clear_status(info);
> 
>                 if (urb->status == 0) {

Should we move this checking upward? Enqueue skb into tx_rpt_skb only if
urb->status == 0?

> @@ -372,6 +380,7 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
>  {
>         struct rtw89_tx_desc_info *desc_info = &tx_req->desc_info;
>         struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
> +       struct rtw89_tx_skb_data *skb_data;
>         struct sk_buff *skb = tx_req->skb;
>         struct rtw89_txwd_body *txdesc;
>         u32 txdesc_size;
> @@ -398,6 +407,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> 
>         le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
> 
> +       skb_data = RTW89_TX_SKB_CB(skb);
> +       skb_data->tx_rpt_sn = tx_req->desc_info.sn;

Shouldn't set skb_data->tx_pkt_cnt_lmt? 

skb_data->tx_pkt_cnt_lmt = tx_req->desc_info.tx_cnt_lmt;

Also, should we check desc_info.{report, tx_cnt_lmt_en} individually before 
setting?


> +
>         skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> 
>         return 0;
> @@ -678,7 +690,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);

Have you consider the SKB that has been rtw89_usb_write_port() but
has not yet rtw89_usb_write_port_complete()?

Since we call rtw89_mac_pwr_off() before rtw89_hci_reset() in 
rtw89_core_stop(), it should be not more C2H at rtw89_hci_reset().
It seems to be safe, right?

Also, all are dropped, can't we just call ieee80211_purge_tx_queue()?

>  }
> 
>  static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> @@ -962,6 +974,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_enabled = true;
> 
>         ret = rtw89_usb_intf_init(rtwdev, intf);
>         if (ret) {
> --
> 2.51.0
> 

RE: [PATCH rtw-next v3 8/9] wifi: rtw89: provide TX reports for management frames

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> In order to provide TX reports for the management queue rtw89 should
> configure the firmware.  Do this with SET_CMC_TBL_MGQ_RPT_EN() for the
> WiFi6 chips and with CCTLINFO_G7_W0_MGQ_RPT_EN flag for the WiFi7 ones.
> 
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

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

RE: [PATCH rtw-next v3 9/9] 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 the C2H handler via private driver data of
> skb.  Try to do this as similar to PCIe path as possible.  When the
> corresponding TX wait skb is found inside TX report queue, unlink it from
> there and call rtw89_core_tx_wait_complete() to mark the completion.
> 
> If the callee waiting for the completion has already timed out, the TX
> wait skb is placed into TX wait list (like PCIe part does).
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

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

[...]

>  static inline
>  void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
>  {
> +       struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> 
> +       if (rtw89_core_tx_wait_complete(rtwdev, skb_data, tx_status))
> +               return;
> +

nit: move 'info = IEEE80211_SKB_CB(skb);' here like PCIE does.

>         ieee80211_tx_info_clear_status(info);
>         if (tx_status == RTW89_TX_DONE)
>                 info->flags |= IEEE80211_TX_STAT_ACK;

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

[Ping-Ke Shih]

Ping-Ke Shih <pkshih@realtek.com> wrote:
> 
> Also, all are dropped, can't we just call ieee80211_purge_tx_queue()?
> 

Please ignore this comment. Since you want to complete wait, then
rtw89_tx_wait_list_clear() can delete wait marked as completed from list.

Re: [PATCH rtw-next v3 5/9] wifi: rtw89: implement C2H TX report handler

[Fedor Pchelkin]

On Wed, 22. Oct 06:21, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > @@ -5457,6 +5457,35 @@ 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)
> > +{
> > +       u8 sw_define, tx_status, data_txcnt;
> > +
> > +       if (rtwdev->chip->chip_id == RTL8922A) {
> 
> Add a chip_ops c2h_tx_rpt? Then, no need chip_id checking, and reduce line
> length (normally we prefer shorter than 80 or 90 characters; over 100 characters
> isn't a good idea).
> 
> Maybe this is because you want to store the status into local variables.
> With a chip_ops, you should define another struct to store them.
> 
> Or, you just keep it as was, but wrap lines to be shorter, and give shorter
> naming. For example,
>   - rpt_v2 -> v2
> 
>   - data_txcnt -> txcnt
> 
> 		if (rtwdev->chip->chip_id == RTL8852C)
> 			txcnt = le32_get_bits(rpt->w5,
> 					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> 		else
> 			txcnt = le32_get_bits(rpt->w5,
> 					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> 

The chip_ops variant is more abstract (and good in such a way) but I don't
think it's worth mangling with modifying struct chip_ops of each chip and
adding new structs as we probably won't need to scale or expand
rtw89_mac_c2h_tx_rpt() further unless some another V3 format is supported.
Also other mac C2H functions tend to check chip_id in place if they need.

I'll try to make the lines shorter - the longest one will be 86 symbols.

> 
> > +               const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> > +
> > +               rpt_v2 = (const struct rtw89_c2h_mac_tx_rpt_v2 *)c2h->data;
> > +               sw_define = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2);
> > +               tx_status = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2);
> > +               data_txcnt = le32_get_bits(rpt_v2->w14, RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2);
> > +       } else {
> > +               const struct rtw89_c2h_mac_tx_rpt *rpt;
> > +
> > +               rpt = (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
> > +               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);
> > +               if (rtwdev->chip->chip_id == RTL8852C)
> > +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> > +               else
> > +                       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)
> >  {
> 
> [...]
> 

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

[Fedor Pchelkin]

On Wed, 22. Oct 07:16, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > @@ -5849,6 +5852,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);
> 
> not sure if it's worth to initialize tx_rpt.sn to zero?

That shouldn't be needed because rtwdev is zero initialized in
rtw89_alloc_ieee80211_hw().  ieee80211_alloc_hw() fills the private
driver part with zeroes.

> > @@ -5484,6 +5488,26 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> >         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(&tx_rpt->queue.lock, flags);
> > +       skb_queue_walk_safe(&tx_rpt->queue, skb, tmp) {
> > +               skb_data = RTW89_TX_SKB_CB(skb);
> > +
> > +               /* skip if sequence number doesn't match */
> > +               if (sw_define != skb_data->tx_rpt_sn)
> > +                       continue;
> > +               /* skip if TX attempt has failed and retry limit has not been
> > +                * reached yet
> > +                */
> > +               if (tx_status != RTW89_TX_DONE &&
> > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> > +                       continue;
> > +
> > +               __skb_unlink(skb, &tx_rpt->queue);
> > +               rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
> 
> Would it be better to run rtw89_tx_rpt_tx_status() after this loop outside
> spin_lock()?

I don't have a strong opinion here: PCIe side implements the release
like

rtw89_pci_poll_rpq_dma()
 spin_lock_bh(&rtwpci->trx_lock)
 rtw89_pci_release_tx()
 ...
   rtw89_pci_release_txwd_skb()
	skb_queue_walk_safe(&txwd->queue, skb, tmp) {
		skb_unlink(skb, &txwd->queue);

		tx_data = RTW89_PCI_TX_SKB_CB(skb);
		dma_unmap_single(&rtwpci->pdev->dev, tx_data->dma, skb->len,
				 DMA_TO_DEVICE);

		rtw89_pci_tx_status(rtwdev, tx_ring, skb, tx_status);
	}
 ...
 spin_unlock_bh(&rtwpci->trx_lock)


Apart from bh/irqsave part the iteration over skbs looks visually similar
at the moment.

> > --- a/drivers/net/wireless/realtek/rtw89/usb.c
> > +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> > @@ -216,6 +216,14 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
> >                 skb_pull(skb, txdesc_size);
> > 
> >                 info = IEEE80211_SKB_CB(skb);
> > +               if (rtw89_is_tx_rpt_skb(skb)) {
> > +                       /* sequence number is passed to rtw89_mac_c2h_tx_rpt() via
> 
> nit: The 'via' is over 80 characters a little bit. Move to next line.
> 
> > +                        * driver data
> > +                        */
> > +                       skb_queue_tail(&rtwdev->tx_rpt.queue, skb);
> > +                       continue;
> > +               }
> > +
> >                 ieee80211_tx_info_clear_status(info);
> > 
> >                 if (urb->status == 0) {
> 
> Should we move this checking upward? Enqueue skb into tx_rpt_skb only if
> urb->status == 0?

Yep, I agree we can do it.  Just need to report it immediately with
RTW89_TX_MACID_DROP status.

As it currently stands, we should not receive notification from the
firmware for such skb so it would remain inside tx_rpt.queue until HCI
reset occurs.

However, I've not considered the case when the queue is full and we start
queueing skbs with duplicate sequence numbers - the overall range we have
is just 0xF, theoretically the situation is possible if the firmware
fatally crashes and doesn't provide notifications in time.  IMHO the TX
status will be the last thing we're going to be interested in when this
happens.  In the end, HCI reset during SER activity will purge the queue.

But the possibility of having skbs with duplicate seq numbers is not good.
Though I'm not sure if we can ever hit such a situation... Generally it
indicates that the firmware doesn't respond or performs very badly, and
we'd better reset it or something.  What do you think on this?

> 
> > @@ -372,6 +380,7 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> >  {
> >         struct rtw89_tx_desc_info *desc_info = &tx_req->desc_info;
> >         struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
> > +       struct rtw89_tx_skb_data *skb_data;
> >         struct sk_buff *skb = tx_req->skb;
> >         struct rtw89_txwd_body *txdesc;
> >         u32 txdesc_size;
> > @@ -398,6 +407,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> > 
> >         le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
> > 
> > +       skb_data = RTW89_TX_SKB_CB(skb);
> > +       skb_data->tx_rpt_sn = tx_req->desc_info.sn;
> 
> Shouldn't set skb_data->tx_pkt_cnt_lmt? 
> 
> skb_data->tx_pkt_cnt_lmt = tx_req->desc_info.tx_cnt_lmt;
> 
> Also, should we check desc_info.{report, tx_cnt_lmt_en} individually before 
> setting?

Right, this all makes sense.  Will fix it, thanks!

> 
> 
> > +
> >         skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> > 
> >         return 0;
> > @@ -678,7 +690,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);
> 
> Have you consider the SKB that has been rtw89_usb_write_port() but
> has not yet rtw89_usb_write_port_complete()?
> 
> Since we call rtw89_mac_pwr_off() before rtw89_hci_reset() in 
> rtw89_core_stop(), it should be not more C2H at rtw89_hci_reset().
> It seems to be safe, right?

Well, rtw89_usb_write_port_complete() is asynchronous URB callback managed
by USB subsystem and it's mainly independent of rtw89.  We're guaranteed
all pending URB completions will complete when the device is being
disconnected and rtw89_usb_disconnect() method is called.  That's all, I
think.

In other call sites like SER we risk URB completion be called after
purging the queue, the only consequence will be the obsolete skb still
added to the queue.

We can implement an anchor for TX URBs and explicitly wait with
usb_kill_anchored_urbs() in ->reset() until all pending URB completions
are done, and then purge the queue.

If nothing else, I'll add it in the next respin of the series.

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

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> On Wed, 22. Oct 07:16, Ping-Ke Shih wrote:
> > Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > @@ -5849,6 +5852,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);
> >
> > not sure if it's worth to initialize tx_rpt.sn to zero?
> 
> That shouldn't be needed because rtwdev is zero initialized in
> rtw89_alloc_ieee80211_hw().  ieee80211_alloc_hw() fills the private
> driver part with zeroes.

Ah. I mentioned this in wrong place. I meant that we can initialize tx_rpt.sn
in rtw89_core_start() or do it right after downloading firmware in
__rtw89_fw_download() like ' fw_info->h2c_seq = 0;'.

> 
> > > @@ -5484,6 +5488,26 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> > >         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(&tx_rpt->queue.lock, flags);
> > > +       skb_queue_walk_safe(&tx_rpt->queue, skb, tmp) {
> > > +               skb_data = RTW89_TX_SKB_CB(skb);
> > > +
> > > +               /* skip if sequence number doesn't match */
> > > +               if (sw_define != skb_data->tx_rpt_sn)
> > > +                       continue;
> > > +               /* skip if TX attempt has failed and retry limit has not been
> > > +                * reached yet
> > > +                */
> > > +               if (tx_status != RTW89_TX_DONE &&
> > > +                   data_txcnt != skb_data->tx_pkt_cnt_lmt)
> > > +                       continue;
> > > +
> > > +               __skb_unlink(skb, &tx_rpt->queue);
> > > +               rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
> >
> > Would it be better to run rtw89_tx_rpt_tx_status() after this loop outside
> > spin_lock()?
> 
> I don't have a strong opinion here: PCIe side implements the release
> like
> 
> rtw89_pci_poll_rpq_dma()
>  spin_lock_bh(&rtwpci->trx_lock)
>  rtw89_pci_release_tx()
>  ...
>    rtw89_pci_release_txwd_skb()
>         skb_queue_walk_safe(&txwd->queue, skb, tmp) {
>                 skb_unlink(skb, &txwd->queue);
> 
>                 tx_data = RTW89_PCI_TX_SKB_CB(skb);
>                 dma_unmap_single(&rtwpci->pdev->dev, tx_data->dma, skb->len,
>                                  DMA_TO_DEVICE);
> 
>                 rtw89_pci_tx_status(rtwdev, tx_ring, skb, tx_status);
>         }
>  ...
>  spin_unlock_bh(&rtwpci->trx_lock)
> 
> 
> Apart from bh/irqsave part the iteration over skbs looks visually similar
> at the moment.
> 
> > > --- a/drivers/net/wireless/realtek/rtw89/usb.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> > > @@ -216,6 +216,14 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
> > >                 skb_pull(skb, txdesc_size);
> > >
> > >                 info = IEEE80211_SKB_CB(skb);
> > > +               if (rtw89_is_tx_rpt_skb(skb)) {
> > > +                       /* sequence number is passed to rtw89_mac_c2h_tx_rpt() via
> >
> > nit: The 'via' is over 80 characters a little bit. Move to next line.
> >
> > > +                        * driver data
> > > +                        */
> > > +                       skb_queue_tail(&rtwdev->tx_rpt.queue, skb);
> > > +                       continue;
> > > +               }
> > > +
> > >                 ieee80211_tx_info_clear_status(info);
> > >
> > >                 if (urb->status == 0) {
> >
> > Should we move this checking upward? Enqueue skb into tx_rpt_skb only if
> > urb->status == 0?
> 
> Yep, I agree we can do it.  Just need to report it immediately with
> RTW89_TX_MACID_DROP status.
> 
> As it currently stands, we should not receive notification from the
> firmware for such skb so it would remain inside tx_rpt.queue until HCI
> reset occurs.
> 
> However, I've not considered the case when the queue is full and we start
> queueing skbs with duplicate sequence numbers - the overall range we have
> is just 0xF, theoretically the situation is possible if the firmware
> fatally crashes and doesn't provide notifications in time.  IMHO the TX
> status will be the last thing we're going to be interested in when this
> happens.  In the end, HCI reset during SER activity will purge the queue.
> 
> But the possibility of having skbs with duplicate seq numbers is not good.
> Though I'm not sure if we can ever hit such a situation... Generally it
> indicates that the firmware doesn't respond or performs very badly, and
> we'd better reset it or something.  What do you think on this?

Maybe we can maintain a bitmap to detect duplicate seq numbers. Also, we can
search for a SKB according to tx_report.sn in constant time instead of doing
sequential search in a list. Then we can detect and remove duplicate seq
numbers at TX side, if a tx_report.sn has been existing in the bitmap.

> 
> >
> > > @@ -372,6 +380,7 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> > >  {
> > >         struct rtw89_tx_desc_info *desc_info = &tx_req->desc_info;
> > >         struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
> > > +       struct rtw89_tx_skb_data *skb_data;
> > >         struct sk_buff *skb = tx_req->skb;
> > >         struct rtw89_txwd_body *txdesc;
> > >         u32 txdesc_size;
> > > @@ -398,6 +407,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> > >
> > >         le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
> > >
> > > +       skb_data = RTW89_TX_SKB_CB(skb);
> > > +       skb_data->tx_rpt_sn = tx_req->desc_info.sn;
> >
> > Shouldn't set skb_data->tx_pkt_cnt_lmt?
> >
> > skb_data->tx_pkt_cnt_lmt = tx_req->desc_info.tx_cnt_lmt;
> >
> > Also, should we check desc_info.{report, tx_cnt_lmt_en} individually before
> > setting?
> 
> Right, this all makes sense.  Will fix it, thanks!
> 
> >
> >
> > > +
> > >         skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> > >
> > >         return 0;
> > > @@ -678,7 +690,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);
> >
> > Have you consider the SKB that has been rtw89_usb_write_port() but
> > has not yet rtw89_usb_write_port_complete()?
> >
> > Since we call rtw89_mac_pwr_off() before rtw89_hci_reset() in
> > rtw89_core_stop(), it should be not more C2H at rtw89_hci_reset().
> > It seems to be safe, right?
> 
> Well, rtw89_usb_write_port_complete() is asynchronous URB callback managed
> by USB subsystem and it's mainly independent of rtw89.  We're guaranteed
> all pending URB completions will complete when the device is being
> disconnected and rtw89_usb_disconnect() method is called.  That's all, I
> think.
> 
> In other call sites like SER we risk URB completion be called after
> purging the queue, the only consequence will be the obsolete skb still
> added to the queue.
> 
> We can implement an anchor for TX URBs and explicitly wait with
> usb_kill_anchored_urbs() in ->reset() until all pending URB completions
> are done, and then purge the queue.
> 
> If nothing else, I'll add it in the next respin of the series.

Good to me. Additionally, I'd like to add a comment right after hci->reset
to explicitly point out we should complete all tx_wait, such as

static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
{
	rtwdev->hci.ops->reset(rtwdev);
    /* hci.ops->reset must complete tx_wait of all pending TX SKB */
	rtw89_tx_wait_list_clear(rtwdev);
}

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

[Fedor Pchelkin]

On Mon, 27. Oct 01:14, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > On Wed, 22. Oct 07:16, Ping-Ke Shih wrote:
> > > Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > > @@ -5849,6 +5852,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);
> > >
> > > not sure if it's worth to initialize tx_rpt.sn to zero?
> > 
> > That shouldn't be needed because rtwdev is zero initialized in
> > rtw89_alloc_ieee80211_hw().  ieee80211_alloc_hw() fills the private
> > driver part with zeroes.
> 
> Ah. I mentioned this in wrong place. I meant that we can initialize tx_rpt.sn
> in rtw89_core_start() or do it right after downloading firmware in
> __rtw89_fw_download() like ' fw_info->h2c_seq = 0;'.

To my mind, it's not worth adding extra code to initialize tx_rpt.sn to
zero at some point as it's just a sequential number in [0x0, 0xF] range,
which is replayed to firmware and used to synchronize with it.  Actually
we can start counting from 0x1 or 0xA, it doesn't really matter to care
about counter initialization.