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

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

[Fedor Pchelkin]

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

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.

Sorry for the inconvenience with the timing when the series is sent.  It's
not extremely urgent, and I'd gladly appreciate if it'd be reviewed for
any issues I'm not aware of.  Testing with other USB chips would be great,
too.  Thanks!

Fedor Pchelkin (6):
  wifi: rtw89: usb: fix leak in rtw89_usb_rx_handler()
  wifi: rtw89: usb: fix leak in rtw89_usb_write_port()
  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
  wifi: rtw89: forcefully clear TX wait list on HCI reset

 drivers/net/wireless/realtek/rtw89/core.c | 23 +++++++--
 drivers/net/wireless/realtek/rtw89/core.h | 44 ++++++++++++++--
 drivers/net/wireless/realtek/rtw89/fw.h   |  5 ++
 drivers/net/wireless/realtek/rtw89/mac.c  | 63 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  |  9 ++++
 drivers/net/wireless/realtek/rtw89/pci.c  |  1 +
 drivers/net/wireless/realtek/rtw89/pci.h  |  4 --
 drivers/net/wireless/realtek/rtw89/txrx.h |  2 +
 drivers/net/wireless/realtek/rtw89/usb.c  | 35 +++++++++++--
 drivers/net/wireless/realtek/rtw89/usb.h  | 15 ++++++
 10 files changed, 186 insertions(+), 15 deletions(-)

-- 
2.51.0

[PATCH rtw-next 1/6] wifi: rtw89: usb: fix leak in rtw89_usb_rx_handler()

[Fedor Pchelkin]

Free allocated skb on the error handling path.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 6cf89aee252e..3435599f4740 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -422,6 +422,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);
+			dev_kfree_skb_any(rx_skb);
 			continue;
 		}
 
-- 
2.51.0

[PATCH rtw-next 2/6] 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>
---
 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 3435599f4740..bc0d5e48d39b 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 3/6] 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.  Do 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>
---
 drivers/net/wireless/realtek/rtw89/core.c | 12 +++++++++++-
 drivers/net/wireless/realtek/rtw89/core.h |  2 ++
 drivers/net/wireless/realtek/rtw89/fw.h   |  5 +++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 23 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h  |  9 +++++++++
 drivers/net/wireless/realtek/rtw89/txrx.h |  2 ++
 6 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 917b2adede61..d2a559ddfa2e 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1420,11 +1420,20 @@ 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)
+{
+	bool rpt_en = desc_info->report;
+	u32 dword = FIELD_PREP(RTW89_TXWD_INFO3_SPE_RPT, rpt_en);
+
+	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 +1456,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);
 
 }
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 928c8c84c964..2362724323a9 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -1167,6 +1167,8 @@ struct rtw89_tx_desc_info {
 	u8 ampdu_density;
 	u8 ampdu_num;
 	bool sec_en;
+	bool report;
+	u8 sn;
 	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..f196088a8316 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.h
+++ b/drivers/net/wireless/realtek/rtw89/fw.h
@@ -3747,6 +3747,11 @@ 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))
 
+#define RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h) \
+	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 6))
+#define RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h) \
+	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(12, 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..01afdcd5f36c 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5457,6 +5457,17 @@ 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 = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
+	u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
+
+	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+		    "C2H TX RPT: sn %d, tx_status %d\n",
+		    sw_define, tx_status);
+}
+
 static void
 rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 {
@@ -5691,6 +5702,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 +5794,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 +5831,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..632b85aed032 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -432,6 +432,14 @@ enum rtw89_mac_c2h_mcc_func {
 	NUM_OF_RTW89_MAC_C2H_FUNC_MCC,
 };
 
+enum rtw89_mac_c2h_misc_func {
+	RTW89_MAC_C2H_FUNC_WPS_RPT,
+	RTW89_MAC_C2H_FUNC_TX_REPORT,
+	RTW89_MAC_C2H_FUNC_BF_SENS_FEEDBACK = 0x4,
+
+	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 +478,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..d7259e6d798e 100644
--- a/drivers/net/wireless/realtek/rtw89/txrx.h
+++ b/drivers/net/wireless/realtek/rtw89/txrx.h
@@ -139,8 +139,10 @@ 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_SW_DEFINE GENMASK(3, 0)
 #define RTW89_TXWD_INFO4_RTS_EN BIT(27)
 #define RTW89_TXWD_INFO4_HW_RTS_EN BIT(31)
 
-- 
2.51.0

[PATCH rtw-next 4/6] 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 handler which will initialize the
corresponding values used later when writing to TX descriptor.  The
handler is a no-op for PCIe, TX status reporting behaviour doesn't change
there.

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 (done at core deinitialization phase, too).

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/core.c |  5 ++++
 drivers/net/wireless/realtek/rtw89/core.h | 18 ++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c  | 29 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/pci.c  |  1 +
 drivers/net/wireless/realtek/rtw89/pci.h  |  4 ----
 drivers/net/wireless/realtek/rtw89/usb.c  | 18 ++++++++++++++
 drivers/net/wireless/realtek/rtw89/usb.h  | 15 ++++++++++++
 7 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index d2a559ddfa2e..3e7bd0cedbdf 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1229,6 +1229,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
 	struct ieee80211_sta *sta = rtwsta_link_to_sta_safe(rtwsta_link);
 	struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link);
 	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct rtw89_vif *rtwvif = rtwvif_link->rtwvif;
 	struct rtw89_core_tx_request tx_req = {};
 	int ret;
@@ -1240,6 +1241,9 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
 	tx_req.rtwsta_link = rtwsta_link;
 	tx_req.desc_info.sw_mld = sw_mld;
 
+	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+		rtw89_hci_tx_rpt_enable(rtwdev, &tx_req);
+
 	rtw89_traffic_stats_accu(rtwdev, rtwvif, skb, true, true);
 	rtw89_wow_parse_akm(rtwdev, skb);
 	rtw89_core_tx_update_desc_info(rtwdev, &tx_req);
@@ -5839,6 +5843,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 2362724323a9..4e597a5df005 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -3509,6 +3509,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;
@@ -3646,6 +3651,8 @@ struct rtw89_hci_ops {
 	void (*pause)(struct rtw89_dev *rtwdev, bool pause);
 	void (*switch_mode)(struct rtw89_dev *rtwdev, bool low_power);
 	void (*recalc_int_mit)(struct rtw89_dev *rtwdev);
+	void (*tx_rpt_enable)(struct rtw89_dev *rtwdev,
+			      struct rtw89_core_tx_request *tx_req);
 
 	u8 (*read8)(struct rtw89_dev *rtwdev, u32 addr);
 	u16 (*read16)(struct rtw89_dev *rtwdev, u32 addr);
@@ -6008,6 +6015,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;
@@ -6294,6 +6304,7 @@ static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
 {
 	rtwdev->hci.ops->reset(rtwdev);
 	rtw89_tx_wait_list_clear(rtwdev);
+	skb_queue_purge(&rtwdev->tx_rpt_queue);
 }
 
 static inline int rtw89_hci_start(struct rtw89_dev *rtwdev)
@@ -6336,6 +6347,13 @@ static inline void rtw89_hci_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 	return rtwdev->hci.ops->tx_kick_off(rtwdev, txch);
 }
 
+static inline void rtw89_hci_tx_rpt_enable(struct rtw89_dev *rtwdev,
+					   struct rtw89_core_tx_request *tx_req)
+{
+	if (rtwdev->hci.ops->tx_rpt_enable)
+		rtwdev->hci.ops->tx_rpt_enable(rtwdev, tx_req);
+}
+
 static inline int rtw89_hci_mac_pre_deinit(struct rtw89_dev *rtwdev)
 {
 	return rtwdev->hci.ops->mac_pre_deinit(rtwdev);
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 01afdcd5f36c..831e53aedccc 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5457,15 +5457,44 @@ 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_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, bool acked)
+{
+	struct ieee80211_tx_info *info;
+
+	info = IEEE80211_SKB_CB(skb);
+	ieee80211_tx_info_clear_status(info);
+	if (acked)
+		info->flags |= IEEE80211_TX_STAT_ACK;
+	else
+		info->flags &= ~IEEE80211_TX_STAT_ACK;
+
+	ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
+}
+
 static void
 rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 {
 	u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
 	u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
+	struct sk_buff *cur, *tmp;
+	unsigned long flags;
+	u8 *n;
 
 	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
 		    "C2H TX RPT: sn %d, tx_status %d\n",
 		    sw_define, tx_status);
+
+	spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
+	skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
+		n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;
+		if (*n == sw_define) {
+			__skb_unlink(cur, &rtwdev->tx_rpt_queue);
+			rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status == RTW89_TX_DONE);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
 }
 
 static void
diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index 0ee5f8579447..fdf142d77ecc 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -4675,6 +4675,7 @@ static const struct rtw89_hci_ops rtw89_pci_ops = {
 	.pause		= rtw89_pci_ops_pause,
 	.switch_mode	= rtw89_pci_ops_switch_mode,
 	.recalc_int_mit = rtw89_pci_recalc_int_mit,
+	.tx_rpt_enable  = NULL, /* always enabled */
 
 	.read8		= rtw89_pci_ops_read8,
 	.read16		= rtw89_pci_ops_read16,
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)
 
diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index bc0d5e48d39b..98eff955fc96 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -188,6 +188,13 @@ static u8 rtw89_usb_get_bulkout_id(u8 ch_dma)
 	}
 }
 
+static void rtw89_usb_ops_tx_rpt_enable(struct rtw89_dev *rtwdev,
+					struct rtw89_core_tx_request *tx_req)
+{
+	tx_req->desc_info.report = true;
+	tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
+}
+
 static void rtw89_usb_write_port_complete(struct urb *urb)
 {
 	struct rtw89_usb_tx_ctrl_block *txcb = urb->context;
@@ -216,6 +223,12 @@ 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);
+			continue;
+		}
+
 		ieee80211_tx_info_clear_status(info);
 
 		if (urb->status == 0) {
@@ -364,6 +377,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 sk_buff *skb = tx_req->skb;
+	struct rtw89_usb_tx_data *tx_data;
 	struct rtw89_txwd_body *txdesc;
 	u32 txdesc_size;
 
@@ -387,6 +401,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
 	memset(txdesc, 0, txdesc_size);
 	rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
 
+	tx_data = RTW89_USB_TX_SKB_CB(skb);
+	tx_data->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);
@@ -811,6 +828,7 @@ static const struct rtw89_hci_ops rtw89_usb_ops = {
 	.pause		= rtw89_usb_ops_pause,
 	.switch_mode	= rtw89_usb_ops_switch_mode,
 	.recalc_int_mit = rtw89_usb_ops_recalc_int_mit,
+	.tx_rpt_enable  = rtw89_usb_ops_tx_rpt_enable,
 
 	.read8		= rtw89_usb_ops_read8,
 	.read16		= rtw89_usb_ops_read16,
diff --git a/drivers/net/wireless/realtek/rtw89/usb.h b/drivers/net/wireless/realtek/rtw89/usb.h
index c1b4bfa20979..c8b2ad2eaa22 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.h
+++ b/drivers/net/wireless/realtek/rtw89/usb.h
@@ -53,11 +53,26 @@ struct rtw89_usb {
 	struct sk_buff_head tx_queue[RTW89_TXCH_NUM];
 };
 
+struct rtw89_usb_tx_data {
+	u8 sn;
+};
+
 static inline struct rtw89_usb *rtw89_usb_priv(struct rtw89_dev *rtwdev)
 {
 	return (struct rtw89_usb *)rtwdev->priv;
 }
 
+static inline struct rtw89_usb_tx_data *RTW89_USB_TX_SKB_CB(struct sk_buff *skb)
+{
+	struct rtw89_tx_skb_data *data = RTW89_TX_SKB_CB(skb);
+
+	BUILD_BUG_ON(sizeof(struct rtw89_tx_skb_data) +
+		     sizeof(struct rtw89_usb_tx_data) >
+		     sizeof_field(struct ieee80211_tx_info, driver_data));
+
+	return (struct rtw89_usb_tx_data *)data->hci_priv;
+}
+
 int rtw89_usb_probe(struct usb_interface *intf,
 		    const struct usb_device_id *id);
 void rtw89_usb_disconnect(struct usb_interface *intf);
-- 
2.51.0

[PATCH rtw-next 5/6] 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 a sequence number in a TX wait object so that it'll be possible to
identify completed items inside C2H handler.  No need to add the
corresponding skb to the &txcb->tx_ack_queue on USB part.

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/wireless/realtek/rtw89/core.c |  6 ++++--
 drivers/net/wireless/realtek/rtw89/core.h | 18 +++++++++++++++++-
 drivers/net/wireless/realtek/rtw89/mac.c  | 11 +++++++++++
 drivers/net/wireless/realtek/rtw89/usb.c  |  9 ++++++++-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 3e7bd0cedbdf..e76f04736502 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1161,18 +1161,19 @@ int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *sk
 
 	lockdep_assert_wiphy(rtwdev->hw->wiphy);
 
+	list_add_tail_rcu(&wait->list, &rtwdev->tx_waits);
 	rtw89_core_tx_kick_off(rtwdev, qsel);
 	time_left = wait_for_completion_timeout(&wait->completion,
 						msecs_to_jiffies(timeout));
 
 	if (time_left == 0) {
 		ret = -ETIMEDOUT;
-		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 {
 		if (!wait->tx_done)
 			ret = -EAGAIN;
+		list_del_rcu(&wait->list);
 		rtw89_tx_wait_release(wait);
 	}
 
@@ -1237,11 +1238,12 @@ 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;
 
-	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
+	if (wait || (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS))
 		rtw89_hci_tx_rpt_enable(rtwdev, &tx_req);
 
 	rtw89_traffic_stats_accu(rtwdev, rtwvif, skb, true, true);
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index 4e597a5df005..e7948bd0bdf6 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -1199,6 +1199,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;
@@ -3521,6 +3522,7 @@ struct rtw89_tx_wait_info {
 	struct completion completion;
 	struct sk_buff *skb;
 	bool tx_done;
+	u8 sn;
 };
 
 struct rtw89_tx_skb_data {
@@ -6289,7 +6291,7 @@ static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev)
 	list_for_each_entry_safe(wait, tmp, &rtwdev->tx_waits, list) {
 		if (!completion_done(&wait->completion))
 			continue;
-		list_del(&wait->list);
+		list_del_rcu(&wait->list);
 		rtw89_tx_wait_release(wait);
 	}
 }
@@ -7392,6 +7394,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,
 					       bool tx_done)
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 831e53aedccc..79409eb4d028 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -5477,6 +5477,7 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 {
 	u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
 	u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
+	struct rtw89_tx_wait_info *wait;
 	struct sk_buff *cur, *tmp;
 	unsigned long flags;
 	u8 *n;
@@ -5485,6 +5486,16 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
 		    "C2H TX RPT: sn %d, tx_status %d\n",
 		    sw_define, tx_status);
 
