| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5d0ee6db6ab44ad48222cc4f224aa307@realtek.com>
Date: Wed, 24 Sep 2025 09:18:31 +0000
From: Ping-Ke Shih <pkshih@...ltek.com>
To: Fedor Pchelkin <pchelkin@...ras.ru>,
Bitterblue Smith
<rtl8821cerfe2@...il.com>
CC: Zong-Zhe Yang <kevin_yang@...ltek.com>,
Bernie Huang
<phhuang@...ltek.com>,
"linux-wireless@...r.kernel.org"
<linux-wireless@...r.kernel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"lvc-project@...uxtesting.org"
<lvc-project@...uxtesting.org>
Subject: RE: [PATCH rtw-next 3/6] wifi: rtw89: implement C2H TX report handler
Fedor Pchelkin <pchelkin@...ras.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@...il.com>
> Signed-off-by: Fedor Pchelkin <pchelkin@...ras.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.
Powered by blists - more mailing lists