[PATCH 0/5] wifi: rtw89: preparation of multiple interface concurrency support

[PATCH 0/5] wifi: rtw89: preparation of multiple interface concurrency support

[Ping-Ke Shih]

To support concurrency, we implement beacon filter, CQM and
ieee80211::remain_on_channel ops. Since our firmware doesn't support to TX
null packet while doing remain-on-channel, driver does this instead. To
ensure null packet send out before switching channel, patch 2/5 adds a
waiting mechanism.

The patches 4/5 and 5/5 refine things we found during developing.

Po-Hao Huang (5):
  wifi: rtw89: 8852c: add beacon filter and CQM support
  wifi: rtw89: add function to wait for completion of TX skbs
  wifi: rtw89: add ieee80211::remain_on_channel ops
  wifi: rtw89: add flag check for power state
  wifi: rtw89: fix authentication fail during scan

 drivers/net/wireless/realtek/rtw89/chan.c     |  35 +++
 drivers/net/wireless/realtek/rtw89/chan.h     |   3 +
 drivers/net/wireless/realtek/rtw89/core.c     | 284 +++++++++++++++++-
 drivers/net/wireless/realtek/rtw89/core.h     |  64 ++++
 drivers/net/wireless/realtek/rtw89/fw.c       | 111 ++++++-
 drivers/net/wireless/realtek/rtw89/fw.h       |  94 ++++++
 drivers/net/wireless/realtek/rtw89/mac.c      |  60 ++++
 drivers/net/wireless/realtek/rtw89/mac.h      |   1 +
 drivers/net/wireless/realtek/rtw89/mac80211.c |  88 +++++-
 drivers/net/wireless/realtek/rtw89/pci.c      |   6 +
 drivers/net/wireless/realtek/rtw89/pci.h      |   4 +-
 drivers/net/wireless/realtek/rtw89/ps.c       |   6 +
 drivers/net/wireless/realtek/rtw89/ps.h       |  16 +
 13 files changed, 757 insertions(+), 15 deletions(-)

-- 
2.25.1

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

[Ping-Ke Shih]

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>
---
 drivers/net/wireless/realtek/rtw89/core.c     | 12 ++-
 drivers/net/wireless/realtek/rtw89/core.h     |  1 +
 drivers/net/wireless/realtek/rtw89/fw.c       | 96 +++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/fw.h       | 94 ++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/mac.c      | 57 +++++++++++
 drivers/net/wireless/realtek/rtw89/mac.h      |  1 +
 drivers/net/wireless/realtek/rtw89/mac80211.c |  8 ++
 7 files changed, 268 insertions(+), 1 deletion(-)

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);
 }
 
-- 
2.25.1

[PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Ping-Ke Shih]

From: Po-Hao Huang <phhuang@realtek.com>

Allocate a per-skb completion to track those skbs we are interested in
and wait for them to complete transmission with TX status. To avoid
race condition between process and softirq without addtional locking,
we use a work to free the tx_wait struct later when waiter is finished
referencing it. This must be called in process context and with a
timeout value greater than zero since it might sleep.

We use another workqueue so works can be processed concurrently and
when the PCI device is removed unexpectedly, all pending works can be
flushed. This prevents some works that were scheduled but never processed
leads to memory leak.

Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/core.c | 53 +++++++++++++++++++++++
 drivers/net/wireless/realtek/rtw89/core.h | 33 ++++++++++++++
 drivers/net/wireless/realtek/rtw89/pci.c  |  6 +++
 drivers/net/wireless/realtek/rtw89/pci.h  |  4 +-
 4 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 43080d50db171..369f7fb4e0779 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -849,6 +849,55 @@ void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel)
 	rtw89_hci_tx_kick_off(rtwdev, ch_dma);
 }
 