+	rcu_read_lock();
+	list_for_each_entry_rcu(wait, &rtwdev->tx_waits, list) {
+		if (wait->sn == sw_define) {
+			wait->tx_done = tx_status == RTW89_TX_DONE;
+			complete_all(&wait->completion);
+			break;
+		}
+	}
+	rcu_read_unlock();
+
 	spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
 	skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
 		n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;
diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
index 98eff955fc96..342c05227191 100644
--- a/drivers/net/wireless/realtek/rtw89/usb.c
+++ b/drivers/net/wireless/realtek/rtw89/usb.c
@@ -191,8 +191,13 @@ static u8 rtw89_usb_get_bulkout_id(u8 ch_dma)
 static void rtw89_usb_ops_tx_rpt_enable(struct rtw89_dev *rtwdev,
 					struct rtw89_core_tx_request *tx_req)
 {
+	struct rtw89_tx_wait_info *wait = tx_req->wait;
+
 	tx_req->desc_info.report = true;
 	tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
+
+	if (wait)
+		wait->sn = tx_req->desc_info.sn;
 }
 
 static void rtw89_usb_write_port_complete(struct urb *urb)
@@ -313,7 +318,9 @@ static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
 		txcb->txch = txch;
 		skb_queue_head_init(&txcb->tx_ack_queue);
 
-		skb_queue_tail(&txcb->tx_ack_queue, skb);
+		/* tx_wait skbs are completed in rtw89_mac_c2h_tx_rpt() */
+		if (!rtw89_core_is_tx_wait(rtwdev, RTW89_TX_SKB_CB(skb)))
+			skb_queue_tail(&txcb->tx_ack_queue, skb);
 
 		ret = rtw89_usb_write_port(rtwdev, txch, skb->data, skb->len,
 					   txcb);
-- 
2.51.0

[PATCH rtw-next 6/6] wifi: rtw89: forcefully clear TX wait list on HCI reset

[Fedor Pchelkin]

TX status reporting based on firmware messages does not necessarily happen
when an HCI reset occurs, in contrast to RPP based one where pending skbs
are forcefully flushed, see rtw89_pci_release_txwd_skb().

So for the former case, if completion from the firmware doesn't happen, TX
wait objects are wastefully piled up in the list and not released.
Forcefully clear TX wait list on HCI reset then.

It's okay since wiphy lock is held during HCI reset.  For the RPP case,
all pending completions were done just before in ->reset callback and no
new ones can appear.  For the C2H message case, RCU access to the list
helps.

Found by Linux Verification Center (linuxtesting.org).

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

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index e76f04736502..3a0388d3acbf 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1140,7 +1140,7 @@ 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);
+	rtw89_tx_wait_list_clear(rtwdev, false);
 }
 
 void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index e7948bd0bdf6..0ad871472e79 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -6282,14 +6282,14 @@ static inline void rtw89_tx_wait_release(struct rtw89_tx_wait_info *wait)
 	kfree_rcu(wait, rcu_head);
 }
 
-static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev)
+static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev, bool force)
 {
 	struct rtw89_tx_wait_info *wait, *tmp;
 
 	lockdep_assert_wiphy(rtwdev->hw->wiphy);
 
 	list_for_each_entry_safe(wait, tmp, &rtwdev->tx_waits, list) {
-		if (!completion_done(&wait->completion))
+		if (!force && !completion_done(&wait->completion))
 			continue;
 		list_del_rcu(&wait->list);
 		rtw89_tx_wait_release(wait);
@@ -6305,7 +6305,7 @@ static inline int rtw89_hci_tx_write(struct rtw89_dev *rtwdev,
 static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
 {
 	rtwdev->hci.ops->reset(rtwdev);
-	rtw89_tx_wait_list_clear(rtwdev);
+	rtw89_tx_wait_list_clear(rtwdev, true);
 	skb_queue_purge(&rtwdev->tx_rpt_queue);
 }
 
-- 
2.51.0

RE: [PATCH rtw-next 0/6] wifi: rtw89: improvements for USB part

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> The first two patches concern memory leak issues found during testing.
> 
> 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.
> 
> Sorry for the inconvenience with the timing when the series is sent.  It's
> not extremely urgent, and I'd gladly appreciate if it'd be reviewed for
> any issues I'm not aware of.  Testing with other USB chips would be great,
> too.  Thanks!

Thanks for your patches and description that I understand what you are going
to do. I'd like give people enough time to review this patchset, so I don't
merge this immediately, and also I can review them deeply days later.

Re: [PATCH rtw-next 3/6] wifi: rtw89: implement C2H TX report handler

[Bitterblue Smith]

On 20/09/2025 16:26, Fedor Pchelkin 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.  Do 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>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 12 +++++++++++-
>  drivers/net/wireless/realtek/rtw89/core.h |  2 ++
>  drivers/net/wireless/realtek/rtw89/fw.h   |  5 +++++
>  drivers/net/wireless/realtek/rtw89/mac.c  | 23 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.h  |  9 +++++++++
>  drivers/net/wireless/realtek/rtw89/txrx.h |  2 ++
>  6 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 917b2adede61..d2a559ddfa2e 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1420,11 +1420,20 @@ 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)
> +{
> +	bool rpt_en = desc_info->report;
> +	u32 dword = FIELD_PREP(RTW89_TXWD_INFO3_SPE_RPT, rpt_en);
> +
> +	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 +1456,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);
>  
>  }
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 928c8c84c964..2362724323a9 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -1167,6 +1167,8 @@ struct rtw89_tx_desc_info {
>  	u8 ampdu_density;
>  	u8 ampdu_num;
>  	bool sec_en;
> +	bool report;
> +	u8 sn;
>  	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..f196088a8316 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.h
> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
> @@ -3747,6 +3747,11 @@ 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))
>  
> +#define RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h) \
> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 6))
> +#define RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h) \
> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(12, 8))

This is only 4 bits:

#define TXCCXRPT_SW_DEFINE_SH		8
#define TXCCXRPT_SW_DEFINE_MSK		0xf


The rest of the series looks good to me. (I don't know much about
the RCU stuff.) I will test this tomorrow.

