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

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

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)
>>  
>