RE: [PATCH rtw-next v3 5/9] wifi: rtw89: implement C2H TX report handler

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

Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> rtw89 has several ways of handling TX status report events.  The first one
> is based on RPP feature which is used by PCIe HCI.  The other one depends
> on firmware sending a corresponding C2H message, quite similar to what
> rtw88 has.
> 
> Toggle a bit in the TX descriptor to indicate to the firmware that TX
> report for the frame is expected.   This will allow handling TX wait skbs
> and the ones flagged with IEEE80211_TX_CTL_REQ_TX_STATUS correctly.
> 
> Do the bulk of the patch according to the vendor driver for RTL8851BU.
> However, there are slight differences in C2H message format between
> different types of chips.  RTL885xB ones follow format V0.  RTL8852C has
> format V1, and RTL8922AU has format V2.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Suggested-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---

[...]

> diff --git a/drivers/net/wireless/realtek/rtw89/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h
> index ddebf7972068..e0e587ec4592 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.h
> +++ b/drivers/net/wireless/realtek/rtw89/fw.h
> @@ -3747,6 +3747,47 @@ struct rtw89_c2h_scanofld {
>  #define RTW89_GET_MAC_C2H_MCC_REQ_ACK_H2C_FUNC(c2h) \
>         le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(15, 8))
> 
> +struct rtw89_c2h_mac_tx_rpt {
> +       struct rtw89_c2h_hdr hdr;
> +       __le32 w2;
> +       __le32 w3;
> +       __le32 w4;
> +       __le32 w5;
> +       __le32 w6;
> +       __le32 w7;
> +};

__packed

> +
> +#define RTW89_C2H_MAC_TX_RPT_W2_TX_STATE GENMASK(7, 6)
> +#define RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE GENMASK(11, 8)
> +#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT GENMASK(13, 8)
> +#define RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1 GENMASK(15, 10)
> +
> +struct rtw89_c2h_mac_tx_rpt_v2 {
> +       struct rtw89_c2h_hdr hdr;
> +       __le32 w2;
> +       __le32 w3;
> +       __le32 w4;
> +       __le32 w5;
> +       __le32 w6;
> +       __le32 w7;
> +       __le32 w8;
> +       __le32 w9;
> +       __le32 w10;
> +       __le32 w11;
> +       __le32 w12;
> +       __le32 w13;
> +       __le32 w14;
> +       __le32 w15;
> +       __le32 w16;
> +       __le32 w17;
> +       __le32 w18;
> +       __le32 w19;
> +};

__packed

> +
> +#define RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2 GENMASK(9, 8)
> +#define RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2 GENMASK(15, 12)
> +#define RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2 GENMASK(15, 10)
> +
>  struct rtw89_mac_mcc_tsf_rpt {
>         u32 macid_x;
>         u32 macid_y;
> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
> index fd11b8fb3c89..2fe239f18534 100644
> --- a/drivers/net/wireless/realtek/rtw89/mac.c
> +++ b/drivers/net/wireless/realtek/rtw89/mac.c
> @@ -5457,6 +5457,35 @@ 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_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> +{
> +       u8 sw_define, tx_status, data_txcnt;
> +
> +       if (rtwdev->chip->chip_id == RTL8922A) {

Add a chip_ops c2h_tx_rpt? Then, no need chip_id checking, and reduce line
length (normally we prefer shorter than 80 or 90 characters; over 100 characters
isn't a good idea).

Maybe this is because you want to store the status into local variables.
With a chip_ops, you should define another struct to store them.

Or, you just keep it as was, but wrap lines to be shorter, and give shorter
naming. For example,
  - rpt_v2 -> v2

  - data_txcnt -> txcnt

		if (rtwdev->chip->chip_id == RTL8852C)
			txcnt = le32_get_bits(rpt->w5,
					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
		else
			txcnt = le32_get_bits(rpt->w5,
					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);


> +               const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> +
> +               rpt_v2 = (const struct rtw89_c2h_mac_tx_rpt_v2 *)c2h->data;
> +               sw_define = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2);
> +               tx_status = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2);
> +               data_txcnt = le32_get_bits(rpt_v2->w14, RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2);
> +       } else {
> +               const struct rtw89_c2h_mac_tx_rpt *rpt;
> +
> +               rpt = (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
> +               sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
> +               tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
> +               if (rtwdev->chip->chip_id == RTL8852C)
> +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> +               else
> +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> +       }
> +
> +       rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> +                   "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
> +                   sw_define, tx_status, data_txcnt);
> +}
> +
>  static void
>  rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
>  {

[...]