Message ID | 20230310034631.45299-2-pkshih@realtek.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Kalle Valo |
Headers | show |
Series | wifi: rtw89: preparation of multiple interface concurrency support | expand |
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. > @@ -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.
> -----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
> -----Original Message----- > From: Ping-Ke Shih <pkshih@realtek.com> > Sent: Wednesday, March 15, 2023 4:58 PM > To: Kalle Valo <kvalo@kernel.org> > 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 > > > -----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); > 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); Ping-Ke
On Wed, 2023-03-15 at 19:45 +0800, Ping-Ke Shih wrote: > > -----Original Message----- > > From: Ping-Ke Shih <pkshih@realtek.com> > > Sent: Wednesday, March 15, 2023 4:58 PM > > To: Kalle Valo <kvalo@kernel.org> > > 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 > > > > > -----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); > > > > 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
(changing the subject and adding Arnd) Ping-Ke Shih <pkshih@realtek.com> writes: >> > @@ -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); I was thinking more something towards Arnd's idea he suggests in [1]. Here's my proposal for the beacon filter command as pseudo code (so not compiled and very much buggy!) from the patch[2] which started this recent discussion. So in the header file we would have something like this: #define RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK GENMASK(7, 0) #define RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK GENMASK(9, 8) #define RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK GENMASK(11, 10) #define RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK GENMASK(23, 16) struct rtw89_h2c_cfg_beacon_filter { __le32 word0; } static inline void rtw89_h2c_cfg_beacon_filter_set_word0(struct rtw89_h2c_cfg_beacon_filter *cmd, u32 macid, u32 type, u32 event_mask, u32 ma) { le32_replace_bits(cmd->word0, macid, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); le32_replace_bits(cmd->word0, type, RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK); le32_replace_bits(cmd->word0, event, RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK); le32_replace_bits(cmd->word0, ma, RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK); } static inline u32 rtw89_h2c_cfg_beacon_filter_get_mac_id(const struct rtw89_h2c_cfg_beacon_filter *cmd) { return le32_get_bits(cmd->word0, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); } And an example how to use these: struct rtw89_h2c_cfg_beacon_filter *cmd; skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, sizeof(*cmd)); cmd = (struct rtw89_h2c_cfg_beacon_filter *)skb->data; rtw89_h2c_cfg_beacon_filter_set_word0(cmd, 1, 2, 0, 0); I'm sure this is very buggy and I'm missing a lot but I hope you get the idea anyway. My keypoints here are: * there's a clear struct for the command (an "object" from OOP point of view), something like "__le32 *c2h" is very confusing * no casting * no pointer arithmetic * you get length with a simple "sizeof(*cmd)" Downside of course is that there's quite a lot of boilerplate code but I still consider that positives outweight the negatives. Thoughts? And I'll emphasise that this is not a blocker for anything but it would be nice to clean this up both in rtw88 and rtw89 at some point, if we can. [1] https://lore.kernel.org/all/CAK8P3a1rsKZZKMKFTDWgE3usX9gYKJqUvTMxSdEuZrp8BaKdaA@mail.gmail.com/ [2] https://patchwork.kernel.org/project/linux-wireless/patch/20230310034631.45299-2-pkshih@realtek.com/
Kalle Valo <kvalo@kernel.org> writes: > (changing the subject and adding Arnd) > > Ping-Ke Shih <pkshih@realtek.com> writes: > >>> > @@ -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); > > I was thinking more something towards Arnd's idea he suggests in [1]. > Here's my proposal for the beacon filter command as pseudo code (so not > compiled and very much buggy!) from the patch[2] which started this > recent discussion. > > So in the header file we would have something like this: > > #define RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK GENMASK(7, 0) > #define RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK GENMASK(9, 8) > #define RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK GENMASK(11, 10) > #define RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK GENMASK(23, 16) > > struct rtw89_h2c_cfg_beacon_filter { > __le32 word0; > } > > static inline void rtw89_h2c_cfg_beacon_filter_set_word0(struct rtw89_h2c_cfg_beacon_filter *cmd, > u32 macid, u32 type, u32 event_mask, u32 ma) > > { > le32_replace_bits(cmd->word0, macid, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); > le32_replace_bits(cmd->word0, type, RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK); > le32_replace_bits(cmd->word0, event, RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK); > le32_replace_bits(cmd->word0, ma, RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK); > } > > static inline u32 rtw89_h2c_cfg_beacon_filter_get_mac_id(const struct rtw89_h2c_cfg_beacon_filter *cmd) > { > return le32_get_bits(cmd->word0, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); > } > > And an example how to use these: > > struct rtw89_h2c_cfg_beacon_filter *cmd; > > skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, sizeof(*cmd)); > cmd = (struct rtw89_h2c_cfg_beacon_filter *)skb->data; > rtw89_h2c_cfg_beacon_filter_set_word0(cmd, 1, 2, 0, 0); > > I'm sure this is very buggy and I'm missing a lot but I hope you get the > idea anyway. My keypoints here are: > > * there's a clear struct for the command (an "object" from OOP point of > view), something like "__le32 *c2h" is very confusing > * no casting > * no pointer arithmetic > * you get length with a simple "sizeof(*cmd)" > > Downside of course is that there's quite a lot of boilerplate code but I > still consider that positives outweight the negatives. Thoughts? > > And I'll emphasise that this is not a blocker for anything but it would > be nice to clean this up both in rtw88 and rtw89 at some point, if we > can. Heh, I didn't notice that Ping had done almost the same in v4: https://patchwork.kernel.org/project/linux-wireless/patch/20230320124125.15873-2-pkshih@realtek.com/ The only difference I notice that you didn't use special functions for setting or getting the fields: h2c->w0 = le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_RSSI) | le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_BCN) | le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_EN) | le32_encode_bits(RTW89_BCN_FLTR_OFFLOAD_MODE_DEFAULT, RTW89_H2C_BCNFLTR_W0_MODE) | le32_encode_bits(RTW89_BCN_LOSS_CNT, RTW89_H2C_BCNFLTR_W0_BCN_LOSS_CNT) | le32_encode_bits(bss_conf->cqm_rssi_hyst, RTW89_H2C_BCNFLTR_W0_RSSI_HYST) | le32_encode_bits(bss_conf->cqm_rssi_thold + MAX_RSSI, RTW89_H2C_BCNFLTR_W0_RSSI_THRESHOLD) | le32_encode_bits(rtwvif->mac_id, RTW89_H2C_BCNFLTR_W0_MAC_ID); And I understand why you did it like this, less boilerplate code. So looks good to me, thanks!
On Mon, 2023-04-03 at 16:23 +0300, Kalle Valo wrote: > > Kalle Valo <kvalo@kernel.org> writes: > > > (changing the subject and adding Arnd) > > > > Ping-Ke Shih <pkshih@realtek.com> writes: > > > > > > > @@ -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); > > > > I was thinking more something towards Arnd's idea he suggests in [1]. > > Here's my proposal for the beacon filter command as pseudo code (so not > > compiled and very much buggy!) from the patch[2] which started this > > recent discussion. > > > > So in the header file we would have something like this: > > > > #define RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK GENMASK(7, 0) > > #define RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK GENMASK(9, 8) > > #define RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK GENMASK(11, 10) > > #define RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK GENMASK(23, 16) > > > > struct rtw89_h2c_cfg_beacon_filter { > > __le32 word0; > > } > > > > static inline void rtw89_h2c_cfg_beacon_filter_set_word0(struct rtw89_h2c_cfg_beacon_filter > > *cmd, > > u32 macid, u32 type, u32 event_mask, u32 ma) > > > > { > > le32_replace_bits(cmd->word0, macid, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); > > le32_replace_bits(cmd->word0, type, RTW89_C2H_BEACON_FILTER_WORD0_TYPE_MASK); > > le32_replace_bits(cmd->word0, event, RTW89_C2H_BEACON_FILTER_WORD0_EVENT_MASK); > > le32_replace_bits(cmd->word0, ma, RTW89_C2H_BEACON_FILTER_WORD0_MA_MASK); > > } > > > > static inline u32 rtw89_h2c_cfg_beacon_filter_get_mac_id(const struct > > rtw89_h2c_cfg_beacon_filter *cmd) > > { > > return le32_get_bits(cmd->word0, RTW89_C2H_BEACON_FILTER_WORD0_MACID_MASK); > > } > > > > And an example how to use these: > > > > struct rtw89_h2c_cfg_beacon_filter *cmd; > > > > skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, sizeof(*cmd)); > > cmd = (struct rtw89_h2c_cfg_beacon_filter *)skb->data; > > rtw89_h2c_cfg_beacon_filter_set_word0(cmd, 1, 2, 0, 0); > > > > I'm sure this is very buggy and I'm missing a lot but I hope you get the > > idea anyway. My keypoints here are: > > > > * there's a clear struct for the command (an "object" from OOP point of > > view), something like "__le32 *c2h" is very confusing > > * no casting > > * no pointer arithmetic > > * you get length with a simple "sizeof(*cmd)" Super appreciate to propose pseudo code and these clear rules to us. :-) I will record these rules in our internal wiki page. > > > > Downside of course is that there's quite a lot of boilerplate code but I > > still consider that positives outweight the negatives. Thoughts? > > > > And I'll emphasise that this is not a blocker for anything but it would > > be nice to clean this up both in rtw88 and rtw89 at some point, if we > > can. Since they will be a lot of works and I have a lot of local patches on hand, can I apply these rules to the patches and submit them ahead? Until all things or a bunch of conversion are completed (maybe weeks or one or two months later), I can submit patches that only convert these H2C/C2H with new rules. Does it work to you? > > Heh, I didn't notice that Ping had done almost the same in v4: > > https://patchwork.kernel.org/project/linux-wireless/patch/20230320124125.15873-2-pkshih@realtek.com/ > > The only difference I notice that you didn't use special functions for > setting or getting the fields: > > h2c->w0 = le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_RSSI) | > le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_BCN) | > le32_encode_bits(connect, RTW89_H2C_BCNFLTR_W0_MON_EN) | > le32_encode_bits(RTW89_BCN_FLTR_OFFLOAD_MODE_DEFAULT, > RTW89_H2C_BCNFLTR_W0_MODE) | > le32_encode_bits(RTW89_BCN_LOSS_CNT, RTW89_H2C_BCNFLTR_W0_BCN_LOSS_CNT) | > le32_encode_bits(bss_conf->cqm_rssi_hyst, RTW89_H2C_BCNFLTR_W0_RSSI_HYST) | > le32_encode_bits(bss_conf->cqm_rssi_thold + MAX_RSSI, > RTW89_H2C_BCNFLTR_W0_RSSI_THRESHOLD) | > le32_encode_bits(rtwvif->mac_id, RTW89_H2C_BCNFLTR_W0_MAC_ID); > > And I understand why you did it like this, less boilerplate code. So > looks good to me, thanks! Sorry, I forget to mention this in original discussion thread. As I mentioned in v4, I will open mind to convert them again if any suggestion to improve these things in the future. Appreciate your suggestion again. Ping-Ke
Ping-Ke Shih <pkshih@realtek.com> writes: >> > Downside of course is that there's quite a lot of boilerplate code but I >> > still consider that positives outweight the negatives. Thoughts? >> > >> > And I'll emphasise that this is not a blocker for anything but it would >> > be nice to clean this up both in rtw88 and rtw89 at some point, if we >> > can. > > Since they will be a lot of works and I have a lot of local patches on > hand, can I apply these rules to the patches and submit them ahead? > Until all things or a bunch of conversion are completed (maybe weeks or > one or two months later), I can submit patches that only convert > these H2C/C2H with new rules. > > Does it work to you? Yes, that's totally fine. No rush with this cleanup. Thanks for working on this.
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c index 489fa7a86160d..43080d50db171 100644 --- 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; const u8 *bssid = iter_data->bssid; if (rtwdev->scanning && @@ -1456,8 +1458,11 @@ static void rtw89_vif_rx_stats_iter(void *data, u8 *mac, if (!ether_addr_equal(vif->bss_conf.bssid, bssid)) return; - if (ieee80211_is_beacon(hdr->frame_control)) + if (ieee80211_is_beacon(hdr->frame_control)) { + if (vif->type == NL80211_IFTYPE_STATION) + rtw89_fw_h2c_rssi_offload(rtwdev, phy_ppdu); pkt_stat->beacon_nr++; + } if (!ether_addr_equal(vif->addr, hdr->addr1)) return; @@ -2520,6 +2525,9 @@ int rtw89_core_sta_disassoc(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif = (struct rtw89_vif *)vif->drv_priv; struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv; + if (vif->type == NL80211_IFTYPE_STATION) + rtw89_fw_h2c_set_bcn_fltr_cfg(rtwdev, vif, false); + rtwdev->total_sta_assoc--; if (sta->tdls) rtwvif->tdls_peer--; @@ -3394,6 +3402,8 @@ static int rtw89_core_register_hw(struct rtw89_dev *rtwdev) ieee80211_hw_set(hw, SINGLE_SCAN_ON_ALL_BANDS); ieee80211_hw_set(hw, SUPPORTS_MULTI_BSSID); ieee80211_hw_set(hw, WANT_MONITOR_VIF); + if (RTW89_CHK_FW_FEATURE(BEACON_FILTER, &rtwdev->fw)) + ieee80211_hw_set(hw, CONNECTION_MONITOR); hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) | BIT(NL80211_IFTYPE_AP) | diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h index b1a886898c5a0..aefc95f83d277 100644 --- a/drivers/net/wireless/realtek/rtw89/core.h +++ b/drivers/net/wireless/realtek/rtw89/core.h @@ -3026,6 +3026,7 @@ enum rtw89_fw_feature { RTW89_FW_FEATURE_NO_PACKET_DROP, RTW89_FW_FEATURE_NO_DEEP_PS, RTW89_FW_FEATURE_NO_LPS_PG, + RTW89_FW_FEATURE_BEACON_FILTER, }; struct rtw89_fw_suit { diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c index 1a4ff24078fb9..2088b56263dfa 100644 --- a/drivers/net/wireless/realtek/rtw89/fw.c +++ b/drivers/net/wireless/realtek/rtw89/fw.c @@ -266,6 +266,7 @@ static const struct __fw_feat_cfg fw_feat_tbl[] = { __CFG_FW_FEAT(RTL8852C, ge, 0, 27, 34, 0, TX_WAKE), __CFG_FW_FEAT(RTL8852C, ge, 0, 27, 36, 0, SCAN_OFFLOAD), __CFG_FW_FEAT(RTL8852C, ge, 0, 27, 40, 0, CRASH_TRIGGER), + __CFG_FW_FEAT(RTL8852C, ge, 0, 27, 56, 10, BEACON_FILTER), }; static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev) @@ -1735,6 +1736,101 @@ int rtw89_fw_h2c_set_ofld_cfg(struct rtw89_dev *rtwdev) return ret; } +#define H2C_CFG_BCN_FLTR_LEN 4 +int rtw89_fw_h2c_set_bcn_fltr_cfg(struct rtw89_dev *rtwdev, + struct ieee80211_vif *vif, + bool connect) +{ + struct rtw89_vif *rtwvif = vif_to_rtwvif_safe(vif); + struct ieee80211_bss_conf *bss_conf = vif ? &vif->bss_conf : NULL; + struct sk_buff *skb; + int ret; + + if (!RTW89_CHK_FW_FEATURE(BEACON_FILTER, &rtwdev->fw)) + return -EINVAL; + + if (!rtwvif || !bss_conf || rtwvif->net_type != RTW89_NET_TYPE_INFRA) + return -EINVAL; + + skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, H2C_CFG_BCN_FLTR_LEN); + if (!skb) { + rtw89_err(rtwdev, "failed to alloc skb for h2c bcn filter\n"); + return -ENOMEM; + } + + skb_put(skb, H2C_CFG_BCN_FLTR_LEN); + RTW89_SET_BCNFLTR_MON_RSSI(skb->data, connect); + RTW89_SET_BCNFLTR_MON_BCN(skb->data, connect); + RTW89_SET_BCNFLTR_MON_EN(skb->data, connect); + RTW89_SET_BCNFLTR_MODE(skb->data, RTW89_BCN_FLTR_OFFLOAD_MODE_DEFAULT); + RTW89_SET_BCNFLTR_BCN_LOSS_CNT(skb->data, RTW89_BCN_LOSS_CNT); + RTW89_SET_BCNFLTR_RSSI_HYST(skb->data, bss_conf->cqm_rssi_hyst); + RTW89_SET_BCNFLTR_RSSI_THRESHOLD(skb->data, + bss_conf->cqm_rssi_thold + MAX_RSSI); + RTW89_SET_BCNFLTR_MAC_ID(skb->data, rtwvif->mac_id); + + rtw89_h2c_pkt_set_hdr(rtwdev, skb, FWCMD_TYPE_H2C, + H2C_CAT_MAC, H2C_CL_MAC_FW_OFLD, + H2C_FUNC_CFG_BCNFLTR, 0, 1, + H2C_CFG_BCN_FLTR_LEN); + + ret = rtw89_h2c_tx(rtwdev, skb, false); + if (ret) { + rtw89_err(rtwdev, "failed to send h2c\n"); + goto fail; + } + + return 0; +fail: + dev_kfree_skb_any(skb); + + return ret; +} + +#define H2C_RSSI_OFLD_LEN 8 +int rtw89_fw_h2c_rssi_offload(struct rtw89_dev *rtwdev, + struct rtw89_rx_phy_ppdu *phy_ppdu) +{ + struct sk_buff *skb; + s8 rssi; + int ret; + + if (!RTW89_CHK_FW_FEATURE(BEACON_FILTER, &rtwdev->fw)) + return -EINVAL; + + if (!phy_ppdu) + return -EINVAL; + + skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, H2C_RSSI_OFLD_LEN); + if (!skb) { + rtw89_err(rtwdev, "failed to alloc skb for h2c rssi\n"); + return -ENOMEM; + } + + rssi = phy_ppdu->rssi_avg >> RSSI_FACTOR; + skb_put(skb, H2C_RSSI_OFLD_LEN); + RTW89_SET_OFLD_RSSI_MACID(skb->data, phy_ppdu->mac_id); + RTW89_SET_OFLD_RSSI_NUM(skb->data, 1); + RTW89_SET_OFLD_RSSI_VAL(skb->data, rssi); + + rtw89_h2c_pkt_set_hdr(rtwdev, skb, FWCMD_TYPE_H2C, + H2C_CAT_MAC, H2C_CL_MAC_FW_OFLD, + H2C_FUNC_OFLD_RSSI, 0, 1, + H2C_RSSI_OFLD_LEN); + + ret = rtw89_h2c_tx(rtwdev, skb, false); + if (ret) { + rtw89_err(rtwdev, "failed to send h2c\n"); + goto fail; + } + + return 0; +fail: + dev_kfree_skb_any(skb); + + return ret; +} + #define H2C_RA_LEN 16 int rtw89_fw_h2c_ra(struct rtw89_dev *rtwdev, struct rtw89_ra_info *ra, bool csi) { diff --git a/drivers/net/wireless/realtek/rtw89/fw.h b/drivers/net/wireless/realtek/rtw89/fw.h index 3f6e0871381df..efcd88c41eb9e 100644 --- a/drivers/net/wireless/realtek/rtw89/fw.h +++ b/drivers/net/wireless/realtek/rtw89/fw.h @@ -162,6 +162,27 @@ enum rtw89_p2pps_action { RTW89_P2P_ACT_TERMINATE = 3, }; +enum rtw89_bcn_fltr_offload_mode { + RTW89_BCN_FLTR_OFFLOAD_MODE_0 = 0, + RTW89_BCN_FLTR_OFFLOAD_MODE_1, + RTW89_BCN_FLTR_OFFLOAD_MODE_2, + RTW89_BCN_FLTR_OFFLOAD_MODE_3, + + RTW89_BCN_FLTR_OFFLOAD_MODE_DEFAULT = RTW89_BCN_FLTR_OFFLOAD_MODE_0, +}; + +enum rtw89_bcn_fltr_type { + RTW89_BCN_FLTR_BEACON_LOSS, + RTW89_BCN_FLTR_RSSI, + RTW89_BCN_FLTR_NOTIFY, +}; + +enum rtw89_bcn_fltr_rssi_event { + RTW89_BCN_FLTR_RSSI_NOT_CHANGED, + RTW89_BCN_FLTR_RSSI_HIGH, + RTW89_BCN_FLTR_RSSI_LOW, +}; + #define FWDL_SECTION_MAX_NUM 10 #define FWDL_SECTION_CHKSUM_LEN 8 #define FWDL_SECTION_PER_PKT_LEN 2020 @@ -216,6 +237,8 @@ struct rtw89_h2creg_sch_tx_en { #define RTW89_SCAN_LIST_LIMIT \ ((RTW89_H2C_MAX_SIZE / RTW89_MAC_CHINFO_SIZE) - RTW89_SCAN_LIST_GUARD) +#define RTW89_BCN_LOSS_CNT 10 + struct rtw89_mac_chinfo { u8 period; u8 dwell_time; @@ -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)) + #define RTW89_GET_PHY_C2H_RA_RPT_MACID(c2h) \ le32_get_bits(*((const __le32 *)(c2h) + 2), GENMASK(15, 0)) #define RTW89_GET_PHY_C2H_RA_RPT_RETRY_RATIO(c2h) \ @@ -3274,6 +3306,61 @@ static_assert(sizeof(struct rtw89_mac_mcc_tsf_rpt) <= RTW89_COMPLETION_BUF_SIZE) #define RTW89_GET_MAC_C2H_MCC_STATUS_RPT_TSF_HIGH(c2h) \ le32_get_bits(*((const __le32 *)(c2h) + 4), GENMASK(31, 0)) +static inline void RTW89_SET_BCNFLTR_MON_RSSI(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, BIT(0)); +} + +static inline void RTW89_SET_BCNFLTR_MON_BCN(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, BIT(1)); +} + +static inline void RTW89_SET_BCNFLTR_MON_EN(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, BIT(2)); +} + +static inline void RTW89_SET_BCNFLTR_MODE(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(4, 3)); +} + +static inline void RTW89_SET_BCNFLTR_BCN_LOSS_CNT(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(11, 8)); +} + +static inline void RTW89_SET_BCNFLTR_RSSI_HYST(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(15, 12)); +} + +static inline void RTW89_SET_BCNFLTR_RSSI_THRESHOLD(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(23, 16)); +} + +static inline void RTW89_SET_BCNFLTR_MAC_ID(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(31, 24)); +} + +static inline void RTW89_SET_OFLD_RSSI_MACID(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(7, 0)); +} + +static inline void RTW89_SET_OFLD_RSSI_NUM(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x00, val, GENMASK(15, 8)); +} + +static inline void RTW89_SET_OFLD_RSSI_VAL(void *cmd, u32 val) +{ + le32p_replace_bits((__le32 *)cmd + 0x01, val, GENMASK(7, 0)); +} + #define RTW89_FW_HDR_SIZE 32 #define RTW89_FW_SECTION_HDR_SIZE 16 @@ -3401,6 +3488,8 @@ struct rtw89_fw_h2c_rf_reg_info { #define H2C_FUNC_ADD_SCANOFLD_CH 0x16 #define H2C_FUNC_SCANOFLD 0x17 #define H2C_FUNC_PKT_DROP 0x1b +#define H2C_FUNC_CFG_BCNFLTR 0x1e +#define H2C_FUNC_OFLD_RSSI 0x1f /* CLASS 10 - Security CAM */ #define H2C_CL_MAC_SEC_CAM 0xa @@ -3501,6 +3590,11 @@ int rtw89_fw_h2c_macid_pause(struct rtw89_dev *rtwdev, u8 sh, u8 grp, int rtw89_fw_h2c_set_edca(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif, u8 ac, u32 val); int rtw89_fw_h2c_set_ofld_cfg(struct rtw89_dev *rtwdev); +int rtw89_fw_h2c_set_bcn_fltr_cfg(struct rtw89_dev *rtwdev, + struct ieee80211_vif *vif, + bool connect); +int rtw89_fw_h2c_rssi_offload(struct rtw89_dev *rtwdev, + struct rtw89_rx_phy_ppdu *phy_ppdu); int rtw89_fw_h2c_ra(struct rtw89_dev *rtwdev, struct rtw89_ra_info *ra, bool csi); int rtw89_fw_h2c_cxdrv_init(struct rtw89_dev *rtwdev); int rtw89_fw_h2c_cxdrv_role(struct rtw89_dev *rtwdev); diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c index 3d1e4ffef1b16..0e1471c34be00 100644 --- a/drivers/net/wireless/realtek/rtw89/mac.c +++ b/drivers/net/wireless/realtek/rtw89/mac.c @@ -4235,6 +4235,62 @@ rtw89_mac_c2h_scanofld_rsp(struct rtw89_dev *rtwdev, struct sk_buff *c2h, } } +static void +rtw89_mac_bcn_fltr_rpt(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif, + struct sk_buff *skb) +{ + struct ieee80211_vif *vif = rtwvif_to_vif_safe(rtwvif); + enum nl80211_cqm_rssi_threshold_event nl_event; + u8 type, event, mac_id; + s8 sig; + + type = RTW89_GET_MAC_BCNFLTR_RPT_TYPE(skb->data); + sig = RTW89_GET_MAC_BCNFLTR_RPT_MA(skb->data) - MAX_RSSI; + event = RTW89_GET_MAC_BCNFLTR_RPT_EVENT(skb->data); + mac_id = RTW89_GET_MAC_BCNFLTR_RPT_MACID(skb->data); + + if (mac_id != rtwvif->mac_id) + return; + + rtw89_debug(rtwdev, RTW89_DBG_FW, + "C2H bcnfltr rpt macid: %d, type: %d, ma: %d, event: %d\n", + mac_id, type, sig, event); + + switch (type) { + case RTW89_BCN_FLTR_BEACON_LOSS: + if (!rtwdev->scanning) + ieee80211_connection_loss(vif); + else + rtw89_fw_h2c_set_bcn_fltr_cfg(rtwdev, vif, true); + return; + case RTW89_BCN_FLTR_NOTIFY: + nl_event = NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH; + break; + case RTW89_BCN_FLTR_RSSI: + if (event == RTW89_BCN_FLTR_RSSI_LOW) + nl_event = NL80211_CQM_RSSI_THRESHOLD_EVENT_LOW; + else if (event == RTW89_BCN_FLTR_RSSI_HIGH) + nl_event = NL80211_CQM_RSSI_THRESHOLD_EVENT_HIGH; + else + return; + break; + default: + return; + } + + ieee80211_cqm_rssi_notify(vif, nl_event, sig, GFP_KERNEL); +} + +static void +rtw89_mac_c2h_bcn_fltr_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, + u32 len) +{ + struct rtw89_vif *rtwvif; + + rtw89_for_each_rtwvif(rtwdev, rtwvif) + rtw89_mac_bcn_fltr_rpt(rtwdev, rtwvif, c2h); +} + static void rtw89_mac_c2h_rec_ack(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len) { @@ -4455,6 +4511,7 @@ void (* const rtw89_mac_c2h_ofld_handler[])(struct rtw89_dev *rtwdev, [RTW89_MAC_C2H_FUNC_MACID_PAUSE] = rtw89_mac_c2h_macid_pause, [RTW89_MAC_C2H_FUNC_SCANOFLD_RSP] = rtw89_mac_c2h_scanofld_rsp, [RTW89_MAC_C2H_FUNC_TSF32_TOGL_RPT] = rtw89_mac_c2h_tsf32_toggle_rpt, + [RTW89_MAC_C2H_FUNC_BCNFLTR_RPT] = rtw89_mac_c2h_bcn_fltr_rpt, }; static diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h index 8064d3953d7f2..899c45c6774e2 100644 --- a/drivers/net/wireless/realtek/rtw89/mac.h +++ b/drivers/net/wireless/realtek/rtw89/mac.h @@ -359,6 +359,7 @@ enum rtw89_mac_c2h_ofld_func { RTW89_MAC_C2H_FUNC_MACID_PAUSE, RTW89_MAC_C2H_FUNC_TSF32_TOGL_RPT = 0x6, RTW89_MAC_C2H_FUNC_SCANOFLD_RSP = 0x9, + RTW89_MAC_C2H_FUNC_BCNFLTR_RPT = 0xd, RTW89_MAC_C2H_FUNC_OFLD_MAX, }; diff --git a/drivers/net/wireless/realtek/rtw89/mac80211.c b/drivers/net/wireless/realtek/rtw89/mac80211.c index 367a7bf319dae..629b32dafecb8 100644 --- a/drivers/net/wireless/realtek/rtw89/mac80211.c +++ b/drivers/net/wireless/realtek/rtw89/mac80211.c @@ -114,6 +114,11 @@ static int rtw89_ops_add_interface(struct ieee80211_hw *hw, vif->addr, vif->type, vif->p2p); mutex_lock(&rtwdev->mutex); + + if (RTW89_CHK_FW_FEATURE(BEACON_FILTER, &rtwdev->fw)) + vif->driver_flags |= IEEE80211_VIF_BEACON_FILTER | + IEEE80211_VIF_SUPPORTS_CQM_RSSI; + rtwvif->rtwdev = rtwdev; list_add_tail(&rtwvif->list, &rtwdev->rtwvifs_list); INIT_WORK(&rtwvif->update_beacon_work, rtw89_core_update_beacon_work); @@ -425,6 +430,9 @@ static void rtw89_ops_bss_info_changed(struct ieee80211_hw *hw, if (changed & BSS_CHANGED_P2P_PS) rtw89_process_p2p_ps(rtwdev, vif); + if (changed & BSS_CHANGED_CQM) + rtw89_fw_h2c_set_bcn_fltr_cfg(rtwdev, vif, true); + mutex_unlock(&rtwdev->mutex); }