> +
>  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..01afdcd5f36c 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,6 +5457,17 @@ 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 = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
> +	u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> +
> +	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> +		    "C2H TX RPT: sn %d, tx_status %d\n",
> +		    sw_define, tx_status);
> +}
> +
>  static void
>  rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
> @@ -5691,6 +5702,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 +5794,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 +5831,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..632b85aed032 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -432,6 +432,14 @@ enum rtw89_mac_c2h_mcc_func {
>  	NUM_OF_RTW89_MAC_C2H_FUNC_MCC,
>  };
>  
> +enum rtw89_mac_c2h_misc_func {
> +	RTW89_MAC_C2H_FUNC_WPS_RPT,
> +	RTW89_MAC_C2H_FUNC_TX_REPORT,
> +	RTW89_MAC_C2H_FUNC_BF_SENS_FEEDBACK = 0x4,
> +
> +	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 +478,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..d7259e6d798e 100644
> --- a/drivers/net/wireless/realtek/rtw89/txrx.h
> +++ b/drivers/net/wireless/realtek/rtw89/txrx.h
> @@ -139,8 +139,10 @@ 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_SW_DEFINE GENMASK(3, 0)
>  #define RTW89_TXWD_INFO4_RTS_EN BIT(27)
>  #define RTW89_TXWD_INFO4_HW_RTS_EN BIT(31)
>  

RE: [PATCH rtw-next 1/6] wifi: rtw89: usb: fix leak in rtw89_usb_rx_handler()

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> Free allocated skb on the error handling path.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>  drivers/net/wireless/realtek/rtw89/usb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index 6cf89aee252e..3435599f4740 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -422,6 +422,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);
> +                       dev_kfree_skb_any(rx_skb);
>                         continue;
>                 }

I feel we should goto free_or_reuse like below:

free_or_reuse:
		if (skb_queue_len(&rtwusb->rx_free_queue) >= RTW89_USB_RX_SKB_NUM)
			dev_kfree_skb_any(rx_skb);
		else
			skb_queue_tail(&rtwusb->rx_free_queue, rx_skb);

If just free skb, rtwusb->rx_free_queue might be starved.

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

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> 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 3435599f4740..bc0d5e48d39b 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);

By the way, during I review this function, txcb->tx_ack_queue is a
struct sk_buff_head, how about just struct sk_buff *skb? 
(I might ask Bitterblue Smith about this).

More, since skb here is from mac80211, so ieee80211_free_txskb() would be 
more suitable rather than dev_kfree_skb_any()?

>                         kfree(txcb);
> --
> 2.51.0
> 

RE: [PATCH rtw-next 3/6] 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.  Do this according to the vendor driver for
                            ^^ nit: two spaces

> 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>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 12 +++++++++++-
>  drivers/net/wireless/realtek/rtw89/core.h |  2 ++
>  drivers/net/wireless/realtek/rtw89/fw.h   |  5 +++++
>  drivers/net/wireless/realtek/rtw89/mac.c  | 23 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.h  |  9 +++++++++
>  drivers/net/wireless/realtek/rtw89/txrx.h |  2 ++
>  6 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 917b2adede61..d2a559ddfa2e 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1420,11 +1420,20 @@ 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)
> +{
> +       bool rpt_en = desc_info->report;
> +       u32 dword = FIELD_PREP(RTW89_TXWD_INFO3_SPE_RPT, rpt_en);

just:
	      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 +1456,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);
> 
>  }
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 928c8c84c964..2362724323a9 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -1167,6 +1167,8 @@ struct rtw89_tx_desc_info {
>         u8 ampdu_density;
>         u8 ampdu_num;
>         bool sec_en;
> +       bool report;
> +       u8 sn;

Since you limit this to 4 bits by:
   tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;

How about just 'u8 sn: 4' here?

>         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..f196088a8316 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.h
> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
> @@ -3747,6 +3747,11 @@ 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))
> 
> +#define RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h) \
> +       le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 6))
> +#define RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h) \
> +       le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(12, 8))
> +

This is old style we don't prefer now.

Please define a struct and masks, and the consumer use le32_get_bits() to
get the values. See rtw89_fw_c2h_parse_attr() for example.

>  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..01afdcd5f36c 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,6 +5457,17 @@ 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 = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
> +       u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> +
> +       rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> +                   "C2H TX RPT: sn %d, tx_status %d\n",
> +                   sw_define, tx_status);
> +}
> +
>  static void
>  rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
> @@ -5691,6 +5702,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,
> +};
> +

RTW89_MAC_C2H_FUNC_TX_REPORT is 1, so the size of rtw89_mac_c2h_misc_handler[] is 2.

But, below you check ' if (func < NUM_OF_RTW89_MAC_C2H_FUNC_MISC)', where
NUM_OF_RTW89_MAC_C2H_FUNC_MISC is 5. 

I prefer just defining used enum. Remove RTW89_MAC_C2H_FUNC_WPS_RPT and 
RTW89_MAC_C2H_FUNC_BF_SENS_FEEDBACK.


>  static
>  void (* const rtw89_mac_c2h_mlo_handler[])(struct rtw89_dev *rtwdev,
>                                            struct sk_buff *c2h, u32 len) = {
> @@ -5777,6 +5794,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 +5831,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..632b85aed032 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -432,6 +432,14 @@ enum rtw89_mac_c2h_mcc_func {
>         NUM_OF_RTW89_MAC_C2H_FUNC_MCC,
>  };
> 
> +enum rtw89_mac_c2h_misc_func {
> +       RTW89_MAC_C2H_FUNC_WPS_RPT,
> +       RTW89_MAC_C2H_FUNC_TX_REPORT,
> +       RTW89_MAC_C2H_FUNC_BF_SENS_FEEDBACK = 0x4,
> +
> +       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 +478,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..d7259e6d798e 100644
> --- a/drivers/net/wireless/realtek/rtw89/txrx.h
> +++ b/drivers/net/wireless/realtek/rtw89/txrx.h
> @@ -139,8 +139,10 @@ 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_SW_DEFINE GENMASK(3, 0)
>  #define RTW89_TXWD_INFO4_RTS_EN BIT(27)
>  #define RTW89_TXWD_INFO4_HW_RTS_EN BIT(31)

We also have rtw89_core_fill_txdesc_v1() and rtw89_core_fill_txdesc_v2().
If you have looked up the fields already, we can define them by this patch.

Re: [PATCH rtw-next 3/6] wifi: rtw89: implement C2H TX report handler

[Bitterblue Smith]

On 24/09/2025 01:12, Bitterblue Smith wrote:
> On 20/09/2025 16:26, Fedor Pchelkin 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.  Do 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>
>> ---
>>  drivers/net/wireless/realtek/rtw89/core.c | 12 +++++++++++-
>>  drivers/net/wireless/realtek/rtw89/core.h |  2 ++
>>  drivers/net/wireless/realtek/rtw89/fw.h   |  5 +++++
>>  drivers/net/wireless/realtek/rtw89/mac.c  | 23 +++++++++++++++++++++++
>>  drivers/net/wireless/realtek/rtw89/mac.h  |  9 +++++++++
>>  drivers/net/wireless/realtek/rtw89/txrx.h |  2 ++
>>  6 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
>> index 917b2adede61..d2a559ddfa2e 100644
>> --- a/drivers/net/wireless/realtek/rtw89/core.c
>> +++ b/drivers/net/wireless/realtek/rtw89/core.c
>> @@ -1420,11 +1420,20 @@ 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)
>> +{
>> +	bool rpt_en = desc_info->report;
>> +	u32 dword = FIELD_PREP(RTW89_TXWD_INFO3_SPE_RPT, rpt_en);
>> +
>> +	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 +1456,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);
>>  
>>  }
>> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
>> index 928c8c84c964..2362724323a9 100644
>> --- a/drivers/net/wireless/realtek/rtw89/core.h
>> +++ b/drivers/net/wireless/realtek/rtw89/core.h
>> @@ -1167,6 +1167,8 @@ struct rtw89_tx_desc_info {
>>  	u8 ampdu_density;
>>  	u8 ampdu_num;
>>  	bool sec_en;
>> +	bool report;
>> +	u8 sn;
>>  	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..f196088a8316 100644
>> --- a/drivers/net/wireless/realtek/rtw89/fw.h
>> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
>> @@ -3747,6 +3747,11 @@ 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))
>>  
>> +#define RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h) \
>> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 6))
>> +#define RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h) \
>> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(12, 8))
> 
> This is only 4 bits:
> 
> #define TXCCXRPT_SW_DEFINE_SH		8
> #define TXCCXRPT_SW_DEFINE_MSK		0xf
> 
> 
> The rest of the series looks good to me. (I don't know much about
> the RCU stuff.) I will test this tomorrow.
> 

