| 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: <71b89748d72d42c28696c9ba1ee3addf@realtek.com>
Date: Mon, 27 Oct 2025 01:14:35 +0000
From: Ping-Ke Shih <pkshih@...ltek.com>
To: Fedor Pchelkin <pchelkin@...ras.ru>
CC: Bitterblue Smith <rtl8821cerfe2@...il.com>,
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:
> On Wed, 22. Oct 07:16, Ping-Ke Shih wrote:
> > Fedor Pchelkin <pchelkin@...ras.ru> wrote:
> > > @@ -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?
>
> That shouldn't be needed because rtwdev is zero initialized in
> rtw89_alloc_ieee80211_hw(). ieee80211_alloc_hw() fills the private
> driver part with zeroes.
Ah. I mentioned this in wrong place. I meant that we can initialize tx_rpt.sn
in rtw89_core_start() or do it right after downloading firmware in
__rtw89_fw_download() like ' fw_info->h2c_seq = 0;'.
>
> > > @@ -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()?
>
> I don't have a strong opinion here: PCIe side implements the release
> like
>
> rtw89_pci_poll_rpq_dma()
> spin_lock_bh(&rtwpci->trx_lock)
> rtw89_pci_release_tx()
> ...
> rtw89_pci_release_txwd_skb()
> skb_queue_walk_safe(&txwd->queue, skb, tmp) {
> skb_unlink(skb, &txwd->queue);
>
> tx_data = RTW89_PCI_TX_SKB_CB(skb);
> dma_unmap_single(&rtwpci->pdev->dev, tx_data->dma, skb->len,
> DMA_TO_DEVICE);
>
> rtw89_pci_tx_status(rtwdev, tx_ring, skb, tx_status);
> }
> ...
> spin_unlock_bh(&rtwpci->trx_lock)
>
>
> Apart from bh/irqsave part the iteration over skbs looks visually similar
> at the moment.
>
> > > --- 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?
>
> Yep, I agree we can do it. Just need to report it immediately with
> RTW89_TX_MACID_DROP status.
>
> As it currently stands, we should not receive notification from the
> firmware for such skb so it would remain inside tx_rpt.queue until HCI
> reset occurs.
>
> However, I've not considered the case when the queue is full and we start
> queueing skbs with duplicate sequence numbers - the overall range we have
> is just 0xF, theoretically the situation is possible if the firmware
> fatally crashes and doesn't provide notifications in time. IMHO the TX
> status will be the last thing we're going to be interested in when this
> happens. In the end, HCI reset during SER activity will purge the queue.
>
> But the possibility of having skbs with duplicate seq numbers is not good.
> Though I'm not sure if we can ever hit such a situation... Generally it
> indicates that the firmware doesn't respond or performs very badly, and
> we'd better reset it or something. What do you think on this?
Maybe we can maintain a bitmap to detect duplicate seq numbers. Also, we can
search for a SKB according to tx_report.sn in constant time instead of doing
sequential search in a list. Then we can detect and remove duplicate seq
numbers at TX side, if a tx_report.sn has been existing in the bitmap.
>
> >
> > > @@ -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?
>
> Right, this all makes sense. Will fix it, thanks!
>
> >
> >
> > > +
> > > 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?
>
> Well, rtw89_usb_write_port_complete() is asynchronous URB callback managed
> by USB subsystem and it's mainly independent of rtw89. We're guaranteed
> all pending URB completions will complete when the device is being
> disconnected and rtw89_usb_disconnect() method is called. That's all, I
> think.
>
> In other call sites like SER we risk URB completion be called after
> purging the queue, the only consequence will be the obsolete skb still
> added to the queue.
>
> We can implement an anchor for TX URBs and explicitly wait with
> usb_kill_anchored_urbs() in ->reset() until all pending URB completions
> are done, and then purge the queue.
>
> If nothing else, I'll add it in the next respin of the series.
Good to me. Additionally, I'd like to add a comment right after hci->reset
to explicitly point out we should complete all tx_wait, such as
static inline void rtw89_hci_reset(struct rtw89_dev *rtwdev)
{
rtwdev->hci.ops->reset(rtwdev);
/* hci.ops->reset must complete tx_wait of all pending TX SKB */
rtw89_tx_wait_list_clear(rtwdev);
}
Powered by blists - more mailing lists