RE: [PATCH rtw-next 4/6] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Ping-Ke Shih <pkshih@realtek.com>]

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> > > +       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.

Thanks for the info. I will write down this for reference myself.

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

I knew this aren't mixed use of API for PCI and USB. I just think if it's
not in IRQ context, we can avoid to use spin_lock_irqsave(), but in fact
it does.


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

I feel before the commit, it also allow 2 'void *' since 
   " void *status_driver_data[16 / sizeof(void *)];"

But Zong-Zhe noted me that original definition is weird, so I change it to
be more reasonable (I hope).

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

As your above information about context, just ignore this. 

I just thought if we can reduce critical section of spin_lock_irqsave(),
but it seems to be no way. So keep it as was if we still no better idea
before getting merged. 

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

Not sure how you will do. Let's see it in your next version. :-)

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

Yes, it looks better. Thanks.