| 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: <de5673b6c65d460187b9d99a14783a7e@realtek.com>
Date: Thu, 25 Sep 2025 02:05:42 +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 4/6] 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.
^^ nit: two spaces
>
> 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
^^ nit: two spaces
I wonder if you want two spaces intentionally?
> scheme based on processing C2H messages.
>
> Thus define a .tx_rpt_enable handler which will initialize the
> corresponding values used later when writing to TX descriptor. The
> handler is a no-op for PCIe, TX status reporting behaviour doesn't change
> there.
>
> 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 (done at core deinitialization phase, too).
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
> ---
> drivers/net/wireless/realtek/rtw89/core.c | 5 ++++
> drivers/net/wireless/realtek/rtw89/core.h | 18 ++++++++++++++
> drivers/net/wireless/realtek/rtw89/mac.c | 29 +++++++++++++++++++++++
> drivers/net/wireless/realtek/rtw89/pci.c | 1 +
> drivers/net/wireless/realtek/rtw89/pci.h | 4 ----
> drivers/net/wireless/realtek/rtw89/usb.c | 18 ++++++++++++++
> drivers/net/wireless/realtek/rtw89/usb.h | 15 ++++++++++++
> 7 files changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index d2a559ddfa2e..3e7bd0cedbdf 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1229,6 +1229,7 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
> struct ieee80211_sta *sta = rtwsta_link_to_sta_safe(rtwsta_link);
> struct ieee80211_vif *vif = rtwvif_link_to_vif(rtwvif_link);
> struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> struct rtw89_vif *rtwvif = rtwvif_link->rtwvif;
> struct rtw89_core_tx_request tx_req = {};
> int ret;
> @@ -1240,6 +1241,9 @@ static int rtw89_core_tx_write_link(struct rtw89_dev *rtwdev,
> tx_req.rtwsta_link = rtwsta_link;
> tx_req.desc_info.sw_mld = sw_mld;
>
> + if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS)
> + rtw89_hci_tx_rpt_enable(rtwdev, &tx_req);
> +
Normally, we update TX description in rtw89_core_tx_update_desc_info(), and
rtw89_hci_tx_rpt_enable() can be implemented in common flow (but give proper
name). And add a flag in rtw89_hci_info to determine if we should enable
TX report.
By the way, I'm thinking if add a `struct rtw89_hci_attr` as HCI common
attributes for common flow.
Another is that rtw89_core_tx_update_desc_info() is a common entry to all
TX types, but most conditions (including we are going to add) are unnecessary
to RTW89_CORE_TX_TYPE_FWCMD. I also plan to refine this.
Anyway, just add what you need first.
> rtw89_traffic_stats_accu(rtwdev, rtwvif, skb, true, true);
> rtw89_wow_parse_akm(rtwdev, skb);
> rtw89_core_tx_update_desc_info(rtwdev, &tx_req);
> @@ -5839,6 +5843,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 2362724323a9..4e597a5df005 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3509,6 +3509,11 @@ struct rtw89_phy_rate_pattern {
> bool enable;
> };
>
> +#define RTW89_TX_DONE 0x0
> +#define RTW89_TX_RETRY_LIMIT 0x1
> +#define RTW89_TX_LIFE_TIME 0x2
> +#define RTW89_TX_MACID_DROP 0x3
> +
> #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
> struct rtw89_tx_wait_info {
> struct rcu_head rcu_head;
> @@ -3646,6 +3651,8 @@ struct rtw89_hci_ops {
> void (*pause)(struct rtw89_dev *rtwdev, bool pause);
> void (*switch_mode)(struct rtw89_dev *rtwdev, bool low_power);
> void (*recalc_int_mit)(struct rtw89_dev *rtwdev);
> + void (*tx_rpt_enable)(struct rtw89_dev *rtwdev,
> + struct rtw89_core_tx_request *tx_req);
>
> u8 (*read8)(struct rtw89_dev *rtwdev, u32 addr);
> u16 (*read16)(struct rtw89_dev *rtwdev, u32 addr);
> @@ -6008,6 +6015,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;
> @@ -6294,6 +6304,7 @@ static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
> {
> rtwdev->hci.ops->reset(rtwdev);
> rtw89_tx_wait_list_clear(rtwdev);
> + skb_queue_purge(&rtwdev->tx_rpt_queue);
ieee80211_purge_tx_queue()?
(a caller needs to hold lock)
> }
>
> static inline int rtw89_hci_start(struct rtw89_dev *rtwdev)
> @@ -6336,6 +6347,13 @@ static inline void rtw89_hci_tx_kick_off(struct rtw89_dev *rtwdev, u8 txch)
> return rtwdev->hci.ops->tx_kick_off(rtwdev, txch);
> }
>
> +static inline void rtw89_hci_tx_rpt_enable(struct rtw89_dev *rtwdev,
> + struct rtw89_core_tx_request *tx_req)
> +{
> + if (rtwdev->hci.ops->tx_rpt_enable)
> + rtwdev->hci.ops->tx_rpt_enable(rtwdev, tx_req);
> +}
> +
> static inline int rtw89_hci_mac_pre_deinit(struct rtw89_dev *rtwdev)
> {
> return rtwdev->hci.ops->mac_pre_deinit(rtwdev);
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 01afdcd5f36c..831e53aedccc 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,15 +5457,44 @@ 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_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, bool acked)
> +{
> + struct ieee80211_tx_info *info;
> +
> + info = IEEE80211_SKB_CB(skb);
nit: just declare ` struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);`
> + ieee80211_tx_info_clear_status(info);
> + if (acked)
> + info->flags |= IEEE80211_TX_STAT_ACK;
> + else
> + info->flags &= ~IEEE80211_TX_STAT_ACK;
> +
> + ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
I'm not aware USB use _irqsafe version before. Can I know the context of
rtw89_usb_write_port_complete()? Is it IRQ context?
> +}
> +
> 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);
> + struct sk_buff *cur, *tmp;
> + unsigned long flags;
> + u8 *n;
>
> rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> "C2H TX RPT: sn %d, tx_status %d\n",
> sw_define, tx_status);
> +
> + spin_lock_irqsave(&rtwdev->tx_rpt_queue.lock, flags);
> + skb_queue_walk_safe(&rtwdev->tx_rpt_queue, cur, tmp) {
> + n = (u8 *)RTW89_TX_SKB_CB(cur)->hci_priv;
The *n is rtw89_usb_tx_data::sn, right? I feel this is hard to ensure
correctness. Why not just define this in struct rtw89_tx_skb_data?
So no need RTW89_USB_TX_SKB_CB() for this.
> + if (*n == sw_define) {
> + __skb_unlink(cur, &rtwdev->tx_rpt_queue);
> + rtw89_tx_rpt_tx_status(rtwdev, cur, tx_status == RTW89_TX_DONE);
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&rtwdev->tx_rpt_queue.lock, flags);
If we can use ieee80211_tx_status_ni() or ieee80211_tx_status_skb(),
I'd like use skb_queue_splice() to create a local skb list, and iterate the
local list, and then splice back to original.
(Reference to mesh_path_move_to_queue())
> }
>
> static void
> diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
> index 0ee5f8579447..fdf142d77ecc 100644
> --- a/drivers/net/wireless/realtek/rtw89/pci.c
> +++ b/drivers/net/wireless/realtek/rtw89/pci.c
> @@ -4675,6 +4675,7 @@ static const struct rtw89_hci_ops rtw89_pci_ops = {
> .pause = rtw89_pci_ops_pause,
> .switch_mode = rtw89_pci_ops_switch_mode,
> .recalc_int_mit = rtw89_pci_recalc_int_mit,
> + .tx_rpt_enable = NULL, /* always enabled */
The comment is weird. PCI devices don't never use TX report, no?
>
> .read8 = rtw89_pci_ops_read8,
> .read16 = rtw89_pci_ops_read16,
> diff --git a/drivers/net/wireless/realtek/rtw89/pci.h b/drivers/net/wireless/realtek/rtw89/pci.h
> index cb05c83dfd56..16dfb0e79d77 100644
> --- a/drivers/net/wireless/realtek/rtw89/pci.h
> +++ b/drivers/net/wireless/realtek/rtw89/pci.h
> @@ -1487,10 +1487,6 @@ struct rtw89_pci_tx_addr_info_32_v1 {
> #define RTW89_PCI_RPP_POLLUTED BIT(31)
> #define RTW89_PCI_RPP_SEQ GENMASK(30, 16)
> #define RTW89_PCI_RPP_TX_STATUS GENMASK(15, 13)
> -#define RTW89_TX_DONE 0x0
> -#define RTW89_TX_RETRY_LIMIT 0x1
> -#define RTW89_TX_LIFE_TIME 0x2
> -#define RTW89_TX_MACID_DROP 0x3
> #define RTW89_PCI_RPP_QSEL GENMASK(12, 8)
> #define RTW89_PCI_RPP_MACID GENMASK(7, 0)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index bc0d5e48d39b..98eff955fc96 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -188,6 +188,13 @@ static u8 rtw89_usb_get_bulkout_id(u8 ch_dma)
> }
> }
>
> +static void rtw89_usb_ops_tx_rpt_enable(struct rtw89_dev *rtwdev,
> + struct rtw89_core_tx_request *tx_req)
> +{
> + tx_req->desc_info.report = true;
> + tx_req->desc_info.sn = atomic_inc_return(&rtwdev->sn) & 0xF;
> +}
> +
> static void rtw89_usb_write_port_complete(struct urb *urb)
> {
> struct rtw89_usb_tx_ctrl_block *txcb = urb->context;
> @@ -216,6 +223,12 @@ 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);
> + continue;
> + }
> +
> ieee80211_tx_info_clear_status(info);
>
> if (urb->status == 0) {
> @@ -364,6 +377,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 sk_buff *skb = tx_req->skb;
> + struct rtw89_usb_tx_data *tx_data;
> struct rtw89_txwd_body *txdesc;
> u32 txdesc_size;
>
> @@ -387,6 +401,9 @@ static int rtw89_usb_ops_tx_write(struct rtw89_dev *rtwdev,
> memset(txdesc, 0, txdesc_size);
> rtw89_chip_fill_txdesc(rtwdev, desc_info, txdesc);
>
> + tx_data = RTW89_USB_TX_SKB_CB(skb);
> + tx_data->sn = tx_req->desc_info.sn;
> +
> le32p_replace_bits(&txdesc->dword0, 1, RTW89_TXWD_BODY0_STF_MODE);
>
> skb_queue_tail(&rtwusb->tx_queue[desc_info->ch_dma], skb);
> @@ -811,6 +828,7 @@ static const struct rtw89_hci_ops rtw89_usb_ops = {
> .pause = rtw89_usb_ops_pause,
> .switch_mode = rtw89_usb_ops_switch_mode,
> .recalc_int_mit = rtw89_usb_ops_recalc_int_mit,
> + .tx_rpt_enable = rtw89_usb_ops_tx_rpt_enable,
>
> .read8 = rtw89_usb_ops_read8,
> .read16 = rtw89_usb_ops_read16,
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.h b/drivers/net/wireless/realtek/rtw89/usb.h
> index c1b4bfa20979..c8b2ad2eaa22 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.h
> +++ b/drivers/net/wireless/realtek/rtw89/usb.h
> @@ -53,11 +53,26 @@ struct rtw89_usb {
> struct sk_buff_head tx_queue[RTW89_TXCH_NUM];
> };
>
> +struct rtw89_usb_tx_data {
> + u8 sn;
> +};
> +
> static inline struct rtw89_usb *rtw89_usb_priv(struct rtw89_dev *rtwdev)
> {
> return (struct rtw89_usb *)rtwdev->priv;
> }
>
> +static inline struct rtw89_usb_tx_data *RTW89_USB_TX_SKB_CB(struct sk_buff *skb)
> +{
> + struct rtw89_tx_skb_data *data = RTW89_TX_SKB_CB(skb);
> +
> + BUILD_BUG_ON(sizeof(struct rtw89_tx_skb_data) +
> + sizeof(struct rtw89_usb_tx_data) >
> + sizeof_field(struct ieee80211_tx_info, driver_data));
> +
> + return (struct rtw89_usb_tx_data *)data->hci_priv;
> +}
> +
> int rtw89_usb_probe(struct usb_interface *intf,
> const struct usb_device_id *id);
> void rtw89_usb_disconnect(struct usb_interface *intf);
> --
> 2.51.0
>
Powered by blists - more mailing lists