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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251025125848-1a66489b56a61c3e63193431-pchelkin@ispras>
Date: Sat, 25 Oct 2025 13:12:31 +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 v3 5/9] wifi: rtw89: implement C2H TX report
 handler

On Wed, 22. Oct 06:21, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin@...ras.ru> wrote:
> > @@ -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);
> 

The chip_ops variant is more abstract (and good in such a way) but I don't
think it's worth mangling with modifying struct chip_ops of each chip and
adding new structs as we probably won't need to scale or expand
rtw89_mac_c2h_tx_rpt() further unless some another V3 format is supported.
Also other mac C2H functions tend to check chip_id in place if they need.

I'll try to make the lines shorter - the longest one will be 86 symbols.

> 
> > +               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)
> >  {
> 
> [...]
>