Message-ID: <20251025131637-d3a03888f5c753e6b213e204-pchelkin@ispras>
Date: Sat, 25 Oct 2025 15:10:59 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Ping-Ke Shih <pkshih@...ltek.com>
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

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.

> > @@ -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?

> 
> > @@ -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.

Powered by blists - more mailing lists