[lvc-project] [PATCH rtw-next v4 08/10] wifi: rtw89: handle IEEE80211_TX_CTL_REQ_TX_STATUS frames for USB

[Ping-Ke Shih]

Fedor Pchelkin <pchelkin at ispras.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.
> 
> 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
> scheme based on processing C2H messages.
> 
> Thus define a .tx_rpt_enabled flag indicating which HCIs need to enable a
> TX report feature.  Currently it is USB only.
> 
> Toggle a bit in the TX descriptor and place flagged skbs in a fix-sized
> queue to wait for a message from the firmware.  Firmware maintains a 4-bit
> sequence number for required frames hence the queue can contain just 16
> elements simultaneously.  That's enough for normal driver / firmware
> communication.  If the firmware crashes for any reason and doesn't provide
> TX reports in time, driver will handle this and report the obsolete frames
> as dropped.
> 
> rtw89 also has a new feature providing a TX report for each transmission
> attempt.  Ignore a failed TX status reported by the firmware until retry
> limit is reached or successful status appears.  When there is no success
> and the retry limit is reached, report the frame up to the wireless stack
> as failed eventually.
> 
> HCI reset should stop all pending TX activity so forcefully flush the
> queue there.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin at ispras.ru>

Thanks for your work. I have only some minor suggestions for v4.

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index 9372e30a0039..015d7833841f 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -3517,6 +3517,14 @@ struct rtw89_phy_rate_pattern {
>  #define RTW89_TX_LIFE_TIME             0x2
>  #define RTW89_TX_MACID_DROP            0x3
> 
> +#define RTW89_MAX_TX_RPTS              16
> +#define RTW89_MAX_TX_RPTS_MASK         (RTW89_MAX_TX_RPTS - 1)
> +struct rtw89_tx_rpt {
> +       struct sk_buff *skbs[RTW89_MAX_TX_RPTS];
> +       spinlock_t skb_lock;

I think we should add a comment to describe how/where this lock can be used.
At least checkpatch will complain this. 

> +       atomic_t sn;
> +};
> +
>  #define RTW89_TX_WAIT_WORK_TIMEOUT msecs_to_jiffies(500)
>  struct rtw89_tx_wait_info {
>         struct rcu_head rcu_head;

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index e4e126a4428b..a6642c5761cb 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5463,7 +5463,10 @@ 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;
> +       struct rtw89_tx_skb_data *skb_data;
>         u8 sw_define, tx_status, txcnt;
> +       struct sk_buff *skb;
> 
>         if (rtwdev->chip->chip_id == RTL8922A) {
>                 const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> @@ -5492,6 +5495,29 @@ 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, txcnt %d\n",
>                     sw_define, tx_status, txcnt);

To claim sw_define is not over size of tx_rpt->skbs[], we can add below:

	static_assert(hweight32(RTW89_MAX_TX_RPTS_MASK) ==
		      hweight32(RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2) &&
		      hweight32(RTW89_MAX_TX_RPTS_MASK) ==
		      hweight32(RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE));


> +
> +       scoped_guard(spinlock_irqsave, &tx_rpt->skb_lock) {
> +               skb = tx_rpt->skbs[sw_define];
> +
> +               /* skip if no skb (normally shouldn't happen) */
> +               if (!skb) {
> +                       rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> +                                   "C2H TX RPT: no skb found in queue\n");
> +                       return;
> +               }
> +
> +               skb_data = RTW89_TX_SKB_CB(skb);
> +
> +               /* skip if TX attempt has failed and retry limit has not been
> +                * reached yet
> +                */
> +               if (tx_status != RTW89_TX_DONE &&
> +                   txcnt != skb_data->tx_pkt_cnt_lmt)
> +                       return;
> +
> +               tx_rpt->skbs[sw_define] = NULL;
> +               rtw89_tx_rpt_tx_status(rtwdev, skb, tx_status);
> +       }
>  }
> 
>  static void

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index c359b469aabe..5e587c93268e 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -216,6 +216,15 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 skb_pull(skb, txdesc_size);
> 
>                 info = IEEE80211_SKB_CB(skb);

Since newly added chunk doesn't use 'info', just do it before
ieee80211_tx_info_clear_status().

> +               if (rtw89_is_tx_rpt_skb(skb)) {
> +                       if (urb->status == 0)
> +                               rtw89_tx_rpt_skb_add(rtwdev, skb);
> +                       else
> +                               rtw89_tx_rpt_tx_status(rtwdev, skb,
> +                                                      RTW89_TX_MACID_DROP);
> +                       continue;
> +               }
> +
>                 ieee80211_tx_info_clear_status(info);
> 
>                 if (urb->status == 0) {

[...]