+static void rtw89_core_free_tx_wait_work(struct work_struct *work)
+{
+	struct rtw89_tx_wait_info *wait =
+			container_of(work, struct rtw89_tx_wait_info, work);
+	struct rtw89_dev *rtwdev = wait->rtwdev;
+	int done, ret;
+
+	ret = read_poll_timeout(atomic_read, done, done, 1000, 100000, false,
+				&wait->wait_done);
+
+	if (ret)
+		rtw89_err(rtwdev, "tx wait timed out, stop polling\n");
+	else
+		kfree(wait);
+}
+
+int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *skb,
+				    int qsel, unsigned int timeout)
+{
+	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
+	struct rtw89_tx_wait_info *wait;
+	unsigned long time_left;
+	int ret = 0;
+
+	skb_data->wait = kzalloc(sizeof(*wait), GFP_KERNEL);
+	wait = skb_data->wait;
+	if (!wait) {
+		rtw89_warn(rtwdev, "alloc tx wait info failed\n");
+		rtw89_core_tx_kick_off(rtwdev, qsel);
+		return 0;
+	}
+
+	init_completion(&wait->completion);
+	INIT_WORK(&wait->work, rtw89_core_free_tx_wait_work);
+	wait->rtwdev = rtwdev;
+
+	rtw89_core_tx_kick_off(rtwdev, qsel);
+	time_left = wait_for_completion_timeout(&wait->completion,
+						msecs_to_jiffies(timeout));
+	if (time_left == 0)
+		ret = -ETIMEDOUT;
+	else if (!wait->tx_done)
+		ret = -EAGAIN;
+
+	atomic_inc(&wait->wait_done);
+
+	return ret;
+}
+
 int rtw89_h2c_tx(struct rtw89_dev *rtwdev,
 		 struct sk_buff *skb, bool fwdl)
 {
@@ -3193,6 +3242,9 @@ int rtw89_core_init(struct rtw89_dev *rtwdev)
 	rtwdev->txq_wq = alloc_workqueue("rtw89_tx_wq", WQ_UNBOUND | WQ_HIGHPRI, 0);
 	if (!rtwdev->txq_wq)
 		return -ENOMEM;
+	rtwdev->gc_wq = alloc_workqueue("rtw89_gc_wq", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
+	if (!rtwdev->gc_wq)
+		return -ENOMEM;
 	spin_lock_init(&rtwdev->ba_lock);
 	spin_lock_init(&rtwdev->rpwm_lock);
 	mutex_init(&rtwdev->mutex);
@@ -3234,6 +3286,7 @@ void rtw89_core_deinit(struct rtw89_dev *rtwdev)
 	rtw89_fw_free_all_early_h2c(rtwdev);
 
 	destroy_workqueue(rtwdev->txq_wq);
+	destroy_workqueue(rtwdev->gc_wq);
 	mutex_destroy(&rtwdev->rf_mutex);
 	mutex_destroy(&rtwdev->mutex);
 }
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index aefc95f83d277..dd4165394c8de 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -2421,6 +2421,19 @@ struct rtw89_phy_rate_pattern {
 	bool enable;
 };
 
+struct rtw89_tx_wait_info {
+	struct rtw89_dev *rtwdev;
+	struct completion completion;
+	struct work_struct work;
+	atomic_t wait_done;
+	bool tx_done;
+};
+
+struct rtw89_tx_skb_data {
+	struct rtw89_tx_wait_info *wait;
+	u8 hci_priv[];
+};
+
 #define RTW89_P2P_MAX_NOA_NUM 2
 
 struct rtw89_vif {
@@ -3758,6 +3771,7 @@ struct rtw89_dev {
 	/* used to protect rf read write */
 	struct mutex rf_mutex;
 	struct workqueue_struct *txq_wq;
+	struct workqueue_struct *gc_wq;
 	struct work_struct txq_work;
 	struct delayed_work txq_reinvoke_work;
 	/* used to protect ba_list and forbid_ba_list */
@@ -3969,6 +3983,14 @@ static inline void rtw89_hci_clear(struct rtw89_dev *rtwdev, struct pci_dev *pde
 		rtwdev->hci.ops->clear(rtwdev, pdev);
 }
 
+static inline
+struct rtw89_tx_skb_data *RTW89_TX_SKB_CB(struct sk_buff *skb)
+{
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+	return (struct rtw89_tx_skb_data *)info->status.status_driver_data;
+}
+
 static inline u8 rtw89_read8(struct rtw89_dev *rtwdev, u32 addr)
 {
 	return rtwdev->hci.ops->read8(rtwdev, addr);
@@ -4612,11 +4634,22 @@ static inline struct sk_buff *rtw89_alloc_skb_for_rx(struct rtw89_dev *rtwdev,
 	return dev_alloc_skb(length);
 }
 
+static inline void rtw89_core_tx_wait_complete(struct rtw89_dev *rtwdev,
+					       struct rtw89_tx_wait_info *wait,
+					       bool tx_done)
+{
+	wait->tx_done = tx_done;
+	complete(&wait->completion);
+	queue_work(rtwdev->gc_wq, &wait->work);
+}
+
 int rtw89_core_tx_write(struct rtw89_dev *rtwdev, struct ieee80211_vif *vif,
 			struct ieee80211_sta *sta, struct sk_buff *skb, int *qsel);
 int rtw89_h2c_tx(struct rtw89_dev *rtwdev,
 		 struct sk_buff *skb, bool fwdl);
 void rtw89_core_tx_kick_off(struct rtw89_dev *rtwdev, u8 qsel);
+int rtw89_core_tx_kick_off_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *skb,
+				    int qsel, unsigned int timeout);
 void rtw89_core_fill_txdesc(struct rtw89_dev *rtwdev,
 			    struct rtw89_tx_desc_info *desc_info,
 			    void *txdesc);
diff --git a/drivers/net/wireless/realtek/rtw89/pci.c b/drivers/net/wireless/realtek/rtw89/pci.c
index ec8bb5f10482e..ca6b63944ee99 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.c
+++ b/drivers/net/wireless/realtek/rtw89/pci.c
@@ -364,6 +364,8 @@ static void rtw89_pci_tx_status(struct rtw89_dev *rtwdev,
 				struct rtw89_pci_tx_ring *tx_ring,
 				struct sk_buff *skb, u8 tx_status)
 {
+	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
+	struct rtw89_tx_wait_info *wait = skb_data->wait;
 	struct ieee80211_tx_info *info;
 
 	info = IEEE80211_SKB_CB(skb);
@@ -394,6 +396,8 @@ static void rtw89_pci_tx_status(struct rtw89_dev *rtwdev,
 		}
 	}
 
+	if (wait)
+		rtw89_core_tx_wait_complete(rtwdev, wait, tx_status == RTW89_TX_DONE);
 	ieee80211_tx_status_ni(rtwdev->hw, skb);
 }
 
@@ -1203,6 +1207,7 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev,
 	struct pci_dev *pdev = rtwpci->pdev;
 	struct sk_buff *skb = tx_req->skb;
 	struct rtw89_pci_tx_data *tx_data = RTW89_PCI_TX_SKB_CB(skb);
+	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
 	bool en_wd_info = desc_info->en_wd_info;
 	u32 txwd_len;
 	u32 txwp_len;
@@ -1218,6 +1223,7 @@ static int rtw89_pci_txwd_submit(struct rtw89_dev *rtwdev,
 	}
 
 	tx_data->dma = dma;
+	skb_data->wait = NULL;
 
 	txwp_len = sizeof(*txwp_info);
 	txwd_len = chip->txwd_body_size;
diff --git a/drivers/net/wireless/realtek/rtw89/pci.h b/drivers/net/wireless/realtek/rtw89/pci.h
index 1e19740db8c54..0e4bd210b100f 100644
--- a/drivers/net/wireless/realtek/rtw89/pci.h
+++ b/drivers/net/wireless/realtek/rtw89/pci.h
@@ -1004,9 +1004,9 @@ rtw89_pci_rxbd_increase(struct rtw89_pci_rx_ring *rx_ring, u32 cnt)
 
 static inline struct rtw89_pci_tx_data *RTW89_PCI_TX_SKB_CB(struct sk_buff *skb)
 {
-	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+	struct rtw89_tx_skb_data *data = RTW89_TX_SKB_CB(skb);
 
-	return (struct rtw89_pci_tx_data *)info->status.status_driver_data;
+	return (struct rtw89_pci_tx_data *)data->hci_priv;
 }
 
 static inline struct rtw89_pci_tx_bd_32 *
-- 
2.25.1

[PATCH 3/5] wifi: rtw89: add ieee80211::remain_on_channel ops

[Ping-Ke Shih]

From: Po-Hao Huang <phhuang@realtek.com>

Add support of remain on channel ops. Since channel context is
required to enable multi-channel concurrent(MCC) and the current
ROC in mac80211 don't support more than 1 channel context, add this
to let P2P and other protocols relying on this work as expected.
The off-channel duration and cancel timing is purely controlled by
upper layers.

Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/chan.c     |  35 +++
 drivers/net/wireless/realtek/rtw89/chan.h     |   3 +
 drivers/net/wireless/realtek/rtw89/core.c     | 212 +++++++++++++++++-
 drivers/net/wireless/realtek/rtw89/core.h     |  30 +++
 drivers/net/wireless/realtek/rtw89/mac.c      |   5 +-
 drivers/net/wireless/realtek/rtw89/mac80211.c |  80 ++++++-
 drivers/net/wireless/realtek/rtw89/ps.h       |  16 ++
 7 files changed, 378 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/chan.c b/drivers/net/wireless/realtek/rtw89/chan.c
index 90596806bc93f..4663db4ce2f66 100644
--- a/drivers/net/wireless/realtek/rtw89/chan.c
+++ b/drivers/net/wireless/realtek/rtw89/chan.c
@@ -141,6 +141,38 @@ void rtw89_config_entity_chandef(struct rtw89_dev *rtwdev,
 	__rtw89_config_entity_chandef(rtwdev, idx, chandef, true);
 }
 
+void rtw89_config_roc_chandef(struct rtw89_dev *rtwdev,
+			      enum rtw89_sub_entity_idx idx,
+			      const struct cfg80211_chan_def *chandef)
+{
+	struct rtw89_hal *hal = &rtwdev->hal;
+	enum rtw89_sub_entity_idx cur;
+
+	if (chandef) {
+		cur = atomic_cmpxchg(&hal->roc_entity_idx,
+				     RTW89_SUB_ENTITY_IDLE, idx);
+		if (cur != RTW89_SUB_ENTITY_IDLE) {
+			rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+				    "ROC still processing on entity %d\n", idx);
+			return;
+		}
+
+		hal->roc_chandef = *chandef;
+	} else {
+		cur = atomic_cmpxchg(&hal->roc_entity_idx, idx,
+				     RTW89_SUB_ENTITY_IDLE);
+		if (cur == idx)
+			return;
+
+		if (cur == RTW89_SUB_ENTITY_IDLE)
+			rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+				    "ROC already finished on entity %d\n", idx);
+		else
+			rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+				    "ROC is processing on entity %d\n", cur);
+	}
+}
+
 static void rtw89_config_default_chandef(struct rtw89_dev *rtwdev)
 {
 	struct cfg80211_chan_def chandef = {0};
@@ -154,6 +186,7 @@ void rtw89_entity_init(struct rtw89_dev *rtwdev)
 	struct rtw89_hal *hal = &rtwdev->hal;
 
 	bitmap_zero(hal->entity_map, NUM_OF_RTW89_SUB_ENTITY);
+	atomic_set(&hal->roc_entity_idx, RTW89_SUB_ENTITY_IDLE);
 	rtw89_config_default_chandef(rtwdev);
 }
 
@@ -229,6 +262,8 @@ void rtw89_chanctx_ops_remove(struct rtw89_dev *rtwdev,
 			rtwvif->sub_entity_idx = RTW89_SUB_ENTITY_0;
 	}
 
