RE: [PATCH rtw-next v3 7/9] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

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

Fedor Pchelkin <pchelkin@ispras.ru> wrote:

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index abe8eec1d0f5..3aa9a9a28118 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1112,6 +1112,9 @@ rtw89_core_tx_update_desc_info(struct rtw89_dev *rtwdev,
>         if (addr_cam->valid && desc_info->mlo)
>                 upd_wlan_hdr = true;
> 
> +       if (rtw89_is_tx_rpt_skb(tx_req->skb))
> +               rtw89_tx_rpt_init(rtwdev, tx_req);
> +
>         is_bmc = (is_broadcast_ether_addr(hdr->addr1) ||
>                   is_multicast_ether_addr(hdr->addr1));
> 
> @@ -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?

>         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 66b7bfa5902e..8641e3a8d36d 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3516,6 +3516,11 @@ struct rtw89_phy_rate_pattern {
>  #define RTW89_TX_LIFE_TIME             0x2
>  #define RTW89_TX_MACID_DROP            0x3
> 
> +struct rtw89_tx_rpt {
> +       struct sk_buff_head queue;
> +       atomic_t sn;
> +};
> +
>  #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
>  struct rtw89_tx_wait_info {
>         struct rcu_head rcu_head;
> @@ -3527,6 +3532,8 @@ struct rtw89_tx_wait_info {
> 
>  struct rtw89_tx_skb_data {
>         struct rtw89_tx_wait_info __rcu *wait;
> +       u8 tx_rpt_sn;
> +       u8 tx_pkt_cnt_lmt;
>         u8 hci_priv[];
>  };
> 
> @@ -3696,6 +3703,7 @@ struct rtw89_hci_info {
>         u32 rpwm_addr;
>         u32 cpwm_addr;
>         bool paused;
> +       bool tx_rpt_enabled;
>  };
> 
>  struct rtw89_chip_ops {
> @@ -6015,6 +6023,8 @@ struct rtw89_dev {
>         struct list_head tx_waits;
>         struct wiphy_delayed_work tx_wait_work;
> 
> +       struct rtw89_tx_rpt tx_rpt;
> +
>         struct rtw89_cam_info cam_info;
> 
>         struct sk_buff_head c2h_queue;
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index 2fe239f18534..26c7476afdec 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5460,7 +5460,11 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
>  static void
>  rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {
> +       struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
>         u8 sw_define, tx_status, data_txcnt;
> +       struct rtw89_tx_skb_data *skb_data;
> +       struct sk_buff *skb, *tmp;
> +       unsigned long flags;
> 
>         if (rtwdev->chip->chip_id == RTL8922A) {
>                 const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> @@ -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()?

> +               break;
> +       }
> +       spin_unlock_irqrestore(&tx_rpt->queue.lock, flags);
>  }
> 
>  static void
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
> index 15c5c7e4033c..e8bd92223497 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1616,4 +1616,60 @@ int rtw89_mac_scan_offload(struct rtw89_dev *rtwdev,
> 
>         return ret;
>  }
> +
> +static inline
> +void rtw89_tx_rpt_init(struct rtw89_dev *rtwdev,
> +                      struct rtw89_core_tx_request *tx_req)
> +{
> +       struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> +
> +       if (!rtwdev->hci.tx_rpt_enabled)
> +               return;
> +
> +       tx_req->desc_info.report = true;
> +       /* firmware maintains a 4-bit sequence number */
> +       tx_req->desc_info.sn = atomic_inc_return(&tx_rpt->sn) & 0xF;
> +       tx_req->desc_info.tx_cnt_lmt_en = true;
> +       tx_req->desc_info.tx_cnt_lmt = 8;
> +}
> +
> +static inline
> +bool rtw89_is_tx_rpt_skb(struct sk_buff *skb)
> +{
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> +       return info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
> +}
> +
> +static inline
> +void rtw89_tx_rpt_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb, u8 tx_status)
> +{
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +
> +       ieee80211_tx_info_clear_status(info);
> +       if (tx_status == RTW89_TX_DONE)
> +               info->flags |= IEEE80211_TX_STAT_ACK;
> +       else
> +               info->flags &= ~IEEE80211_TX_STAT_ACK;
> +
> +       ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> +}
> +
> +static inline
> +void rtw89_tx_rpt_queue_purge(struct rtw89_dev *rtwdev)
> +{
> +       struct rtw89_tx_rpt *tx_rpt = &rtwdev->tx_rpt;
> +       struct sk_buff_head q;
> +       struct sk_buff *skb;
> +       unsigned long flags;
> +
> +       __skb_queue_head_init(&q);
> +
> +       spin_lock_irqsave(&tx_rpt->queue.lock, flags);
> +       skb_queue_splice_init(&tx_rpt->queue, &q);
> +       spin_unlock_irqrestore(&tx_rpt->queue.lock, flags);
> +
> +       while ((skb = __skb_dequeue(&q)))
> +               rtw89_tx_rpt_tx_status(rtwdev, skb, RTW89_TX_MACID_DROP);
> +}
>  #endif
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index 655e8437d62e..22994c3501f8 100644
> --- 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?

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


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

Also, all are dropped, can't we just call ieee80211_purge_tx_queue()?

>  }
> 
>  static int rtw89_usb_ops_start(struct rtw89_dev *rtwdev)
> @@ -962,6 +974,7 @@ int rtw89_usb_probe(struct usb_interface *intf,
> 
>         rtwdev->hci.ops = &rtw89_usb_ops;
>         rtwdev->hci.type = RTW89_HCI_TYPE_USB;
> +       rtwdev->hci.tx_rpt_enabled = true;
> 
>         ret = rtw89_usb_intf_init(rtwdev, intf);
>         if (ret) {
> --
> 2.51.0
>