Actually, I found this in my notes:

"how to get just one tx report for each request? currently it seems
to provide a report for each transmission attempt. how is the vendor
driver coping with that?"

I think your code doesn't account for this.

Sorry I forgot about this detail. This behaviour is new in rtw89.
The chips supported by rtw88 provide only one report for each request.

>> +
>>  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..01afdcd5f36c 100644
>> --- a/drivers/net/wireless/realtek/rtw89/mac.c
>> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
>> @@ -5457,6 +5457,17 @@ 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 = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
>> +	u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
>> +
>> +	rtw89_debug(rtwdev, RTW89_DBG_TXRX,
>> +		    "C2H TX RPT: sn %d, tx_status %d\n",
>> +		    sw_define, tx_status);
>> +}
>> +
>>  static void
>>  rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>>  {
>> @@ -5691,6 +5702,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 +5794,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 +5831,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..632b85aed032 100644
>> --- a/drivers/net/wireless/realtek/rtw89/mac.h
>> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
>> @@ -432,6 +432,14 @@ enum rtw89_mac_c2h_mcc_func {
>>  	NUM_OF_RTW89_MAC_C2H_FUNC_MCC,
>>  };
>>  
>> +enum rtw89_mac_c2h_misc_func {
>> +	RTW89_MAC_C2H_FUNC_WPS_RPT,
>> +	RTW89_MAC_C2H_FUNC_TX_REPORT,
>> +	RTW89_MAC_C2H_FUNC_BF_SENS_FEEDBACK = 0x4,
>> +
>> +	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 +478,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..d7259e6d798e 100644
>> --- a/drivers/net/wireless/realtek/rtw89/txrx.h
>> +++ b/drivers/net/wireless/realtek/rtw89/txrx.h
>> @@ -139,8 +139,10 @@ 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_SW_DEFINE GENMASK(3, 0)
>>  #define RTW89_TXWD_INFO4_RTS_EN BIT(27)
>>  #define RTW89_TXWD_INFO4_HW_RTS_EN BIT(31)
>>  
> 

RE: [PATCH rtw-next 4/6] 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.
                 ^^ nit: two spaces
> 
> 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
                                         ^^ nit: two spaces

I wonder if you want two spaces intentionally? 

> scheme based on processing C2H messages.
> 
> Thus define a .tx_rpt_enable handler which will initialize the
> corresponding values used later when writing to TX descriptor.  The
> handler is a no-op for PCIe, TX status reporting behaviour doesn't change
> there.
> 
> 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 (done at core deinitialization phase, too).
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c |  5 ++++
>  drivers/net/wireless/realtek/rtw89/core.h | 18 ++++++++++++++
>  drivers/net/wireless/realtek/rtw89/mac.c  | 29 +++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw89/pci.c  |  1 +
>  drivers/net/wireless/realtek/rtw89/pci.h  |  4 ----
>  drivers/net/wireless/realtek/rtw89/usb.c  | 18 ++++++++++++++
>  drivers/net/wireless/realtek/rtw89/usb.h  | 15 ++++++++++++
>  7 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index d2a559ddfa2e..3e7bd0cedbdf 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1229,6 +1229,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
>         struct ieee80211_sta *sta = rtwsta_link_to_sta_safe(rtwsta_link);
>         struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link);
>         struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>         struct rtw89_vif *rtwvif = rtwvif_link->rtwvif;
>         struct rtw89_core_tx_request tx_req = {};
>         int ret;
> @@ -1240,6 +1241,9 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
>         tx_req.rtwsta_link = rtwsta_link;
>         tx_req.desc_info.sw_mld = sw_mld;
> 
> +       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> +               rtw89_hci_tx_rpt_enable(rtwdev, &tx_req);
> +

Normally, we update TX description in rtw89_core_tx_update_desc_info(), and
rtw89_hci_tx_rpt_enable() can be implemented in common flow (but give proper
name). And add a flag in rtw89_hci_info to determine if we should enable
TX report.

By the way, I'm thinking if add a `struct rtw89_hci_attr` as HCI common
attributes for common flow. 

Another is that rtw89_core_tx_update_desc_info() is a common entry to all
TX types, but most conditions (including we are going to add) are unnecessary
to RTW89_CORE_TX_TYPE_FWCMD. I also plan to refine this.

Anyway, just add what you need first. 

>         rtw89_traffic_stats_accu(rtwdev, rtwvif, skb, true, true);
>         rtw89_wow_parse_akm(rtwdev, skb);
>         rtw89_core_tx_update_desc_info(rtwdev, &tx_req);
> @@ -5839,6 +5843,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 2362724323a9..4e597a5df005 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3509,6 +3509,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;
> @@ -3646,6 +3651,8 @@ struct rtw89_hci_ops {
>         void (*pause)(struct rtw89_dev *rtwdev, bool pause);
>         void (*switch_mode)(struct rtw89_dev *rtwdev, bool low_power);
>         void (*recalc_int_mit)(struct rtw89_dev *rtwdev);
> +       void (*tx_rpt_enable)(struct rtw89_dev *rtwdev,
> +                             struct rtw89_core_tx_request *tx_req);
> 
>         u8 (*read8)(struct rtw89_dev *rtwdev, u32 addr);
>         u16 (*read16)(struct rtw89_dev *rtwdev, u32 addr);
> @@ -6008,6 +6015,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;
> @@ -6294,6 +6304,7 @@ static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
>  {
>         rtwdev->hci.ops->reset(rtwdev);
>         rtw89_tx_wait_list_clear(rtwdev);
> +       skb_queue_purge(&rtwdev->tx_rpt_queue);

ieee80211_purge_tx_queue()? 
(a caller needs to hold lock)

>  }
> 
>  static inline int rtw89_hci_start(struct rtw89_dev *rtwdev)
> @@ -6336,6 +6347,13 @@ static inline void rtw89_hci_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
>         return rtwdev->hci.ops->tx_kick_off(rtwdev, txch);
>  }
> 
> +static inline void rtw89_hci_tx_rpt_enable(struct rtw89_dev *rtwdev,
> +                                          struct rtw89_core_tx_request *tx_req)
> +{
> +       if (rtwdev->hci.ops->tx_rpt_enable)
> +               rtwdev->hci.ops->tx_rpt_enable(rtwdev, tx_req);
> +}
> +
>  static inline int rtw89_hci_mac_pre_deinit(struct rtw89_dev *rtwdev)
>  {
>         return rtwdev->hci.ops->mac_pre_deinit(rtwdev);
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 01afdcd5f36c..831e53aedccc 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,15 +5457,44 @@ 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_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, bool acked)
> +{
> +       struct ieee80211_tx_info *info;
> +
> +       info = IEEE80211_SKB_CB(skb);

nit: just declare ` struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);` 

> +       ieee80211_tx_info_clear_status(info);
> +       if (acked)
> +               info->flags |= IEEE80211_TX_STAT_ACK;
> +       else
> +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> +
> +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);

I'm not aware USB use _irqsafe version before. Can I know the context of
rtw89_usb_write_port_complete()? Is it IRQ context?

> +}
> +
>  static void
>  rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
>         u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
>         u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> +       struct sk_buff *cur, *tmp;
> +       unsigned long flags;
> +       u8 *n;
> 
>         rtw89_debug(rtwdev, RTW89_DBG_TXRX,
>                     "C2H TX RPT: sn %d, tx_status %d\n",
>                     sw_define, tx_status);
> +
> +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> +       skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> +               n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;

