On Wed, 2023-03-15 at 19:45 +0800, Ping-Ke Shih wrote: > > -----Original Message----- > > From: Ping-Ke Shih <pkshih@xxxxxxxxxxx> > > Sent: Wednesday, March 15, 2023 4:58 PM > > To: Kalle Valo <kvalo@xxxxxxxxxx> > > Cc: Bernie Huang <phhuang@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx > > Subject: RE: [PATCH 1/5] wifi: rtw89: 8852c: add beacon filter and CQM support > > > > > -----Original Message----- > > > From: Kalle Valo <kvalo@xxxxxxxxxx> > > > Sent: Wednesday, March 15, 2023 4:32 PM > > > To: Ping-Ke Shih <pkshih@xxxxxxxxxxx> > > > Cc: Bernie Huang <phhuang@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH 1/5] wifi: rtw89: 8852c: add beacon filter and CQM support > > > > > > Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes: > > > > > > > From: Po-Hao Huang <phhuang@xxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxx> > > > > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx> > > > > > > [...] > > > > > > > --- 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@xxxxxxxxxxxxxx/ > > > > > > 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); > > > > I forget to propose a RFC for writing (e.g. H2C commands to firmware), and > here they are: > > __le32 *h2c = skb->data; > > #define RTW89_H2C_DISCONNECT_DETECT_W0_ENABLE BIT(0) > #define RTW89_H2C_DISCONNECT_DETECT_W0_TRYOK_BCNFAIL_COUNT_EN BIT(1) > #define RTW89_H2C_DISCONNECT_DETECT_W0_DISCONNECT BIT(2) > #define RTW89_H2C_DISCONNECT_DETECT_W0_MAC_ID GENMASK(15, 8) > #define RTW89_H2C_DISCONNECT_DETECT_W0_CHECK_PERIOD GENMASK(23, 16) > #define RTW89_H2C_DISCONNECT_DETECT_W0_TRY_PKT_COUNT GENMASK(31, 24) > #define RTW89_H2C_DISCONNECT_DETECT_W1_TRYOK_BCNFAIL_COUNT_LIMIT GENMASK(7, 0) > > h2c[0] = le32_encode_bits(enable, RTW89_H2C_DISCONNECT_DETECT_W0_ENABLE) | > le32_encode_bits(!enable, RTW89_H2C_DISCONNECT_DETECT_W0_DISCONNECT) | > le32_encode_bits(macid, RTW89_H2C_DISCONNECT_DETECT_W0_MAC_ID) | > le32_encode_bits(100, RTW89_H2C_DISCONNECT_DETECT_W0_CHECK_PERIOD); > h2c[1] = le32_encode_bits(5, RTW89_H2C_DISCONNECT_DETECT_W1_TRYOK_BCNFAIL_COUNT_LIMIT); > I have sent v3 (v2 is messed up) with above style and it looks more clear. If this is okay, I will convert existing code to this style. Ping-Ke
