| 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: <f013f65b97a447e2b744a4f3d6aff269@realtek.com>
Date: Wed, 22 Oct 2025 07:16:36 +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 v3 7/9] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB
Fedor Pchelkin <pchelkin@...ras.ru> wrote:
[...]
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index abe8eec1d0f5..3aa9a9a28118 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1112,6 +1112,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
> if (addr_cam->valid && desc_info->mlo)
> upd_wlan_hdr = true;
>
> + if (rtw89_is_tx_rpt_skb(tx_req->skb))
> + rtw89_tx_rpt_init(rtwdev, tx_req);
> +
> is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
> is_multicast_ether_addr(hdr->addr1));
>
> @@ -5849,6 +5852,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);
not sure if it's worth to initialize tx_rpt.sn to zero?
> 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..8641e3a8d36d 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3516,6 +3516,11 @@ struct rtw89_phy_rate_pattern {
> #define RTW89_TX_LIFE_TIME 0x2
> #define RTW89_TX_MACID_DROP 0x3
>
> +struct rtw89_tx_rpt {
> + struct sk_buff_head queue;
> + atomic_t sn;
> +};
> +
> #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
> struct rtw89_tx_wait_info {
> struct rcu_head rcu_head;
> @@ -3527,6 +3532,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 +3703,7 @@ struct rtw89_hci_info {
> u32 rpwm_addr;
> u32 cpwm_addr;
> bool paused;
> + bool tx_rpt_enabled;
> };
>
> struct rtw89_chip_ops {
> @@ -6015,6 +6023,8 @@ struct rtw89_dev {
> struct list_head tx_waits;
> struct wiphy_delayed_work tx_wait_work;
>
> + struct rtw89_tx_rpt tx_rpt;
> +
> 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 2fe239f18534..26c7476afdec 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5460,7 +5460,11 @@ 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;
> u8 sw_define, tx_status, data_txcnt;
> + struct rtw89_tx_skb_data *skb_data;
> + struct sk_buff *skb, *tmp;
> + unsigned long flags;
>
> if (rtwdev->chip->chip_id == RTL8922A) {
> const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> @@ -5484,6 +5488,26 @@ 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, data_txcnt %d\n",
> sw_define, tx_status, data_txcnt);
> +
> + spin_lock_irqsave(&tx_rpt->queue.lock, flags);
> + skb_queue_walk_safe(&tx_rpt->queue, skb, tmp) {
> + skb_data = RTW89_TX_SKB_CB(skb);
> +
> + /* skip if sequence number doesn't match */
> + if (sw_define != skb_data->tx_rpt_sn)
> + continue;
> + /* skip if TX attempt has failed and retry limit has not been
> + * reached yet
> + */
> + if (tx_status != RTW89_TX_DONE &&
> + data_txcnt != skb_data->tx_pkt_cnt_lmt)
> + continue;
> +
> + __skb_unlink(skb, &tx_rpt->queue);
> + rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
Would it be better to run rtw89_tx_rpt_tx_status() after this loop outside
spin_lock()?
> + break;
> + }
> + spin_unlock_irqrestore(&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..e8bd92223497 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1616,4 +1616,60 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
>
> return ret;
> }
> +
> +static inline
> +void rtw89_tx_rpt_init(struct rtw89_dev *rtwdev,
> + struct rtw89_core_tx_request *tx_req)
> +{
> + struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> +
> + if (!rtwdev->hci.tx_rpt_enabled)
> + return;
> +
> + tx_req->desc_info.report = true;
> + /* firmware maintains a 4-bit sequence number */
> + tx_req->desc_info.sn = atomic_inc_return(&tx_rpt->sn) & 0xF;
> + tx_req->desc_info.tx_cnt_lmt_en = true;
> + tx_req->desc_info.tx_cnt_lmt = 8;
> +}
> +
> +static inline
> +bool rtw89_is_tx_rpt_skb(struct sk_buff *skb)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> + return info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
> +}
> +
> +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 rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> + struct sk_buff_head q;
> + struct sk_buff *skb;
> + unsigned long flags;
> +
> + __skb_queue_head_init(&q);
> +
> + spin_lock_irqsave(&tx_rpt->queue.lock, flags);
> + skb_queue_splice_init(&tx_rpt->queue, &q);
> + spin_unlock_irqrestore(&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..22994c3501f8 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,6 +216,14 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
> skb_pull(skb, txdesc_size);
>
> info = IEEE80211_SKB_CB(skb);
> + if (rtw89_is_tx_rpt_skb(skb)) {
> + /* sequence number is passed to rtw89_mac_c2h_tx_rpt() via
nit: The 'via' is over 80 characters a little bit. Move to next line.
> + * driver data
> + */
> + skb_queue_tail(&rtwdev->tx_rpt.queue, skb);
> + continue;
> + }
> +
> ieee80211_tx_info_clear_status(info);
>
> if (urb->status == 0) {
Should we move this checking upward? Enqueue skb into tx_rpt_skb only if
urb->status == 0?
> @@ -372,6 +380,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 rtw89_tx_skb_data *skb_data;
> struct sk_buff *skb = tx_req->skb;
> struct rtw89_txwd_body *txdesc;
> u32 txdesc_size;
> @@ -398,6 +407,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
>
> le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
>
> + skb_data = RTW89_TX_SKB_CB(skb);
> + skb_data->tx_rpt_sn = tx_req->desc_info.sn;
Shouldn't set skb_data->tx_pkt_cnt_lmt?
skb_data->tx_pkt_cnt_lmt = tx_req->desc_info.tx_cnt_lmt;
Also, should we check desc_info.{report, tx_cnt_lmt_en} individually before
setting?
> +
> skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
>
> return 0;
> @@ -678,7 +690,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);
Have you consider the SKB that has been rtw89_usb_write_port() but
has not yet rtw89_usb_write_port_complete()?
Since we call rtw89_mac_pwr_off() before rtw89_hci_reset() in
rtw89_core_stop(), it should be not more C2H at rtw89_hci_reset().
It seems to be safe, right?
Also, all are dropped, can't we just call ieee80211_purge_tx_queue()?
> }
>
> static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> @@ -962,6 +974,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_enabled = true;
>
> ret = rtw89_usb_intf_init(rtwdev, intf);
> if (ret) {
> --
> 2.51.0
>
Powered by blists - more mailing lists