The *n is rtw89_usb_tx_data::sn, right? I feel this is hard to ensure
correctness. Why not just define this in struct rtw89_tx_skb_data?
So no need RTW89_USB_TX_SKB_CB() for this.

> +               if (*n == sw_define) {
> +                       __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> +                       rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status == RTW89_TX_DONE);
> +                       break;
> +               }
> +       }
> +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);

If we can use ieee80211_tx_status_ni() or ieee80211_tx_status_skb(), 
I'd like use skb_queue_splice() to create a local skb list, and iterate the
local list, and then splice back to original.

(Reference to mesh_path_move_to_queue())

>  }
> 
>  static void
> diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
> index 0ee5f8579447..fdf142d77ecc 100644
> --- a/drivers/net/wireless/realtek/rtw89/pci.c
> +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> @@ -4675,6 +4675,7 @@ static const struct rtw89_hci_ops rtw89_pci_ops = {
>         .pause          = rtw89_pci_ops_pause,
>         .switch_mode    = rtw89_pci_ops_switch_mode,
>         .recalc_int_mit = rtw89_pci_recalc_int_mit,
> +       .tx_rpt_enable  = NULL, /* always enabled */

The comment is weird. PCI devices don't never use TX report, no?

> 
>         .read8          = rtw89_pci_ops_read8,
>         .read16         = rtw89_pci_ops_read16,
> 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)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index bc0d5e48d39b..98eff955fc96 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -188,6 +188,13 @@ static u8 rtw89_usb_get_bulkout_id(u8 ch_dma)
>         }
>  }
> 
> +static void rtw89_usb_ops_tx_rpt_enable(struct rtw89_dev *rtwdev,
> +                                       struct rtw89_core_tx_request *tx_req)
> +{
> +       tx_req->desc_info.report = true;
> +       tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
> +}
> +
>  static void rtw89_usb_write_port_complete(struct urb *urb)
>  {
>         struct rtw89_usb_tx_ctrl_block *txcb = urb->context;
> @@ -216,6 +223,12 @@ 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);
> +                       continue;
> +               }
> +
>                 ieee80211_tx_info_clear_status(info);
> 
>                 if (urb->status == 0) {
> @@ -364,6 +377,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 sk_buff *skb = tx_req->skb;
> +       struct rtw89_usb_tx_data *tx_data;
>         struct rtw89_txwd_body *txdesc;
>         u32 txdesc_size;
> 
> @@ -387,6 +401,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
>         memset(txdesc, 0, txdesc_size);
>         rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
> 
> +       tx_data = RTW89_USB_TX_SKB_CB(skb);
> +       tx_data->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);
> @@ -811,6 +828,7 @@ static const struct rtw89_hci_ops rtw89_usb_ops = {
>         .pause          = rtw89_usb_ops_pause,
>         .switch_mode    = rtw89_usb_ops_switch_mode,
>         .recalc_int_mit = rtw89_usb_ops_recalc_int_mit,
> +       .tx_rpt_enable  = rtw89_usb_ops_tx_rpt_enable,
> 
>         .read8          = rtw89_usb_ops_read8,
>         .read16         = rtw89_usb_ops_read16,
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.h b/drivers/net/wireless/realtek/rtw89/usb.h
> index c1b4bfa20979..c8b2ad2eaa22 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.h
> +++ b/drivers/net/wireless/realtek/rtw89/usb.h
> @@ -53,11 +53,26 @@ struct rtw89_usb {
>         struct sk_buff_head tx_queue[RTW89_TXCH_NUM];
>  };
> 
> +struct rtw89_usb_tx_data {
> +       u8 sn;
> +};
> +
>  static inline struct rtw89_usb *rtw89_usb_priv(struct rtw89_dev *rtwdev)
>  {
>         return (struct rtw89_usb *)rtwdev->priv;
>  }
> 
> +static inline struct rtw89_usb_tx_data *RTW89_USB_TX_SKB_CB(struct sk_buff *skb)
> +{
> +       struct rtw89_tx_skb_data *data = RTW89_TX_SKB_CB(skb);
> +
> +       BUILD_BUG_ON(sizeof(struct rtw89_tx_skb_data) +
> +                    sizeof(struct rtw89_usb_tx_data) >
> +                    sizeof_field(struct ieee80211_tx_info, driver_data));
> +
> +       return (struct rtw89_usb_tx_data *)data->hci_priv;
> +}
> +
>  int rtw89_usb_probe(struct usb_interface *intf,
>                     const struct usb_device_id *id);
>  void rtw89_usb_disconnect(struct usb_interface *intf);
> --
> 2.51.0
> 

RE: [PATCH rtw-next 5/6] 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 a sequence number in a TX wait object so that it'll be possible to
> identify completed items inside C2H handler.  No need to add the
> corresponding skb to the &txcb->tx_ack_queue on USB part.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> 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 831e53aedccc..79409eb4d028 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5477,6 +5477,7 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
>         u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
>         u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> +       struct rtw89_tx_wait_info *wait;
>         struct sk_buff *cur, *tmp;
>         unsigned long flags;
>         u8 *n;
> @@ -5485,6 +5486,16 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>                     "C2H TX RPT: sn %d, tx_status %d\n",
>                     sw_define, tx_status);
> 
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(wait, &rtwdev->tx_waits, list) {
> +               if (wait->sn == sw_define) {
> +                       wait->tx_done = tx_status == RTW89_TX_DONE;
> +                       complete_all(&wait->completion);
> +                       break;
> +               }
> +       }
> +       rcu_read_unlock();
> +

Since we can get 'wait' from RTW89_TX_SKB_CB(), can we just use
rtwdev->tx_rpt_queue?

Also, call rtw89_core_tx_wait_complete() to complete wait?

>         spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
>         skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
>                 n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;

RE: [PATCH rtw-next 6/6] wifi: rtw89: forcefully clear TX wait list on HCI reset

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> TX status reporting based on firmware messages does not necessarily happen
> when an HCI reset occurs, in contrast to RPP based one where pending skbs
> are forcefully flushed, see rtw89_pci_release_txwd_skb().

Is it possible that USB implement HCI reset as the same behavior? So flow can
be common and people can be easier to understand the driver.

Any limitation of USB subsystem in TX path?

> 
> So for the former case, if completion from the firmware doesn't happen, TX
> wait objects are wastefully piled up in the list and not released.
> Forcefully clear TX wait list on HCI reset then.
> 
> It's okay since wiphy lock is held during HCI reset.  For the RPP case,
> all pending completions were done just before in ->reset callback and no
> new ones can appear.  For the C2H message case, RCU access to the list
> helps.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 2 +-
>  drivers/net/wireless/realtek/rtw89/core.h | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index e76f04736502..3a0388d3acbf 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1140,7 +1140,7 @@ 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);
> +       rtw89_tx_wait_list_clear(rtwdev, false);
>  }
> 
>  void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index e7948bd0bdf6..0ad871472e79 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -6282,14 +6282,14 @@ static inline void rtw89_tx_wait_release(struct rtw89_tx_wait_info *wait)
>         kfree_rcu(wait, rcu_head);
>  }
> 
> -static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev)
> +static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev, bool force)
>  {
>         struct rtw89_tx_wait_info *wait, *tmp;
> 
>         lockdep_assert_wiphy(rtwdev->hw->wiphy);
> 
>         list_for_each_entry_safe(wait, tmp, &rtwdev->tx_waits, list) {
> -               if (!completion_done(&wait->completion))
> +               if (!force && !completion_done(&wait->completion))
>                         continue;
>                 list_del_rcu(&wait->list);
>                 rtw89_tx_wait_release(wait);
> @@ -6305,7 +6305,7 @@ static inline int rtw89_hci_tx_write(struct rtw89_dev *rtwdev,
>  static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
>  {
>         rtwdev->hci.ops->reset(rtwdev);
> -       rtw89_tx_wait_list_clear(rtwdev);
> +       rtw89_tx_wait_list_clear(rtwdev, true);
>         skb_queue_purge(&rtwdev->tx_rpt_queue);
>  }
> 
> --
> 2.51.0
> 

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

[Fedor Pchelkin]

On Wed, 24. Sep 09:03, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > @@ -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);

Hi, thanks for reviews!  I'm in process of addressing the provided
comments.

> 
> By the way, during I review this function, txcb->tx_ack_queue is a
> struct sk_buff_head, how about just struct sk_buff *skb? 
> (I might ask Bitterblue Smith about this).

tx_ack_queue is implemented like in rtw88, but there is no TX aggregation
in rtw89 yet I guess.

> 
> More, since skb here is from mac80211, so ieee80211_free_txskb() would be 
> more suitable rather than dev_kfree_skb_any()?

Agree, will do this in a separate patch.

> 
> >                         kfree(txcb);
> > --
> > 2.51.0
> > 
> 

Re: [PATCH rtw-next 3/6] wifi: rtw89: implement C2H TX report handler

[Fedor Pchelkin]

On Wed, 24. Sep 22:16, Bitterblue Smith wrote:
> On 24/09/2025 01:12, Bitterblue Smith wrote:
> > On 20/09/2025 16:26, Fedor Pchelkin wrote:
> >> diff --git a/drivers/net/wireless/realtek/rtw89/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h
> >> index ddebf7972068..f196088a8316 100644
> >> --- a/drivers/net/wireless/realtek/rtw89/fw.h
> >> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
> >> @@ -3747,6 +3747,11 @@ 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))
> >>  
> >> +#define RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h) \
> >> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 6))
> >> +#define RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h) \
> >> +	le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(12, 8))
> > 
> > This is only 4 bits:
> > 

Right, will be fixed.

> > #define TXCCXRPT_SW_DEFINE_SH		8
> > #define TXCCXRPT_SW_DEFINE_MSK		0xf
> > 
> > 
> > The rest of the series looks good to me. (I don't know much about
> > the RCU stuff.) I will test this tomorrow.
> > 
> 
> Actually, I found this in my notes:
> 
> "how to get just one tx report for each request? currently it seems
> to provide a report for each transmission attempt. how is the vendor
> driver coping with that?"
> 
> I think your code doesn't account for this.
> 
> Sorry I forgot about this detail. This behaviour is new in rtw89.
> The chips supported by rtw88 provide only one report for each request.

I think this part is also controlled with a couple of extra bits in TX
descriptor.

The vendor driver specifies a retry limit of 8 times.  It's probably the
default one followed by the firmware anyway because multiple retries on
failure status are seen via debug output of rtw89_mac_c2h_tx_rpt().
https://github.com/fofajardo/rtl8851bu/blob/1f1a14492fdac757c64a7efb7846be6374984d09/core/rtw_xmit.c#L7438

When there is a failed TX status reported by the firmware, the report is
ignored until the limit is reached or success status appears.
https://github.com/fofajardo/rtl8851bu/blob/1f1a14492fdac757c64a7efb7846be6374984d09/phl/hal_g6/hal_api_mac.c#L9547

The only concern is what happens if the sequence number of transmitted
frames wraps (4 bits is not a big range) when the previous frames with the
same sequence number were not processed yet - probability of the situation
increases with high retry limit being set.  I imagine the firmware has
some sort of handling for this but your reply suggests constraining retry
limit in the driver, are there maybe some other reasons we should set the
retry limit to one TX report?

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

[Fedor Pchelkin]

On Thu, 25. Sep 02:05, 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.
>                  ^^ nit: two spaces
> > 
> > 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
>                                          ^^ nit: two spaces
> 
> I wonder if you want two spaces intentionally? 

Oh, it's intentional "style" used to mark sentence endings more
distinctively.  I've already done that in the previous series.

> > @@ -6294,6 +6304,7 @@ static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
> >  {
> >         rtwdev->hci.ops->reset(rtwdev);
> >         rtw89_tx_wait_list_clear(rtwdev);
> > +       skb_queue_purge(&rtwdev->tx_rpt_queue);
> 
> ieee80211_purge_tx_queue()? 
> (a caller needs to hold lock)

Alright, plain skb_queue_purge() may lead to "Have pending ack frames!"
WARNING later.

> > diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> > index 01afdcd5f36c..831e53aedccc 100644
> > --- a/drivers/net/wireless/realtek/rtw89/mac.c
> > +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> > @@ -5457,15 +5457,44 @@ 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_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, bool acked)
> > +{
> > +       struct ieee80211_tx_info *info;
> > +
> > +       info = IEEE80211_SKB_CB(skb);
> 
> nit: just declare ` struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);` 
> 
> > +       ieee80211_tx_info_clear_status(info);
> > +       if (acked)
> > +               info->flags |= IEEE80211_TX_STAT_ACK;
> > +       else
> > +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> > +
> > +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> 
> I'm not aware USB use _irqsafe version before. Can I know the context of
> rtw89_usb_write_port_complete()? Is it IRQ context?
> 

Depends on the USB host controller if I'm not mistaken.  URB completion
callback may be invoked either in hard IRQ or BH context.
usb_hcd_giveback_urb() has an updated doc stating:

 * Context: atomic. The completion callback is invoked either in a work queue
 * (BH) context or in the caller's context, depending on whether the HCD_BH
 * flag is set in the @hcd structure, except that URBs submitted to the
 * root hub always complete in BH context.

If HCD_BH is not set for the host controller in use then, depending on host
controller, URB handler might be executed in hard IRQ context.

I guess you're implying to unify the usage of ieee80211_tx_status_* for
PCIe (which has ieee80211_tx_status_ni) and USB variants of rtw89.  These
calls are not mixed for the single hardware so there is no real issue
for unification.

> > +}
> > +
> >  static void
> >  rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> >  {
> >         u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
> >         u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> > +       struct sk_buff *cur, *tmp;
> > +       unsigned long flags;
> > +       u8 *n;
> > 
> >         rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> >                     "C2H TX RPT: sn %d, tx_status %d\n",
> >                     sw_define, tx_status);
> > +
> > +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> > +       skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> > +               n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;
> 
> The *n is rtw89_usb_tx_data::sn, right? I feel this is hard to ensure
> correctness. Why not just define this in struct rtw89_tx_skb_data?
> So no need RTW89_USB_TX_SKB_CB() for this.

