| 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: <1f5893398aac4966a2ae2939a6cb7f9c@realtek.com>
Date: Mon, 3 Nov 2025 03:05:03 +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 v4 08/10] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB
Fedor Pchelkin <pchelkin@...ras.ru> wrote:
> Frames flagged with IEEE80211_TX_CTL_REQ_TX_STATUS mean the driver has to
> report to mac80211 stack whether AP sent ACK for the null frame/probe
> request or not. It's not implemented in USB part of the driver yet.
>
> PCIe HCI has its own way of getting TX status incorporated into RPP
> feature, and it's always enabled there. Other HCIs need a different
> scheme based on processing C2H messages.
>
> Thus define a .tx_rpt_enabled flag indicating which HCIs need to enable a
> TX report feature. Currently it is USB only.
>
> Toggle a bit in the TX descriptor and place flagged skbs in a fix-sized
> queue to wait for a message from the firmware. Firmware maintains a 4-bit
> sequence number for required frames hence the queue can contain just 16
> elements simultaneously. That's enough for normal driver / firmware
> communication. If the firmware crashes for any reason and doesn't provide
> TX reports in time, driver will handle this and report the obsolete frames
> as dropped.
>
> rtw89 also has a new feature providing a TX report for each transmission
> attempt. Ignore a failed TX status reported by the firmware until retry
> limit is reached or successful status appears. When there is no success
> and the retry limit is reached, report the frame up to the wireless stack
> as failed eventually.
>
> HCI reset should stop all pending TX activity so forcefully flush the
> queue there.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
Thanks for your work. I have only some minor suggestions for v4.
[...]
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 9372e30a0039..015d7833841f 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3517,6 +3517,14 @@ struct rtw89_phy_rate_pattern {
> #define RTW89_TX_LIFE_TIME 0x2
> #define RTW89_TX_MACID_DROP 0x3
>
> +#define RTW89_MAX_TX_RPTS 16
> +#define RTW89_MAX_TX_RPTS_MASK (RTW89_MAX_TX_RPTS - 1)
> +struct rtw89_tx_rpt {
> + struct sk_buff *skbs[RTW89_MAX_TX_RPTS];
> + spinlock_t skb_lock;
I think we should add a comment to describe how/where this lock can be used.
At least checkpatch will complain this.
> + atomic_t sn;
> +};
> +
> #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
> struct rtw89_tx_wait_info {
> struct rcu_head rcu_head;
[...]
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index e4e126a4428b..a6642c5761cb 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5463,7 +5463,10 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
> static void
> rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> {
> + struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> + struct rtw89_tx_skb_data *skb_data;
> u8 sw_define, tx_status, txcnt;
> + struct sk_buff *skb;
>
> if (rtwdev->chip->chip_id == RTL8922A) {
> const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> @@ -5492,6 +5495,29 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> "C2H TX RPT: sn %d, tx_status %d, txcnt %d\n",
> sw_define, tx_status, txcnt);
To claim sw_define is not over size of tx_rpt->skbs[], we can add below:
static_assert(hweight32(RTW89_MAX_TX_RPTS_MASK) ==
hweight32(RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2) &&
hweight32(RTW89_MAX_TX_RPTS_MASK) ==
hweight32(RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE));
> +
> + scoped_guard(spinlock_irqsave, &tx_rpt->skb_lock) {
> + skb = tx_rpt->skbs[sw_define];
> +
> + /* skip if no skb (normally shouldn't happen) */
> + if (!skb) {
> + rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> + "C2H TX RPT: no skb found in queue\n");
> + return;
> + }
> +
> + skb_data = RTW89_TX_SKB_CB(skb);
> +
> + /* skip if TX attempt has failed and retry limit has not been
> + * reached yet
> + */
> + if (tx_status != RTW89_TX_DONE &&
> + txcnt != skb_data->tx_pkt_cnt_lmt)
> + return;
> +
> + tx_rpt->skbs[sw_define] = NULL;
> + rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
> + }
> }
>
> static void
[...]
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index c359b469aabe..5e587c93268e 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,6 +216,15 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
> skb_pull(skb, txdesc_size);
>
> info = IEEE80211_SKB_CB(skb);
Since newly added chunk doesn't use 'info', just do it before
ieee80211_tx_info_clear_status().
> + if (rtw89_is_tx_rpt_skb(skb)) {
> + if (urb->status == 0)
> + rtw89_tx_rpt_skb_add(rtwdev, skb);
> + else
> + rtw89_tx_rpt_tx_status(rtwdev, skb,
> + RTW89_TX_MACID_DROP);
> + continue;
> + }
> +
> ieee80211_tx_info_clear_status(info);
>
> if (urb->status == 0) {
[...]
Powered by blists - more mailing lists