| 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: <b1725b36caba476ba9ee4916910d8232@realtek.com>
Date: Tue, 7 Oct 2025 06:31:06 +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 v2 6/7] 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_enable flag defining which HCIs would need a TX
> report feature. Currently it is USB only.
>
> 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 and on timeout via delayed work handler used
> for TX waits - it's convenient since the further changes will pass TX wait
> skbs management to the same TX report queue.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
> ---
>
> v2: - update TX rpt description in rtw89_core_tx_update_desc_info()
> - add a flag in rtw89_hci_info to determine if we should enable TX report (Ping-Ke)
> - refine rtw89_tx_rpt_queue_purge() - it's called on USB HCI reset
> and at rtw89_tx_wait_work. Queueing delayed rtw89_tx_wait_work may be
> suboptimal but basically it's what rtw88 does with timer stuff.
> We can drop it and rely only on HCI reset to free remaining buffers
> in case firmware didn't send any TX status report. I'd like to know
> your thoughts on this.
>
> drivers/net/wireless/realtek/rtw89/core.c | 9 ++++-
> drivers/net/wireless/realtek/rtw89/core.h | 6 ++++
> drivers/net/wireless/realtek/rtw89/mac.c | 19 ++++++++++
> drivers/net/wireless/realtek/rtw89/mac.h | 43 +++++++++++++++++++++++
> drivers/net/wireless/realtek/rtw89/usb.c | 14 +++++++-
> 5 files changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index 49ecc248464b..214924f8bee0 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1107,6 +1107,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
> tx_req->rtwsta_link);
> if (addr_cam->valid && desc_info->mlo)
> upd_wlan_hdr = true;
> +
> + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> + rtw89_tx_rpt_enable(rtwdev, tx_req);
During review of previous round, I add a patch [1] internally to adjust
this function, because we only need to fill few fields for FWCMD type.
Please add my patch into your patchset before this patch.
[1] https://lore.kernel.org/linux-wireless/20251007032656.13189-1-pkshih@realtek.com/T/#u
> }
> is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
> is_multicast_ether_addr(hdr->addr1));
> @@ -1140,7 +1143,10 @@ 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);
> + if (!rtwdev->hci.tx_rpt_enable)
> + rtw89_tx_wait_list_clear(rtwdev);
> + else
> + rtw89_tx_rpt_queue_purge(rtwdev);
I feel rtw89_tx_wait_work() should only free list of 'rtwdev->tx_waits'.
To mix TX report queue ('rtwdev->tx_rpt_queue') looks hard to understand.
A skb exists in two queues looks not easy to handle...
Also, I see rtw89_hci_reset() calls rtw89_tx_wait_list_clear(). Is it still
correct after this patch?
> }
>
> void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
> @@ -5847,6 +5853,7 @@ int rtw89_core_init(struct rtw89_dev *rtwdev)
> wiphy_work_init(&rtwdev->cancel_6ghz_probe_work, rtw89_cancel_6ghz_probe_work);
> INIT_WORK(&rtwdev->load_firmware_work, rtw89_load_firmware_work);
>
> + skb_queue_head_init(&rtwdev->tx_rpt_queue);
> skb_queue_head_init(&rtwdev->c2h_queue);
> rtw89_core_ppdu_sts_init(rtwdev);
> rtw89_traffic_stats_init(rtwdev, &rtwdev->stats);
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 66b7bfa5902e..3940e54353d3 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3527,6 +3527,8 @@ struct rtw89_tx_wait_info {
>
> struct rtw89_tx_skb_data {
> struct rtw89_tx_wait_info __rcu *wait;
> + u8 tx_rpt_sn;
> + u8 tx_pkt_cnt_lmt;
> u8 hci_priv[];
> };
>
> @@ -3696,6 +3698,7 @@ struct rtw89_hci_info {
> u32 rpwm_addr;
> u32 cpwm_addr;
> bool paused;
> + bool tx_rpt_enable;
tx_rpt_enabled?
> };
>
> struct rtw89_chip_ops {
> @@ -6015,6 +6018,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;
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 10c2a39e544b..75d9efac452b 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5465,10 +5465,29 @@ rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> u8 sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
> u8 tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
> u8 data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> + struct rtw89_tx_skb_data *skb_data;
> + struct sk_buff *cur, *tmp;
> + unsigned long flags;
>
> rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
> sw_define, tx_status, data_txcnt);
> +
> + spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> + skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> + skb_data = RTW89_TX_SKB_CB(cur);
Prefer just 'skb' instead of 'cur'.
I don't need look back to understand 'cur' is a sk_buff.
> +
> + if (sw_define != skb_data->tx_rpt_sn)
> + continue;
> + if (tx_status != RTW89_TX_DONE &&
> + data_txcnt != skb_data->tx_pkt_cnt_lmt)
As commit message of previous patch, "When there is a failed
TX status reported by the firmware, the report is ignored until the limit
is reached or success status appears."
Do you still need to check data_txcnt for failed cases?
> + continue;
> +
> + __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> + rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status);
> + break;
> + }
> + spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> }
>
> static void
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 15c5c7e4033c..1f7d3734d15f 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1616,4 +1616,47 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
>
> return ret;
> }
> +
> +static inline
> +void rtw89_tx_rpt_enable(struct rtw89_dev *rtwdev,
> + struct rtw89_core_tx_request *tx_req)
> +{
> + if (!rtwdev->hci.tx_rpt_enable)
> + return;
> +
> + tx_req->desc_info.report = true;
> + tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
Since you have assigned 4 bits for 'sn', no need '& 0xF'.
If you still want to emphasize this, maybe add a comment.
> + tx_req->desc_info.tx_cnt_lmt_en = true;
> + tx_req->desc_info.tx_cnt_lmt = 8;
Question about '8'. Can it be larger?
I mean if hardware retry over 8 (e.g. 10), and succeed to send at 9th retry.
Would it be a problem?
> +}
> +
> +static inline
> +void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> + ieee80211_tx_info_clear_status(info);
> + if (tx_status == RTW89_TX_DONE)
> + info->flags |= IEEE80211_TX_STAT_ACK;
> + else
> + info->flags &= ~IEEE80211_TX_STAT_ACK;
> +
> + ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> +}
> +
> +static inline
> +void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
> +{
> + struct sk_buff_head q;
> + struct sk_buff *skb;
> + unsigned long flags;
> +
> + __skb_queue_head_init(&q);
With a blank line, the lock would be clear.
> + spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> + skb_queue_splice_init(&rtwdev->tx_rpt_queue, &q);
> + spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
> +
> + while ((skb = __skb_dequeue(&q)))
> + rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +}
> #endif
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index 655e8437d62e..f53ab676e9a8 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);
> + 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);
> + wiphy_delayed_work_queue(rtwdev->hw->wiphy,
> + &rtwdev->tx_wait_work,
> + RTW89_TX_WAIT_WORK_TIMEOUT);
> + continue;
> + }
> +
> ieee80211_tx_info_clear_status(info);
>
> if (urb->status == 0) {
> @@ -396,6 +405,8 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> memset(txdesc, 0, txdesc_size);
> rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
>
> + RTW89_TX_SKB_CB(skb)->tx_rpt_sn = tx_req->desc_info.sn;
Prefer a local variable 'struct rtw89_tx_skb_data *skb_data;'.
> +
> le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
This is also to fill tx desc, so prefer filling tx_rpt_sn after this.
>
> skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> @@ -678,7 +689,7 @@ static void rtw89_usb_deinit_tx(struct rtw89_dev *rtwdev)
>
> static void rtw89_usb_ops_reset(struct rtw89_dev *rtwdev)
> {
> - /* TODO: anything to do here? */
> + rtw89_tx_rpt_queue_purge(rtwdev);
No sure if we can ensure no TX packets still on the fly --
which have been submitted, but not call rtw89_usb_write_port_complete() yet.
But I'm also not sure if it is a problem that newly TX completion happens
after calling rtw89_tx_rpt_queue_purge().
> }
>
> static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> @@ -962,6 +973,7 @@ int rtw89_usb_probe(struct usb_interface *intf,
>
> rtwdev->hci.ops = &rtw89_usb_ops;
> rtwdev->hci.type = RTW89_HCI_TYPE_USB;
> + rtwdev->hci.tx_rpt_enable = true;
>
> ret = rtw89_usb_intf_init(rtwdev, intf);
> if (ret) {
> --
> 2.51.0
>
Powered by blists - more mailing lists