RE: [PATCH 1/5] wifi: rtw89: 8852c: add beacon filter and CQM support

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

> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Wednesday, March 15, 2023 4:32 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Bernie Huang <phhuang@realtek.com>; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 1/5] wifi: rtw89: 8852c: add beacon filter and CQM support
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Po-Hao Huang <phhuang@realtek.com>
> >
> > Adding this supports beacon filter and connection quality monitor.
> > To make host CPU wake up less, let firmware perform signal
> > monitoring and beacon processing, then notify driver upon signal
> > changes or beacon loss.
> >
> > This feature needs firmware 0.27.56 or newer to support it.
> >
> > Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> [...]
> 
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1438,6 +1438,8 @@ static void rtw89_vif_rx_stats_iter(void *data, u8 *mac,
> >       struct rtw89_rx_desc_info *desc_info = iter_data->desc_info;
> >       struct sk_buff *skb = iter_data->skb;
> >       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> > +     struct rtw89_rx_phy_ppdu *phy_ppdu =
> > +             (struct rtw89_rx_phy_ppdu *)iter_data->phy_ppdu;
> 
> Why the cast? I don't think it's needed.

Will fix it by next version.

> 
> > @@ -3181,6 +3204,15 @@ static inline struct rtw89_fw_c2h_attr *RTW89_SKB_C2H_CB(struct sk_buff *skb)
> >  #define RTW89_GET_MAC_C2H_REV_ACK_H2C_SEQ(c2h) \
> >       le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(23, 16))
> >
> > +#define RTW89_GET_MAC_BCNFLTR_RPT_MACID(c2h) \
> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(7, 0))
> > +#define RTW89_GET_MAC_BCNFLTR_RPT_TYPE(c2h) \
> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(9, 8))
> > +#define RTW89_GET_MAC_BCNFLTR_RPT_EVENT(c2h) \
> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(11, 10))
> > +#define RTW89_GET_MAC_BCNFLTR_RPT_MA(c2h) \
> > +     le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(23, 16))
> 
> I have to admit that I every time I see this code pattern it makes me
> regret it. Something like what Arnd proposed back in the day would look
> so much cleaner:
> 
> https://lore.kernel.org/all/CAK8P3a1rsKZZKMKFTDWgE3usX9gYKJqUvTMxSdEuZrp8BaKdaA@mail.gmail.com/
> 
> Of course this is just a generic comment about rtw89, and has nothing to
> do with this patchset, but it would be great if someone could take a
> look and try out Arnd's proposal. It would be good to start with just
> one or two commands and send that as an RFC to see how it looks like.
> 

I write a draft RFC here. Please see if it's in expectation. If so, I can
change all of them by another patch or RFC.

In header file:

#define RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK GENMASK(7, 0)
#define RTW89_C2H_MAC_BCNFLTR_RPT_W2_TYPE_MASK GENMASK(9, 8)
#define RTW89_C2H_MAC_BCNFLTR_RPT_W2_EVENT_MASK GENMASK(11, 10)
#define RTW89_C2H_MAC_BCNFLTR_RPT_W2_MA_MASK GENMASK(23, 16)


Access the values via le32_get_bits() in functions somewhere:

	const __le32 *c2h = skb->data;

	type =   le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK);
	sig =    le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_MA_MASK) - MAX_RSSI;
	event =  le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_EVENT_MASK);
	mac_id = le32_get_bits(c2h[2], RTW89_C2H_MAC_BCNFLTR_RPT_W2_MACID_MASK);


Ping-Ke