+	atomic_cmpxchg(&hal->roc_entity_idx, roll, RTW89_SUB_ENTITY_0);
+
 	drop = roll;
 
 out:
diff --git a/drivers/net/wireless/realtek/rtw89/chan.h b/drivers/net/wireless/realtek/rtw89/chan.h
index ecbd4503bead9..bdf369db50417 100644
--- a/drivers/net/wireless/realtek/rtw89/chan.h
+++ b/drivers/net/wireless/realtek/rtw89/chan.h
@@ -45,6 +45,9 @@ bool rtw89_assign_entity_chan(struct rtw89_dev *rtwdev,
 void rtw89_config_entity_chandef(struct rtw89_dev *rtwdev,
 				 enum rtw89_sub_entity_idx idx,
 				 const struct cfg80211_chan_def *chandef);
+void rtw89_config_roc_chandef(struct rtw89_dev *rtwdev,
+			      enum rtw89_sub_entity_idx idx,
+			      const struct cfg80211_chan_def *chandef);
 void rtw89_entity_init(struct rtw89_dev *rtwdev);
 enum rtw89_entity_mode rtw89_entity_recalc(struct rtw89_dev *rtwdev);
 int rtw89_chanctx_ops_add(struct rtw89_dev *rtwdev,
diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 369f7fb4e0779..594b5e5ea43ce 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -2014,6 +2014,18 @@ static void rtw89_core_free_sta_pending_forbid_ba(struct rtw89_dev *rtwdev,
 	spin_unlock_bh(&rtwdev->ba_lock);
 }
 
+static void rtw89_core_free_sta_pending_roc_tx(struct rtw89_dev *rtwdev,
+					       struct ieee80211_sta *sta)
+{
+	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+	struct sk_buff *skb, *tmp;
+
+	skb_queue_walk_safe(&rtwsta->roc_queue, skb, tmp) {
+		skb_unlink(skb, &rtwsta->roc_queue);
+		dev_kfree_skb_any(skb);
+	}
+}
+
 static void rtw89_core_stop_tx_ba_session(struct rtw89_dev *rtwdev,
 					  struct rtw89_txq *rtwtxq)
 {
@@ -2153,6 +2165,7 @@ static void rtw89_core_txq_schedule(struct rtw89_dev *rtwdev, u8 ac, bool *reinv
 {
 	struct ieee80211_hw *hw = rtwdev->hw;
 	struct ieee80211_txq *txq;
+	struct rtw89_vif *rtwvif;
 	struct rtw89_txq *rtwtxq;
 	unsigned long frame_cnt;
 	unsigned long byte_cnt;
@@ -2162,6 +2175,12 @@ static void rtw89_core_txq_schedule(struct rtw89_dev *rtwdev, u8 ac, bool *reinv
 	ieee80211_txq_schedule_start(hw, ac);
 	while ((txq = ieee80211_next_txq(hw, ac))) {
 		rtwtxq = (struct rtw89_txq *)txq->drv_priv;
+		rtwvif = (struct rtw89_vif *)txq->vif->drv_priv;
+
+		if (rtwvif->offchan) {
+			ieee80211_return_txq(hw, txq, true);
+			continue;
+		}
 		tx_resource = rtw89_check_and_reclaim_tx_resource(rtwdev, txq->tid);
 		sched_txq = false;
 
@@ -2230,6 +2249,187 @@ static void rtw89_forbid_ba_work(struct work_struct *w)
 	spin_unlock_bh(&rtwdev->ba_lock);
 }
 
+static void rtw89_core_sta_pending_tx_iter(void *data,
+					   struct ieee80211_sta *sta)
+{
+	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+	struct rtw89_vif *rtwvif_target = data, *rtwvif = rtwsta->rtwvif;
+	struct rtw89_dev *rtwdev = rtwvif->rtwdev;
+	struct ieee80211_vif *vif = rtwvif_to_vif(rtwvif);
+	struct sk_buff *skb, *tmp;
+	int qsel, ret;
+
+	if (rtwvif->sub_entity_idx != rtwvif_target->sub_entity_idx)
+		return;
+
+	if (skb_queue_len(&rtwsta->roc_queue) == 0)
+		return;
+
+	skb_queue_walk_safe(&rtwsta->roc_queue, skb, tmp) {
+		skb_unlink(skb, &rtwsta->roc_queue);
+
+		ret = rtw89_core_tx_write(rtwdev, vif, sta, skb, &qsel);
+		if (ret) {
+			rtw89_warn(rtwdev, "pending tx failed with %d\n", ret);
+			dev_kfree_skb_any(skb);
+		} else {
+			rtw89_core_tx_kick_off(rtwdev, qsel);
+		}
+	}
+}
+
+static void rtw89_core_handle_sta_pending_tx(struct rtw89_dev *rtwdev,
+					     struct rtw89_vif *rtwvif)
+{
+	ieee80211_iterate_stations_atomic(rtwdev->hw,
+					  rtw89_core_sta_pending_tx_iter,
+					  rtwvif);
+}
+
+static int rtw89_core_send_nullfunc(struct rtw89_dev *rtwdev,
+				    struct rtw89_vif *rtwvif, bool qos, bool ps)
+{
+	struct ieee80211_vif *vif = rtwvif_to_vif(rtwvif);
+	struct ieee80211_sta *sta;
+	struct ieee80211_hdr *hdr;
+	struct sk_buff *skb;
+	int ret, qsel;
+
+	if (vif->type != NL80211_IFTYPE_STATION || !vif->cfg.assoc)
+		return 0;
+
+	rcu_read_lock();
+	sta = ieee80211_find_sta(vif, vif->bss_conf.bssid);
+	if (!sta) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	skb = ieee80211_nullfunc_get(rtwdev->hw, vif, -1, qos);
+	if (!skb) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	hdr = (struct ieee80211_hdr *)skb->data;
+	if (ps)
+		hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
+
+	ret = rtw89_core_tx_write(rtwdev, vif, sta, skb, &qsel);
+	if (ret) {
+		rtw89_warn(rtwdev, "nullfunc transmit failed: %d\n", ret);
+		dev_kfree_skb_any(skb);
+		goto out;
+	}
+
+	rcu_read_unlock();
+
+	return rtw89_core_tx_kick_off_and_wait(rtwdev, skb, qsel,
+					       RTW89_ROC_TX_TIMEOUT);
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
+void rtw89_roc_start(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif)
+{
+	struct ieee80211_hw *hw = rtwdev->hw;
+	struct rtw89_roc *roc = &rtwvif->roc;
+	struct cfg80211_chan_def roc_chan;
+	struct rtw89_vif *tmp;
+	int ret;
+
+	lockdep_assert_held(&rtwdev->mutex);
+
+	ieee80211_queue_delayed_work(hw, &rtwvif->roc.roc_work,
+				     msecs_to_jiffies(rtwvif->roc.duration));
+
+	rtw89_leave_ips_by_hwflags(rtwdev);
+	rtw89_leave_lps(rtwdev);
+
+	ret = rtw89_core_send_nullfunc(rtwdev, rtwvif, true, true);
+	if (ret)
+		rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+			    "roc send null-1 failed: %d\n", ret);
+
+	rtw89_for_each_rtwvif(rtwdev, tmp)
+		if (tmp->sub_entity_idx == rtwvif->sub_entity_idx)
+			tmp->offchan = true;
+
+	cfg80211_chandef_create(&roc_chan, &roc->chan, NL80211_CHAN_NO_HT);
+	rtw89_config_roc_chandef(rtwdev, rtwvif->sub_entity_idx, &roc_chan);
+	rtw89_set_channel(rtwdev);
+	rtw89_write32_clr(rtwdev,
+			  rtw89_mac_reg_by_idx(R_AX_RX_FLTR_OPT, RTW89_MAC_0),
+			  B_AX_A_UC_CAM_MATCH | B_AX_A_BC_CAM_MATCH);
+
+	ieee80211_ready_on_channel(hw);
+}
+
+void rtw89_roc_end(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif)
+{
+	struct ieee80211_hw *hw = rtwdev->hw;
+	struct rtw89_roc *roc = &rtwvif->roc;
+	struct rtw89_vif *tmp;
+	int ret;
+
+	lockdep_assert_held(&rtwdev->mutex);
+
+	ieee80211_remain_on_channel_expired(hw);
+
+	rtw89_leave_ips_by_hwflags(rtwdev);
+	rtw89_leave_lps(rtwdev);
+
+	rtw89_write32_mask(rtwdev,
+			   rtw89_mac_reg_by_idx(R_AX_RX_FLTR_OPT, RTW89_MAC_0),
+			   B_AX_RX_FLTR_CFG_MASK,
+			   rtwdev->hal.rx_fltr);
+
+	roc->state = RTW89_ROC_IDLE;
+	rtw89_config_roc_chandef(rtwdev, rtwvif->sub_entity_idx, NULL);
+	rtw89_set_channel(rtwdev);
+	ret = rtw89_core_send_nullfunc(rtwdev, rtwvif, true, false);
+	if (ret)
+		rtw89_debug(rtwdev, RTW89_DBG_TXRX,
+			    "roc send null-0 failed: %d\n", ret);
+
+	rtw89_for_each_rtwvif(rtwdev, tmp)
+		if (tmp->sub_entity_idx == rtwvif->sub_entity_idx)
+			tmp->offchan = false;
+
+	rtw89_core_handle_sta_pending_tx(rtwdev, rtwvif);
+	queue_work(rtwdev->txq_wq, &rtwdev->txq_work);
+
+	if (hw->conf.flags & IEEE80211_CONF_IDLE)
+		ieee80211_queue_delayed_work(hw, &roc->roc_work,
+					     RTW89_ROC_IDLE_TIMEOUT);
+}
+
+void rtw89_roc_work(struct work_struct *work)
+{
+	struct rtw89_vif *rtwvif = container_of(work, struct rtw89_vif,
+						roc.roc_work.work);
+	struct rtw89_dev *rtwdev = rtwvif->rtwdev;
+	struct rtw89_roc *roc = &rtwvif->roc;
+
+	mutex_lock(&rtwdev->mutex);
+
+	switch (roc->state) {
+	case RTW89_ROC_IDLE:
+		rtw89_enter_ips_by_hwflags(rtwdev);
+		break;
+	case RTW89_ROC_MGMT:
+	case RTW89_ROC_NORMAL:
+		rtw89_roc_end(rtwdev, rtwvif);
+		break;
+	default:
+		break;
+	}
+
+	mutex_unlock(&rtwdev->mutex);
+}
+
 static enum rtw89_tfc_lv rtw89_get_traffic_level(struct rtw89_dev *rtwdev,
 						 u32 throughput, u64 cnt)
 {
@@ -2299,6 +2499,9 @@ static void rtw89_vif_enter_lps(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwv
 	    rtwvif->tdls_peer)
 		return;
 
+	if (rtwvif->offchan)
+		return;
+
 	if (rtwvif->stats.tx_tfc_lv == RTW89_TFC_IDLE &&
 	    rtwvif->stats.rx_tfc_lv == RTW89_TFC_IDLE)
 		rtw89_enter_lps(rtwdev, rtwvif);
@@ -2528,6 +2731,7 @@ int rtw89_core_sta_add(struct rtw89_dev *rtwdev,
 	rtwsta->rtwvif = rtwvif;
 	rtwsta->prev_rssi = 0;
 	INIT_LIST_HEAD(&rtwsta->ba_cam_list);
+	skb_queue_head_init(&rtwsta->roc_queue);
 
 	for (i = 0; i < ARRAY_SIZE(sta->txq); i++)
 		rtw89_core_txq_init(rtwdev, sta->txq[i]);
@@ -2597,6 +2801,8 @@ int rtw89_core_sta_disconnect(struct rtw89_dev *rtwdev,
 	rtw89_mac_bf_disassoc(rtwdev, vif, sta);
 	rtw89_core_free_sta_pending_ba(rtwdev, sta);
 	rtw89_core_free_sta_pending_forbid_ba(rtwdev, sta);
+	rtw89_core_free_sta_pending_roc_tx(rtwdev, sta);
+
 	if (vif->type == NL80211_IFTYPE_AP || sta->tdls)
 		rtw89_cam_deinit_addr_cam(rtwdev, &rtwsta->addr_cam);
 	if (sta->tdls)
@@ -3482,6 +3688,7 @@ static int rtw89_core_register_hw(struct rtw89_dev *rtwdev)
 	hw->wiphy->tid_config_support.peer |= BIT(NL80211_TID_CONFIG_ATTR_AMPDU_CTRL);
 	hw->wiphy->tid_config_support.vif |= BIT(NL80211_TID_CONFIG_ATTR_AMSDU_CTRL);
 	hw->wiphy->tid_config_support.peer |= BIT(NL80211_TID_CONFIG_ATTR_AMSDU_CTRL);
+	hw->wiphy->max_remain_on_channel_duration = 1000;
 
 	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CAN_REPLACE_PTK0);
 
@@ -3561,7 +3768,8 @@ struct rtw89_dev *rtw89_alloc_ieee80211_hw(struct device *device,
 		goto err;
 
 	no_chanctx = chip->support_chanctx_num == 0 ||
-		     !(early_feat_map & BIT(RTW89_FW_FEATURE_SCAN_OFFLOAD));
+		     !(early_feat_map & BIT(RTW89_FW_FEATURE_SCAN_OFFLOAD)) ||
+		     !(early_feat_map & BIT(RTW89_FW_FEATURE_BEACON_FILTER));
 
 	if (no_chanctx) {
 		ops->add_chanctx = NULL;
@@ -3569,6 +3777,8 @@ struct rtw89_dev *rtw89_alloc_ieee80211_hw(struct device *device,
 		ops->change_chanctx = NULL;
 		ops->assign_vif_chanctx = NULL;
 		ops->unassign_vif_chanctx = NULL;
+		ops->remain_on_channel = NULL;
+		ops->cancel_remain_on_channel = NULL;
 	}
 
 	driver_data_size = sizeof(struct rtw89_dev) + bus_data_size;
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index dd4165394c8de..f9093dc9e2b92 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -569,6 +569,7 @@ enum rtw89_sub_entity_idx {
 	RTW89_SUB_ENTITY_0 = 0,
 
 	NUM_OF_RTW89_SUB_ENTITY,
+	RTW89_SUB_ENTITY_IDLE = NUM_OF_RTW89_SUB_ENTITY,
 };
 
 enum rtw89_rf_path {
@@ -2395,6 +2396,7 @@ struct rtw89_sta {
 	struct rtw89_addr_cam_entry addr_cam; /* AP mode or TDLS peer only */
 	struct rtw89_bssid_cam_entry bssid_cam; /* TDLS peer only */
 	struct list_head ba_cam_list;
+	struct sk_buff_head roc_queue;
 
 	bool use_cfg_mask;
 	struct cfg80211_bitrate_mask mask;
@@ -2434,11 +2436,28 @@ struct rtw89_tx_skb_data {
 	u8 hci_priv[];
 };
 
+#define RTW89_ROC_IDLE_TIMEOUT 500
+#define RTW89_ROC_TX_TIMEOUT 30
+enum rtw89_roc_state {
+	RTW89_ROC_IDLE,
+	RTW89_ROC_NORMAL,
+	RTW89_ROC_MGMT,
+};
+
+struct rtw89_roc {
+	struct ieee80211_channel chan;
+	struct delayed_work roc_work;
+	enum ieee80211_roc_type type;
+	enum rtw89_roc_state state;
+	int duration;
+};
+
 #define RTW89_P2P_MAX_NOA_NUM 2
 
 struct rtw89_vif {
 	struct list_head list;
 	struct rtw89_dev *rtwdev;
+	struct rtw89_roc roc;
 	enum rtw89_sub_entity_idx sub_entity_idx;
 
 	u8 mac_id;
@@ -2454,6 +2473,7 @@ struct rtw89_vif {
 	u8 bcn_hit_cond;
 	u8 hit_rule;
 	u8 last_noa_nr;
+	bool offchan;
 	bool trigger;
 	bool lsig_txop;
 	u8 tgt_ind;
@@ -3165,9 +3185,11 @@ struct rtw89_hal {
 	bool tx_path_diversity;
 	bool support_cckpd;
 	bool support_igi;
+	atomic_t roc_entity_idx;
 
 	DECLARE_BITMAP(entity_map, NUM_OF_RTW89_SUB_ENTITY);
 	struct rtw89_sub_entity sub[NUM_OF_RTW89_SUB_ENTITY];
+	struct cfg80211_chan_def roc_chandef;
 
 	bool entity_active;
 	enum rtw89_entity_mode entity_mode;
@@ -3828,6 +3850,7 @@ struct rtw89_dev {
 	struct delayed_work coex_rfk_chk_work;
 	struct delayed_work cfo_track_work;
 	struct delayed_work forbid_ba_work;
+	struct delayed_work roc_work;
 	struct rtw89_ppdu_sts_info ppdu_sts;
 	u8 total_sta_assoc;
 	bool scanning;
@@ -4343,6 +4366,10 @@ const struct cfg80211_chan_def *rtw89_chandef_get(struct rtw89_dev *rtwdev,
 						  enum rtw89_sub_entity_idx idx)
 {
 	struct rtw89_hal *hal = &rtwdev->hal;
+	enum rtw89_sub_entity_idx roc_idx = atomic_read(&hal->roc_entity_idx);
+
+	if (roc_idx == idx)
+		return &hal->roc_chandef;
 
 	return &hal->sub[idx].chandef;
 }
@@ -4719,6 +4746,9 @@ void rtw89_complete_cond(struct rtw89_wait_info *wait, unsigned int cond,
 int rtw89_core_start(struct rtw89_dev *rtwdev);
 void rtw89_core_stop(struct rtw89_dev *rtwdev);
 void rtw89_core_update_beacon_work(struct work_struct *work);
+void rtw89_roc_work(struct work_struct *work);
+void rtw89_roc_start(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif);
+void rtw89_roc_end(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif);
 void rtw89_core_scan_start(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif,
 			   const u8 *mac_addr, bool hw_scan);
 void rtw89_core_scan_complete(struct rtw89_dev *rtwdev,
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c
index 0e1471c34be00..8cceda1cf3994 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.c
+++ b/drivers/net/wireless/realtek/rtw89/mac.c
@@ -4191,6 +4191,9 @@ rtw89_mac_c2h_scanofld_rsp(struct rtw89_dev *rtwdev, struct sk_buff *c2h,
 	u16 chan;
 	int ret;
 
+	if (!rtwvif)
+		return;
+
 	tx_fail = RTW89_GET_MAC_C2H_SCANOFLD_TX_FAIL(c2h->data);
 	status = RTW89_GET_MAC_C2H_SCANOFLD_STATUS(c2h->data);
 	chan = RTW89_GET_MAC_C2H_SCANOFLD_PRI_CH(c2h->data);
@@ -4258,7 +4261,7 @@ rtw89_mac_bcn_fltr_rpt(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif,
 
 	switch (type) {
 	case RTW89_BCN_FLTR_BEACON_LOSS:
-		if (!rtwdev->scanning)
+		if (!rtwdev->scanning && !rtwvif->offchan)
 			ieee80211_connection_loss(vif);
 		else
 			rtw89_fw_h2c_set_bcn_fltr_cfg(rtwdev, vif, true);
diff --git a/drivers/net/wireless/realtek/rtw89/mac80211.c b/drivers/net/wireless/realtek/rtw89/mac80211.c
index 629b32dafecb8..b059aa8d88dbf 100644
--- a/drivers/net/wireless/realtek/rtw89/mac80211.c
+++ b/drivers/net/wireless/realtek/rtw89/mac80211.c
@@ -23,9 +23,19 @@ static void rtw89_ops_tx(struct ieee80211_hw *hw,
 	struct rtw89_dev *rtwdev = hw->priv;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_vif *vif = info->control.vif;
+	struct rtw89_vif *rtwvif = (struct rtw89_vif *)vif->drv_priv;
 	struct ieee80211_sta *sta = control->sta;
+	u32 flags = IEEE80211_SKB_CB(skb)->flags;
 	int ret, qsel;
 
+	if (rtwvif->offchan && !(flags & IEEE80211_TX_CTL_TX_OFFCHAN) && sta) {
+		struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+
+		rtw89_debug(rtwdev, RTW89_DBG_TXRX, "ops_tx during offchan\n");
+		skb_queue_tail(&rtwsta->roc_queue, skb);
+		return;
+	}
+
 	ret = rtw89_core_tx_write(rtwdev, vif, sta, skb, &qsel);
 	if (ret) {
 		rtw89_err(rtwdev, "failed to transmit skb: %d\n", ret);
@@ -115,13 +125,18 @@ static int rtw89_ops_add_interface(struct ieee80211_hw *hw,
 
 	mutex_lock(&rtwdev->mutex);
 
+	rtw89_leave_ips_by_hwflags(rtwdev);
+
 	if (RTW89_CHK_FW_FEATURE(BEACON_FILTER, &rtwdev->fw))
 		vif->driver_flags |= IEEE80211_VIF_BEACON_FILTER |
 				     IEEE80211_VIF_SUPPORTS_CQM_RSSI;
 
 	rtwvif->rtwdev = rtwdev;
+	rtwvif->roc.state = RTW89_ROC_IDLE;
+	rtwvif->offchan = false;
 	list_add_tail(&rtwvif->list, &rtwdev->rtwvifs_list);
 	INIT_WORK(&rtwvif->update_beacon_work, rtw89_core_update_beacon_work);
+	INIT_DELAYED_WORK(&rtwvif->roc.roc_work, rtw89_roc_work);
 	rtw89_leave_ps_mode(rtwdev);
 
 	rtw89_traffic_stats_init(rtwdev, &rtwvif->stats);
@@ -168,6 +183,7 @@ static void rtw89_ops_remove_interface(struct ieee80211_hw *hw,
 		    vif->addr, vif->type, vif->p2p);
 
 	cancel_work_sync(&rtwvif->update_beacon_work);
+	cancel_delayed_work_sync(&rtwvif->roc.roc_work);
 
 	mutex_lock(&rtwdev->mutex);
 	rtw89_leave_ps_mode(rtwdev);
@@ -175,6 +191,8 @@ static void rtw89_ops_remove_interface(struct ieee80211_hw *hw,
 	rtw89_mac_remove_vif(rtwdev, rtwvif);
 	rtw89_core_release_bit_map(rtwdev->hw_port, rtwvif->port);
 	list_del_init(&rtwvif->list);
+	rtw89_enter_ips_by_hwflags(rtwdev);
+
 	mutex_unlock(&rtwdev->mutex);
 }
 
@@ -803,12 +821,13 @@ static int rtw89_ops_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			     struct ieee80211_scan_request *req)
 {
 	struct rtw89_dev *rtwdev = hw->priv;
+	struct rtw89_vif *rtwvif = vif_to_rtwvif_safe(vif);
 	int ret = 0;
 
 	if (!RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw))
 		return 1;
 
-	if (rtwdev->scanning)
+	if (rtwdev->scanning || rtwvif->offchan)
 		return -EBUSY;
 
 	mutex_lock(&rtwdev->mutex);
@@ -911,6 +930,63 @@ static void rtw89_ops_unassign_vif_chanctx(struct ieee80211_hw *hw,
 	mutex_unlock(&rtwdev->mutex);
 }
 
+static int rtw89_ops_remain_on_channel(struct ieee80211_hw *hw,
+				       struct ieee80211_vif *vif,
+				       struct ieee80211_channel *chan,
+				       int duration,
+				       enum ieee80211_roc_type type)
+{
+	struct rtw89_dev *rtwdev = hw->priv;
+	struct rtw89_vif *rtwvif = vif_to_rtwvif_safe(vif);
+	struct rtw89_roc *roc = &rtwvif->roc;
+
+	if (!vif)
+		return -EINVAL;
+
+	mutex_lock(&rtwdev->mutex);
+
+	if (roc->state != RTW89_ROC_IDLE) {
+		mutex_unlock(&rtwdev->mutex);
+		return -EBUSY;
+	}
+
+	if (rtwdev->scanning)
+		rtw89_hw_scan_abort(rtwdev, vif);
+
+	if (type == IEEE80211_ROC_TYPE_MGMT_TX)
+		roc->state = RTW89_ROC_MGMT;
+	else
+		roc->state = RTW89_ROC_NORMAL;
+
+	roc->duration = duration;
+	roc->chan = *chan;
+	roc->type = type;
+
+	rtw89_roc_start(rtwdev, rtwvif);
+
+	mutex_unlock(&rtwdev->mutex);
+
+	return 0;
+}
+
+static int rtw89_ops_cancel_remain_on_channel(struct ieee80211_hw *hw,
+					      struct ieee80211_vif *vif)
+{
+	struct rtw89_dev *rtwdev = hw->priv;
+	struct rtw89_vif *rtwvif = vif_to_rtwvif_safe(vif);
+
+	if (!rtwvif)
+		return -EINVAL;
+
+	cancel_delayed_work_sync(&rtwvif->roc.roc_work);
+
+	mutex_lock(&rtwdev->mutex);
+	rtw89_roc_end(rtwdev, rtwvif);
+	mutex_unlock(&rtwdev->mutex);
+
+	return 0;
+}
+
 static void rtw89_set_tid_config_iter(void *data, struct ieee80211_sta *sta)
 {
 	struct cfg80211_tid_config *tid_config = data;
@@ -1022,6 +1098,8 @@ const struct ieee80211_ops rtw89_ops = {
 	.change_chanctx		= rtw89_ops_change_chanctx,
 	.assign_vif_chanctx	= rtw89_ops_assign_vif_chanctx,
 	.unassign_vif_chanctx	= rtw89_ops_unassign_vif_chanctx,
+	.remain_on_channel		= rtw89_ops_remain_on_channel,
+	.cancel_remain_on_channel	= rtw89_ops_cancel_remain_on_channel,
 	.set_sar_specs		= rtw89_ops_set_sar_specs,
 	.sta_rc_update		= rtw89_ops_sta_rc_update,
 	.set_tid_config		= rtw89_ops_set_tid_config,
diff --git a/drivers/net/wireless/realtek/rtw89/ps.h b/drivers/net/wireless/realtek/rtw89/ps.h
index 6ac1f7ea53394..c9e29d92fc1b9 100644
--- a/drivers/net/wireless/realtek/rtw89/ps.h
+++ b/drivers/net/wireless/realtek/rtw89/ps.h
@@ -15,4 +15,20 @@ void rtw89_leave_ips(struct rtw89_dev *rtwdev);
 void rtw89_set_coex_ctrl_lps(struct rtw89_dev *rtwdev, bool btc_ctrl);
 void rtw89_process_p2p_ps(struct rtw89_dev *rtwdev, struct ieee80211_vif *vif);
 
+static inline void rtw89_leave_ips_by_hwflags(struct rtw89_dev *rtwdev)
+{
+	struct ieee80211_hw *hw = rtwdev->hw;
+
+	if (hw->conf.flags & IEEE80211_CONF_IDLE)
+		rtw89_leave_ips(rtwdev);
+}
+
+static inline void rtw89_enter_ips_by_hwflags(struct rtw89_dev *rtwdev)
+{
+	struct ieee80211_hw *hw = rtwdev->hw;
+
+	if (hw->conf.flags & IEEE80211_CONF_IDLE)
+		rtw89_enter_ips(rtwdev);
+}
+
 #endif
-- 
2.25.1

[PATCH 4/5] wifi: rtw89: add flag check for power state

[Ping-Ke Shih]

From: Po-Hao Huang <phhuang@realtek.com>

Use POWER_ON flag to make sure power on/off is symmetric. Since both
remain_on_channel and hw_scan both alter the power state, this makes
sure that we don't enter/leave IPS mode twice.
Also, replace IPS related functions with inline function that does
similar logic so we can track it more easily.

Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/core.c | 7 +++----
 drivers/net/wireless/realtek/rtw89/ps.c   | 6 ++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 594b5e5ea43ce..7f8f0e551eb04 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -2207,8 +2207,7 @@ static void rtw89_ips_work(struct work_struct *work)
 	struct rtw89_dev *rtwdev = container_of(work, struct rtw89_dev,
 						ips_work);
 	mutex_lock(&rtwdev->mutex);
-	if (rtwdev->hw->conf.flags & IEEE80211_CONF_IDLE)
-		rtw89_enter_ips(rtwdev);
+	rtw89_enter_ips_by_hwflags(rtwdev);
 	mutex_unlock(&rtwdev->mutex);
 }
 
@@ -3505,8 +3504,8 @@ void rtw89_core_scan_start(struct rtw89_dev *rtwdev, struct rtw89_vif *rtwvif,
 
 	rtwdev->scanning = true;
 	rtw89_leave_lps(rtwdev);
-	if (hw_scan && (rtwdev->hw->conf.flags & IEEE80211_CONF_IDLE))
-		rtw89_leave_ips(rtwdev);
+	if (hw_scan)
+		rtw89_leave_ips_by_hwflags(rtwdev);
 
 	ether_addr_copy(rtwvif->mac_addr, mac_addr);
 	rtw89_btc_ntfy_scan_start(rtwdev, RTW89_PHY_0, chan->band_type);
diff --git a/drivers/net/wireless/realtek/rtw89/ps.c b/drivers/net/wireless/realtek/rtw89/ps.c
index 40498812205ea..cf72861c7adcd 100644
--- a/drivers/net/wireless/realtek/rtw89/ps.c
+++ b/drivers/net/wireless/realtek/rtw89/ps.c
@@ -155,6 +155,9 @@ void rtw89_enter_ips(struct rtw89_dev *rtwdev)
 
 	set_bit(RTW89_FLAG_INACTIVE_PS, rtwdev->flags);
 
+	if (!test_bit(RTW89_FLAG_POWERON, rtwdev->flags))
+		return;
+
 	rtw89_for_each_rtwvif(rtwdev, rtwvif)
 		rtw89_mac_vif_deinit(rtwdev, rtwvif);
 
@@ -166,6 +169,9 @@ void rtw89_leave_ips(struct rtw89_dev *rtwdev)
 	struct rtw89_vif *rtwvif;
 	int ret;
 
+	if (test_bit(RTW89_FLAG_POWERON, rtwdev->flags))
+		return;
+
 	ret = rtw89_core_start(rtwdev);
 	if (ret)
 		rtw89_err(rtwdev, "failed to leave idle state\n");
-- 
2.25.1

[PATCH 5/5] wifi: rtw89: fix authentication fail during scan

[Ping-Ke Shih]

From: Po-Hao Huang <phhuang@realtek.com>

We used to store operating channel info after associated. However, scan
might happen before that. Without switching back to operating channel,
authentication or association might fail. Therefore, we switch back to
operating channel when the scanning vif's BSSID is non-zero, which
implies connected or during attempt to connect.

Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/fw.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
index 2088b56263dfa..502adc70dc5bc 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.c
+++ b/drivers/net/wireless/realtek/rtw89/fw.c
@@ -3041,7 +3041,7 @@ static void rtw89_hw_scan_add_chan(struct rtw89_dev *rtwdev, int chan_type,
 }
 
 static int rtw89_hw_scan_add_chan_list(struct rtw89_dev *rtwdev,
-				       struct rtw89_vif *rtwvif)
+				       struct rtw89_vif *rtwvif, bool connected)
 {
 	struct cfg80211_scan_request *req = rtwvif->scan_req;
 	struct rtw89_mac_chinfo	*ch_info, *tmp;
@@ -3085,7 +3085,7 @@ static int rtw89_hw_scan_add_chan_list(struct rtw89_dev *rtwdev,
 			type = RTW89_CHAN_ACTIVE;
 		rtw89_hw_scan_add_chan(rtwdev, type, req->n_ssids, ch_info);
 
-		if (rtwvif->net_type != RTW89_NET_TYPE_NO_LINK &&
+		if (connected &&
 		    off_chan_time + ch_info->period > RTW89_OFF_CHAN_TIME) {
 			tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
 			if (!tmp) {
@@ -3118,7 +3118,7 @@ static int rtw89_hw_scan_add_chan_list(struct rtw89_dev *rtwdev,
 }
 
 static int rtw89_hw_scan_prehandle(struct rtw89_dev *rtwdev,
-				   struct rtw89_vif *rtwvif)
+				   struct rtw89_vif *rtwvif, bool connected)
 {
 	int ret;
 
@@ -3127,7 +3127,7 @@ static int rtw89_hw_scan_prehandle(struct rtw89_dev *rtwdev,
 		rtw89_err(rtwdev, "Update probe request failed\n");
 		goto out;
 	}
-	ret = rtw89_hw_scan_add_chan_list(rtwdev, rtwvif);
+	ret = rtw89_hw_scan_add_chan_list(rtwdev, rtwvif, connected);
 out:
 	return ret;
 }
@@ -3205,16 +3205,19 @@ int rtw89_hw_scan_offload(struct rtw89_dev *rtwdev, struct ieee80211_vif *vif,
 {
 	struct rtw89_scan_option opt = {0};
 	struct rtw89_vif *rtwvif;
+	bool connected;
 	int ret = 0;
 
 	rtwvif = vif ? (struct rtw89_vif *)vif->drv_priv : NULL;
 	if (!rtwvif)
 		return -EINVAL;
 
+	/* This variable implies connected or during attempt to connect */
+	connected = !is_zero_ether_addr(rtwvif->bssid);
 	opt.enable = enable;
-	opt.target_ch_mode = rtwvif->net_type != RTW89_NET_TYPE_NO_LINK;
+	opt.target_ch_mode = connected;
 	if (enable) {
-		ret = rtw89_hw_scan_prehandle(rtwdev, rtwvif);
+		ret = rtw89_hw_scan_prehandle(rtwdev, rtwvif, connected);
 		if (ret)
 			goto out;
 	}
-- 
2.25.1

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

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> writes:

> From: Po-Hao Huang <phhuang@realtek.com>
>
> Allocate a per-skb completion to track those skbs we are interested in
> and wait for them to complete transmission with TX status. To avoid
> race condition between process and softirq without addtional locking,
> we use a work to free the tx_wait struct later when waiter is finished
> referencing it. This must be called in process context and with a
> timeout value greater than zero since it might sleep.
>
> We use another workqueue so works can be processed concurrently and
> when the PCI device is removed unexpectedly, all pending works can be
> flushed. This prevents some works that were scheduled but never processed
> leads to memory leak.
>
> Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> +static void rtw89_core_free_tx_wait_work(struct work_struct *work)
> +{
> +	struct rtw89_tx_wait_info *wait =
> +			container_of(work, struct rtw89_tx_wait_info, work);
> +	struct rtw89_dev *rtwdev = wait->rtwdev;
> +	int done, ret;
> +
> +	ret = read_poll_timeout(atomic_read, done, done, 1000, 100000, false,
> +				&wait->wait_done);
> +
> +	if (ret)
> +		rtw89_err(rtwdev, "tx wait timed out, stop polling\n");
> +	else
> +		kfree(wait);
> +}

I admit I didn't try to understand this patch in detail but this
function just looks odd to me. Why there's polling able to free
something?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Ping-Ke Shih]

> -----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

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

[Ping-Ke Shih]

> -----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

RE: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Ping-Ke Shih]

> -----Original Message-----
> From: Kalle Valo <kvalo@kernel.org>
> Sent: Wednesday, March 15, 2023 4:40 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Bernie Huang <phhuang@realtek.com>; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs
> 
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Po-Hao Huang <phhuang@realtek.com>
> >
> > Allocate a per-skb completion to track those skbs we are interested in
> > and wait for them to complete transmission with TX status. To avoid
> > race condition between process and softirq without addtional locking,
> > we use a work to free the tx_wait struct later when waiter is finished
> > referencing it. This must be called in process context and with a
> > timeout value greater than zero since it might sleep.
> >
> > We use another workqueue so works can be processed concurrently and
> > when the PCI device is removed unexpectedly, all pending works can be
> > flushed. This prevents some works that were scheduled but never processed
> > leads to memory leak.
> >
> > Signed-off-by: Po-Hao Huang <phhuang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> 
> [...]
> 
> > +static void rtw89_core_free_tx_wait_work(struct work_struct *work)
> > +{
> > +     struct rtw89_tx_wait_info *wait =
> > +                     container_of(work, struct rtw89_tx_wait_info, work);
> > +     struct rtw89_dev *rtwdev = wait->rtwdev;
> > +     int done, ret;
> > +
> > +     ret = read_poll_timeout(atomic_read, done, done, 1000, 100000, false,
> > +                             &wait->wait_done);
> > +
> > +     if (ret)
> > +             rtw89_err(rtwdev, "tx wait timed out, stop polling\n");
> > +     else
> > +             kfree(wait);
> > +}
> 
> I admit I didn't try to understand this patch in detail but this
> function just looks odd to me. Why there's polling able to free
> something?
> 

Three works are involved in the "wait/completion".

work 1. remain-on-channel work
   It trigger TX null data and wait (kmalloc 'wait' object, and wait for completion)

work 2. TX completion by napi_poll
   It returns TX status (failed or succeed to TX), and complete the 'wait' object,
   and queue rtw89_core_free_tx_wait_work() to free 'wait' object.

   We queue this by work 2, because time of work 1 is predictable, but
   it is hard to estimate time of work 2. The read_poll_timeout() is for
   the work 1 predictable time.

work 3. This work is to do garbage collection of 'wait' object
   It polls if work 1 is done before doing free 'wait' object.


Things are complex because work 1 and 2 are done asynchronously, so one
of them can't free 'wait' object, or it will causes use-after-free in other
side.

Use case 1: (work 1 could free 'wait' object)
    work 1         work 2          work 3
    wait
                   completion
    wait ok
                                   free 'wait'

Use case 2: (work 2 could free 'wait' object)
    work 1         work 2          work 3
    wait
    wait timeout
                   completion
                                   free 'wait'


I can add a comment as a hint why we can use a read_poll_timeout() to assist
in freeing something. 

Ping-Ke

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

[Ping-Ke Shih]

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

rtw88/rtw89: command/event structure handling

[Kalle Valo]

(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/

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> writes:

>> > +static void rtw89_core_free_tx_wait_work(struct work_struct *work)
>> > +{
>> > +     struct rtw89_tx_wait_info *wait =
>> > +                     container_of(work, struct rtw89_tx_wait_info, work);
>> > +     struct rtw89_dev *rtwdev = wait->rtwdev;
>> > +     int done, ret;
>> > +
>> > +     ret = read_poll_timeout(atomic_read, done, done, 1000, 100000, false,
>> > +                             &wait->wait_done);
>> > +
>> > +     if (ret)
>> > +             rtw89_err(rtwdev, "tx wait timed out, stop polling\n");
>> > +     else
>> > +             kfree(wait);
>> > +}
>> 
>> I admit I didn't try to understand this patch in detail but this
>> function just looks odd to me. Why there's polling able to free
>> something?
>> 
>
> Three works are involved in the "wait/completion".
>
> work 1. remain-on-channel work
>    It trigger TX null data and wait (kmalloc 'wait' object, and wait for completion)
>
> work 2. TX completion by napi_poll
>    It returns TX status (failed or succeed to TX), and complete the 'wait' object,
>    and queue rtw89_core_free_tx_wait_work() to free 'wait' object.
>
>    We queue this by work 2, because time of work 1 is predictable, but
>    it is hard to estimate time of work 2. The read_poll_timeout() is for
>    the work 1 predictable time.
>
> work 3. This work is to do garbage collection of 'wait' object
>    It polls if work 1 is done before doing free 'wait' object.
>
>
> Things are complex because work 1 and 2 are done asynchronously, so one
> of them can't free 'wait' object, or it will causes use-after-free in other
> side.
>
> Use case 1: (work 1 could free 'wait' object)
>     work 1         work 2          work 3
>     wait
>                    completion
>     wait ok
>                                    free 'wait'
>
> Use case 2: (work 2 could free 'wait' object)
>     work 1         work 2          work 3
>     wait
>     wait timeout
>                    completion
>                                    free 'wait'
>
>
> I can add a comment as a hint why we can use a read_poll_timeout() to assist
> in freeing something. 

I would expect that there's polling if you are waiting something from
hardware, or maybe when implementing a spin lock, but not when waiting
for another kernel thread. This just doesn't feel right but I don't have
time to propose a good alternative either, sorry.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rtw88/rtw89: command/event structure handling

[Kalle Valo]

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!

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rtw88/rtw89: command/event structure handling

[Ping-Ke Shih]

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

Re: rtw88/rtw89: command/event structure handling

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Ping-Ke Shih]

On Mon, 2023-04-03 at 13:32 +0300, Kalle Valo wrote:
> 
> I would expect that there's polling if you are waiting something from
> hardware, or maybe when implementing a spin lock, but not when waiting
> for another kernel thread. This just doesn't feel right but I don't have
> time to propose a good alternative either, sorry.
> 

I have found a solution that uses an owner variable with a spin lock
to determine which side to free completion object. Simply show two use
cases below:

Use case 1: (normal case; free completion object by work 1)
    work 1                                work 2
    wait
                                          (spin_lock)
                                          complete
                                          check & set owner --> owner = work1
		                          (spin_unlock)
    wait ok
    (spin_lock)
    check & check owner --> free by work 1
    (spin_unlock)
    free completion


Use case 2: (timeout case; free completion object by work 2)
    work 1                                work 2
    wait
    wait timeout
    (spin_lock)
    check & set owner --> owner = work 2
    (spin_unlock)
                                         (spin_lock)
                                         completion
                                         check & set owner --> free by work 2
                                         (spin_unlock)
                                         free completion

I will apply this by v5.

Ping-Ke

Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Ping-Ke Shih]

On Tue, 2023-04-04 at 10:37 +0800, Ping-Ke Shih wrote:
> On Mon, 2023-04-03 at 13:32 +0300, Kalle Valo wrote:
> > I would expect that there's polling if you are waiting something from
> > hardware, or maybe when implementing a spin lock, but not when waiting
> > for another kernel thread. This just doesn't feel right but I don't have
> > time to propose a good alternative either, sorry.
> > 
> 
> I have found a solution that uses an owner variable with a spin lock
> to determine which side to free completion object. Simply show two use
> cases below:
> 
> Use case 1: (normal case; free completion object by work 1)
>     work 1                                work 2
>     wait
>                                           (spin_lock)
>                                           complete
>                                           check & set owner --> owner = work1
> 		                          (spin_unlock)
>     wait ok
>     (spin_lock)
>     check & check owner --> free by work 1
>     (spin_unlock)
>     free completion
> 
> 
> Use case 2: (timeout case; free completion object by work 2)
>     work 1                                work 2
>     wait
>     wait timeout
>     (spin_lock)
>     check & set owner --> owner = work 2
>     (spin_unlock)
>                                          (spin_lock)
>                                          completion
>                                          check & set owner --> free by work 2
>                                          (spin_unlock)
>                                          free completion
> 
> I will apply this by v5.
> 

We have a better idea that use kfree_rcu() to free completion, so no
need spin_lock(). Then, the use cases become below, and I have sent
this change by v6.

Use case 1: (normal case)
    work 1                                work 2
    (rcu_assign_pointer(wait))
    wait
                                          (rcu_read_lock)
                                          wait = rcu_dereference();
                                          if (wait)    --> wait != NULL
                                              complete
                                          (rcu_read_unlock)
    wait ok
    (rcu_assign_pointer(NULL))
    kfree_rcu(completion)


Use case 2: (timeout case)
    work 1                                work 2
    (rcu_assign_pointer(wait))
    wait
    wait timeout
    (rcu_assign_pointer(NULL))
    kfree_rcu(completion)
                                          (rcu_read_lock)
                                          wait = rcu_dereference();
                                          if (wait)    --> wait == NULL
                                              complete
                                          (rcu_read_unlock)


Ping-Ke

Re: [PATCH 2/5] wifi: rtw89: add function to wait for completion of TX skbs

[Kalle Valo]

Ping-Ke Shih <pkshih@realtek.com> writes:

> On Tue, 2023-04-04 at 10:37 +0800, Ping-Ke Shih wrote:
>
>> On Mon, 2023-04-03 at 13:32 +0300, Kalle Valo wrote:
>> > I would expect that there's polling if you are waiting something from
>> > hardware, or maybe when implementing a spin lock, but not when waiting
>> > for another kernel thread. This just doesn't feel right but I don't have
>> > time to propose a good alternative either, sorry.
>> > 
>> 
>> I have found a solution that uses an owner variable with a spin lock
>> to determine which side to free completion object. Simply show two use
>> cases below:
>> 
>> Use case 1: (normal case; free completion object by work 1)
>>     work 1                                work 2
>>     wait
>>                                           (spin_lock)
>>                                           complete
>>                                           check & set owner --> owner = work1
>> 		                          (spin_unlock)
>>     wait ok
>>     (spin_lock)
>>     check & check owner --> free by work 1
>>     (spin_unlock)
>>     free completion
>> 
>> 
>> Use case 2: (timeout case; free completion object by work 2)
>>     work 1                                work 2
>>     wait
>>     wait timeout
>>     (spin_lock)
>>     check & set owner --> owner = work 2
>>     (spin_unlock)
>>                                          (spin_lock)
>>                                          completion
>>                                          check & set owner --> free by work 2
>>                                          (spin_unlock)
>>                                          free completion
>> 
>> I will apply this by v5.
>> 
>
> We have a better idea that use kfree_rcu() to free completion, so no
> need spin_lock(). Then, the use cases become below, and I have sent
> this change by v6.
>
> Use case 1: (normal case)
>     work 1                                work 2
>     (rcu_assign_pointer(wait))
>     wait
>                                           (rcu_read_lock)
>                                           wait = rcu_dereference();
>                                           if (wait)    --> wait != NULL
>                                               complete
>                                           (rcu_read_unlock)
>     wait ok
>     (rcu_assign_pointer(NULL))
>     kfree_rcu(completion)
>
>
> Use case 2: (timeout case)
>     work 1                                work 2
>     (rcu_assign_pointer(wait))
>     wait
>     wait timeout
>     (rcu_assign_pointer(NULL))
>     kfree_rcu(completion)
>                                           (rcu_read_lock)
>                                           wait = rcu_dereference();
>                                           if (wait)    --> wait == NULL
>                                               complete
>                                           (rcu_read_unlock)

Thanks, I haven't had a chance to look at v6 yet (nor v5 for that
matter) but at least based on this mail your idea looks much better than
polling.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches