Message-ID: <20250929130524-9e0c010a824ad34c47c2e1c4-pchelkin@ispras>
Date: Mon, 29 Sep 2025 17:16:45 +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 4/6] wifi: rtw89: handle
 IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

On Thu, 25. Sep 02:05, Ping-Ke Shih wrote:
> 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? 

Oh, it's intentional "style" used to mark sentence endings more
distinctively.  I've already done that in the previous series.

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

Alright, plain skb_queue_purge() may lead to "Have pending ack frames!"
WARNING later.

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

Depends on the USB host controller if I'm not mistaken.  URB completion
callback may be invoked either in hard IRQ or BH context.
usb_hcd_giveback_urb() has an updated doc stating:

 * Context: atomic. The completion callback is invoked either in a work queue
 * (BH) context or in the caller's context, depending on whether the HCD_BH
 * flag is set in the @hcd structure, except that URBs submitted to the
 * root hub always complete in BH context.

If HCD_BH is not set for the host controller in use then, depending on host
controller, URB handler might be executed in hard IRQ context.

I guess you're implying to unify the usage of ieee80211_tx_status_* for
PCIe (which has ieee80211_tx_status_ni) and USB variants of rtw89.  These
calls are not mixed for the single hardware so there is no real issue
for unification.

> > +}
> > +
> >  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.

Ah, this should work because recent commit 19989c80734c ("wifi: rtw89: use
ieee80211_tx_info::driver_data to store driver TX info") has allowed
storing more than 2 'void *' pointers in private data.

> 
> > +               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(), 

We can't use non-_irqsafe versions here unless rtw89_usb_write_port_complete()
is reworked not to use _irqsafe one.  And there is no way other than
transfering this work from URB completion handler to some other async
worker (in BH context or similar).  Not sure it'll be better overall.

> 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())

Perhaps we can do that with ieee80211_tx_status_irqsafe() still in place.

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

It's me mixing up the terminology, sorry.  The comment was supposed to
indicate that PCI always have TX status reported.  (but it's done via RPP
feature which is actually a separate thing compared to TX Report, okay)

I'd rather replace it with "TX status is reported via RPP" if that comment
is helpful.

Powered by blists - more mailing lists