Ah, this should work because recent commit 19989c80734c ("wifi: rtw89: use
ieee80211_tx_info::driver_data to store driver TX info") has allowed
storing more than 2 'void *' pointers in private data.

> 
> > +               if (*n == sw_define) {
> > +                       __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> > +                       rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status == RTW89_TX_DONE);
> > +                       break;
> > +               }
> > +       }
> > +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> 
> If we can use ieee80211_tx_status_ni() or ieee80211_tx_status_skb(), 

We can't use non-_irqsafe versions here unless rtw89_usb_write_port_complete()
is reworked not to use _irqsafe one.  And there is no way other than
transfering this work from URB completion handler to some other async
worker (in BH context or similar).  Not sure it'll be better overall.

> I'd like use skb_queue_splice() to create a local skb list, and iterate the
> local list, and then splice back to original.
> 
> (Reference to mesh_path_move_to_queue())

Perhaps we can do that with ieee80211_tx_status_irqsafe() still in place.

> 
> >  }
> > 
> >  static void
> > diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
> > index 0ee5f8579447..fdf142d77ecc 100644
> > --- a/drivers/net/wireless/realtek/rtw89/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> > @@ -4675,6 +4675,7 @@ static const struct rtw89_hci_ops rtw89_pci_ops = {
> >         .pause          = rtw89_pci_ops_pause,
> >         .switch_mode    = rtw89_pci_ops_switch_mode,
> >         .recalc_int_mit = rtw89_pci_recalc_int_mit,
> > +       .tx_rpt_enable  = NULL, /* always enabled */
> 
> The comment is weird. PCI devices don't never use TX report, no?

It's me mixing up the terminology, sorry.  The comment was supposed to
indicate that PCI always have TX status reported.  (but it's done via RPP
feature which is actually a separate thing compared to TX Report, okay)

I'd rather replace it with "TX status is reported via RPP" if that comment
is helpful.

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

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > +       ieee80211_tx_info_clear_status(info);
> > > +       if (acked)
> > > +               info->flags |= IEEE80211_TX_STAT_ACK;
> > > +       else
> > > +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> > > +
> > > +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> >
> > I'm not aware USB use _irqsafe version before. Can I know the context of
> > rtw89_usb_write_port_complete()? Is it IRQ context?
> >
> 
> Depends on the USB host controller if I'm not mistaken.  URB completion
> callback may be invoked either in hard IRQ or BH context.
> usb_hcd_giveback_urb() has an updated doc stating:
> 
>  * Context: atomic. The completion callback is invoked either in a work queue
>  * (BH) context or in the caller's context, depending on whether the HCD_BH
>  * flag is set in the @hcd structure, except that URBs submitted to the
>  * root hub always complete in BH context.
> 
> If HCD_BH is not set for the host controller in use then, depending on host
> controller, URB handler might be executed in hard IRQ context.

Thanks for the info. I will write down this for reference myself.

> 
> I guess you're implying to unify the usage of ieee80211_tx_status_* for
> PCIe (which has ieee80211_tx_status_ni) and USB variants of rtw89.  These
> calls are not mixed for the single hardware so there is no real issue
> for unification.

I knew this aren't mixed use of API for PCI and USB. I just think if it's
not in IRQ context, we can avoid to use spin_lock_irqsave(), but in fact
it does.


> 
> > > +}
> > > +
> > >  static void
> > >  rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> > >  {
> > >         u8 sw_define = RTW89_GET_MAC_C2H_TX_RPT_SW_DEFINE(c2h->data);
> > >         u8 tx_status = RTW89_GET_MAC_C2H_TX_RPT_TX_STATE(c2h->data);
> > > +       struct sk_buff *cur, *tmp;
> > > +       unsigned long flags;
> > > +       u8 *n;
> > >
> > >         rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> > >                     "C2H TX RPT: sn %d, tx_status %d\n",
> > >                     sw_define, tx_status);
> > > +
> > > +       spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> > > +       skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> > > +               n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;
> >
> > The *n is rtw89_usb_tx_data::sn, right? I feel this is hard to ensure
> > correctness. Why not just define this in struct rtw89_tx_skb_data?
> > So no need RTW89_USB_TX_SKB_CB() for this.
> 
> Ah, this should work because recent commit 19989c80734c ("wifi: rtw89: use
> ieee80211_tx_info::driver_data to store driver TX info") has allowed
> storing more than 2 'void *' pointers in private data.

I feel before the commit, it also allow 2 'void *' since 
   " void *status_driver_data[16 / sizeof(void *)];"

But Zong-Zhe noted me that original definition is weird, so I change it to
be more reasonable (I hope).

> 
> >
> > > +               if (*n == sw_define) {
> > > +                       __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> > > +                       rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status == RTW89_TX_DONE);
> > > +                       break;
> > > +               }
> > > +       }
> > > +       spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> >
> > If we can use ieee80211_tx_status_ni() or ieee80211_tx_status_skb(),
> 
> We can't use non-_irqsafe versions here unless rtw89_usb_write_port_complete()
> is reworked not to use _irqsafe one.  And there is no way other than
> transfering this work from URB completion handler to some other async
> worker (in BH context or similar).  Not sure it'll be better overall.

As your above information about context, just ignore this. 

I just thought if we can reduce critical section of spin_lock_irqsave(),
but it seems to be no way. So keep it as was if we still no better idea
before getting merged. 

> 
> > I'd like use skb_queue_splice() to create a local skb list, and iterate the
> > local list, and then splice back to original.
> >
> > (Reference to mesh_path_move_to_queue())
> 
> Perhaps we can do that with ieee80211_tx_status_irqsafe() still in place.

Not sure how you will do. Let's see it in your next version. :-)

> 
> >
> > >  }
> > >
> > >  static void
> > > diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
> > > index 0ee5f8579447..fdf142d77ecc 100644
> > > --- a/drivers/net/wireless/realtek/rtw89/pci.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> > > @@ -4675,6 +4675,7 @@ static const struct rtw89_hci_ops rtw89_pci_ops = {
> > >         .pause          = rtw89_pci_ops_pause,
> > >         .switch_mode    = rtw89_pci_ops_switch_mode,
> > >         .recalc_int_mit = rtw89_pci_recalc_int_mit,
> > > +       .tx_rpt_enable  = NULL, /* always enabled */
> >
> > The comment is weird. PCI devices don't never use TX report, no?
> 
> It's me mixing up the terminology, sorry.  The comment was supposed to
> indicate that PCI always have TX status reported.  (but it's done via RPP
> feature which is actually a separate thing compared to TX Report, okay)
> 
> I'd rather replace it with "TX status is reported via RPP" if that comment
> is helpful.

Yes, it looks better. Thanks.

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

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> >
> > By the way, during I review this function, txcb->tx_ack_queue is a
> > struct sk_buff_head, how about just struct sk_buff *skb?
> > (I might ask Bitterblue Smith about this).
> 
> tx_ack_queue is implemented like in rtw88, but there is no TX aggregation
> in rtw89 yet I guess.

Got it & thanks.