[PATCH 0/2] wifi: ath12k: Add support for MLO Multicast Handling

[PATCH 0/2] wifi: ath12k: Add support for MLO Multicast Handling

[Balamurugan Mahalingam]

Patch 1 updates the HTT TCL metadata version to use the new bit definitions
	of the field to pass additional information from Host to Firmware.

Patch 2 moves the MLO Multicast frame duplication logic to the driver to
	have more flexibility to update the HTT TCL metadata with the global
	sequence and with a special vdev id to indicate Multicast handling

Note: ath12k-check verified and it lists one exception for length.
      Exception has been fixed separately.

Balamurugan Mahalingam (2):
  wifi: ath12k: Update HTT_TCL_METADATA version and bit mask definitions
  wifi: ath12k: Add support for MLO Multicast handling in driver

 drivers/net/wireless/ath/ath12k/core.h  |  3 +-
 drivers/net/wireless/ath/ath12k/dp.h    | 27 +++++--
 drivers/net/wireless/ath/ath12k/dp_tx.c | 27 +++++--
 drivers/net/wireless/ath/ath12k/dp_tx.h |  4 +-
 drivers/net/wireless/ath/ath12k/mac.c   | 94 +++++++++++++++++++++++--
 5 files changed, 135 insertions(+), 20 deletions(-)


base-commit: dbe50a7420e22954c747e79e72df2750d795ea77
-- 
2.34.1

[PATCH 1/2] wifi: ath12k: Update HTT_TCL_METADATA version and bit mask definitions

[Balamurugan Mahalingam]

Update the HTT_TCL_METADATA version to the latest version (2)
as the bit definitions have changed a little to support more
features. This new version allows the host to submit a packet with
more information to the firmware. Firmware uses this additional
information to do special processing for certain frames.

All the firmware binaries available in upstream/public are compatible with
this HTT version update.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Balamurugan Mahalingam <quic_bmahalin@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/dp.h    | 21 ++++++++++++++-------
 drivers/net/wireless/ath/ath12k/dp_tx.c | 12 ++++++++++--
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index b178921aaf1d..98012e8cb30d 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH12K_DP_H
@@ -373,17 +373,18 @@ struct ath12k_dp {
 };
 
 /* HTT definitions */
+#define HTT_TAG_TCL_METADATA_VERSION		5
 
-#define HTT_TCL_META_DATA_TYPE			BIT(0)
-#define HTT_TCL_META_DATA_VALID_HTT		BIT(1)
+#define HTT_TCL_META_DATA_TYPE			GENMASK(1, 0)
+#define HTT_TCL_META_DATA_VALID_HTT		BIT(2)
 
 /* vdev meta data */
-#define HTT_TCL_META_DATA_VDEV_ID		GENMASK(9, 2)
-#define HTT_TCL_META_DATA_PDEV_ID		GENMASK(11, 10)
-#define HTT_TCL_META_DATA_HOST_INSPECTED	BIT(12)
+#define HTT_TCL_META_DATA_VDEV_ID		 GENMASK(10, 3)
+#define HTT_TCL_META_DATA_PDEV_ID		 GENMASK(12, 11)
+#define HTT_TCL_META_DATA_HOST_INSPECTED_MISSION BIT(13)
 
 /* peer meta data */
-#define HTT_TCL_META_DATA_PEER_ID		GENMASK(15, 2)
+#define HTT_TCL_META_DATA_PEER_ID		GENMASK(15, 3)
 
 /* HTT tx completion is overlaid in wbm_release_ring */
 #define HTT_TX_WBM_COMP_INFO0_STATUS		GENMASK(16, 13)
@@ -414,9 +415,15 @@ enum htt_h2t_msg_type {
 };
 
 #define HTT_VER_REQ_INFO_MSG_ID		GENMASK(7, 0)
+#define HTT_OPTION_TCL_METADATA_VER_V2	2
+#define HTT_OPTION_TAG			GENMASK(7, 0)
+#define HTT_OPTION_LEN			GENMASK(15, 8)
+#define HTT_OPTION_VALUE		GENMASK(31, 16)
+#define HTT_TCL_METADATA_VER_SZ		4
 
 struct htt_ver_req_cmd {
 	__le32 ver_reg_info;
+	__le32 tcl_metadata_version;
 } __packed;
 
 enum htt_srng_ring_type {
diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
index a8d341a6df01..76626d925adf 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: BSD-3-Clause-Clear
 /*
  * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include "core.h"
@@ -971,7 +971,15 @@ int ath12k_dp_tx_htt_h2t_ver_req_msg(struct ath12k_base *ab)
 	skb_put(skb, len);
 	cmd = (struct htt_ver_req_cmd *)skb->data;
 	cmd->ver_reg_info = le32_encode_bits(HTT_H2T_MSG_TYPE_VERSION_REQ,
-					     HTT_VER_REQ_INFO_MSG_ID);
+					     HTT_OPTION_TAG);
+
+	cmd->tcl_metadata_version = le32_encode_bits(HTT_TAG_TCL_METADATA_VERSION,
+						     HTT_OPTION_TAG);
+	cmd->tcl_metadata_version |= le32_encode_bits(HTT_TCL_METADATA_VER_SZ,
+						      HTT_OPTION_LEN);
+	cmd->tcl_metadata_version |=
+			le32_encode_bits(HTT_OPTION_TCL_METADATA_VER_V2,
+					 HTT_OPTION_VALUE);
 
 	ret = ath12k_htc_send(&ab->htc, dp->eid, skb);
 	if (ret) {
-- 
2.34.1

Re: [PATCH 1/2] wifi: ath12k: Update HTT_TCL_METADATA version and bit mask definitions

[Jeff Johnson]

On 1/9/2025 10:49 AM, Balamurugan Mahalingam wrote:
> Update the HTT_TCL_METADATA version to the latest version (2)
> as the bit definitions have changed a little to support more
> features. This new version allows the host to submit a packet with
> more information to the firmware. Firmware uses this additional
> information to do special processing for certain frames.
> 
> All the firmware binaries available in upstream/public are compatible with
> this HTT version update.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Balamurugan Mahalingam <quic_bmahalin@quicinc.com>

Acked-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com>

[PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Balamurugan Mahalingam]

For MLO netdevice, the broadcast frame should be transmitted with the
same sequence number on all the links.

Per IEEE 802.11be-2024 section 10.3.2.14.2 "Transmitter requirements",
An AP MLD shall use SNS11 in Table 10-5 (Transmitter sequence number
spaces) maintained by the MLD to determine the sequence number of a
group addressed data frame that is transmitted by an AP affiliated
with the AP MLD so that the same group addressed Data frame
transmitted over multiple links by the AP MLD uses the same
sequence number for transmission on each link.

Currently the MLO multicast handling is done in the mac80211 layer.
Enable support for handling MLO Multicast in the driver to update the
hardware descriptors in a custom way to handle the multicast frames.
Firmware expects the MLO multicast frames to the submitted to the
hardware with special vdev_id (actual vdev_id + 128) to recognize it as
a host inspected frame to avoid using the reinjected path and it also
uses the multicast global sequence number (GSN) provided by the host
in the HTT metadata to process and transmit it with the same sequence
number.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Balamurugan Mahalingam <quic_bmahalin@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.h  |  3 +-
 drivers/net/wireless/ath/ath12k/dp.h    |  6 ++
 drivers/net/wireless/ath/ath12k/dp_tx.c | 15 +++-
 drivers/net/wireless/ath/ath12k/dp_tx.h |  4 +-
 drivers/net/wireless/ath/ath12k/mac.c   | 94 +++++++++++++++++++++++--
 5 files changed, 111 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 3dd01ad100c5..203255081221 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH12K_CORE_H
@@ -327,6 +327,7 @@ struct ath12k_vif {
 	u32 key_cipher;
 	u8 tx_encap_type;
 	bool ps;
+	atomic_t mcbc_gsn;
 
 	struct ath12k_link_vif deflink;
 	struct ath12k_link_vif __rcu *link[ATH12K_NUM_MAX_LINKS];
diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index 98012e8cb30d..b2f978f50304 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -386,6 +386,12 @@ struct ath12k_dp {
 /* peer meta data */
 #define HTT_TCL_META_DATA_PEER_ID		GENMASK(15, 3)
 
+/* Global sequence number */
+#define HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM		3
+#define HTT_TCL_META_DATA_GLOBAL_SEQ_HOST_INSPECTED	BIT(2)
+#define HTT_TCL_META_DATA_GLOBAL_SEQ_NUM		GENMASK(14, 3)
+#define HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID	128
+
 /* HTT tx completion is overlaid in wbm_release_ring */
 #define HTT_TX_WBM_COMP_INFO0_STATUS		GENMASK(16, 13)
 #define HTT_TX_WBM_COMP_INFO1_REINJECT_REASON	GENMASK(3, 0)
diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
index 76626d925adf..9d05fe7a870c 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
@@ -217,7 +217,7 @@ static int ath12k_dp_tx_align_payload(struct ath12k_base *ab,
 }
 
 int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
-		 struct sk_buff *skb)
+		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn)
 {
 	struct ath12k_base *ab = ar->ab;
 	struct ath12k_dp *dp = &ab->dp;
@@ -290,13 +290,24 @@ int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
 		msdu_ext_desc = true;
 	}
 
+	if (gsn_valid) {
+		ti.meta_data_flags =
+			u32_encode_bits(HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM,
+					HTT_TCL_META_DATA_TYPE) |
+			u32_encode_bits(mcbc_gsn, HTT_TCL_META_DATA_GLOBAL_SEQ_NUM);
+	}
+
 	ti.encap_type = ath12k_dp_tx_get_encap_type(arvif, skb);
 	ti.addr_search_flags = arvif->hal_addr_search_flags;
 	ti.search_type = arvif->search_type;
 	ti.type = HAL_TCL_DESC_TYPE_BUFFER;
 	ti.pkt_offset = 0;
 	ti.lmac_id = ar->lmac_id;
-	ti.vdev_id = arvif->vdev_id;
+	if (gsn_valid)
+		ti.vdev_id = arvif->vdev_id +
+			     HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;
+	else
+		ti.vdev_id = arvif->vdev_id;
 	ti.bss_ast_hash = arvif->ast_hash;
 	ti.bss_ast_idx = arvif->ast_idx;
 	ti.dscp_tid_tbl_idx = 0;
diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.h b/drivers/net/wireless/ath/ath12k/dp_tx.h
index 46dce23501f3..a5904097dc34 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.h
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2022, 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH12K_DP_TX_H
@@ -17,7 +17,7 @@ struct ath12k_dp_htt_wbm_tx_status {
 
 int ath12k_dp_tx_htt_h2t_ver_req_msg(struct ath12k_base *ab);
 int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
-		 struct sk_buff *skb);
+		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn);
 void ath12k_dp_tx_completion_handler(struct ath12k_base *ab, int ring_id);
 
 int ath12k_dp_tx_htt_h2t_ppdu_stats_req(struct ath12k *ar, u32 mask);
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 48d110e2a7de..562135893926 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: BSD-3-Clause-Clear
 /*
  * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <net/mac80211.h>
@@ -7064,6 +7064,22 @@ static void ath12k_mac_add_p2p_noa_ie(struct ath12k *ar,
 	spin_unlock_bh(&ar->data_lock);
 }
 
+/* Note: called under rcu_read_lock() */
+static void ath12k_mlo_mcast_update_tx_link_address(struct ieee80211_vif *vif,
+						    u8 link, struct sk_buff *skb,
+						    u32 info_flags)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_bss_conf *bss_conf;
+
+	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP)
+		return;
+
+	bss_conf = rcu_dereference(vif->link_conf[link]);
+	if (bss_conf)
+		ether_addr_copy(hdr->addr2, bss_conf->addr);
+}
+
 /* Note: called under rcu_read_lock() */
 static u8 ath12k_mac_get_tx_link(struct ieee80211_sta *sta, struct ieee80211_vif *vif,
 				 u8 link, struct sk_buff *skb, u32 info_flags)
@@ -7175,9 +7191,15 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 	struct ieee80211_key_conf *key = info->control.hw_key;
 	struct ieee80211_sta *sta = control->sta;
+	struct ath12k_link_vif *tmp_arvif;
 	u32 info_flags = info->flags;
-	struct ath12k *ar;
+	struct sk_buff *msdu_copied;
+	struct ath12k *ar, *tmp_ar;
+	unsigned long links_map;
+	bool is_mcast = false;
+	struct ethhdr *eth;
 	bool is_prb_rsp;
+	u16 mcbc_gsn;
 	u8 link_id;
 	int ret;
 
@@ -7214,7 +7236,9 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 	is_prb_rsp = ieee80211_is_probe_resp(hdr->frame_control);
 
 	if (info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP) {
+		eth = (struct ethhdr *)skb->data;
 		skb_cb->flags |= ATH12K_SKB_HW_80211_ENCAP;
+		is_mcast = is_multicast_ether_addr(eth->h_dest);
 	} else if (ieee80211_is_mgmt(hdr->frame_control)) {
 		ret = ath12k_mac_mgmt_tx(ar, skb, is_prb_rsp);
 		if (ret) {
@@ -7225,14 +7249,70 @@ static void ath12k_mac_op_tx(struct ieee80211_hw *hw,
 		return;
 	}
 
+	if (!(info_flags & IEEE80211_TX_CTL_HW_80211_ENCAP))
+		is_mcast = is_multicast_ether_addr(hdr->addr1);
+
 	/* This is case only for P2P_GO */
 	if (vif->type == NL80211_IFTYPE_AP && vif->p2p)
 		ath12k_mac_add_p2p_noa_ie(ar, vif, skb, is_prb_rsp);
 
-	ret = ath12k_dp_tx(ar, arvif, skb);
-	if (ret) {
-		ath12k_warn(ar->ab, "failed to transmit frame %d\n", ret);
-		ieee80211_free_txskb(hw, skb);
+	if (!vif->valid_links || !is_mcast ||
+	    test_bit(ATH12K_FLAG_RAW_MODE, &ar->ab->dev_flags)) {
+		ret = ath12k_dp_tx(ar, arvif, skb, false, 0);
+		if (unlikely(ret)) {
+			ath12k_warn(ar->ab, "failed to transmit frame %d\n", ret);
+			ieee80211_free_txskb(ar->ah->hw, skb);
+			return;
+		}
+	} else {
+		mcbc_gsn = atomic_inc_return(&ahvif->mcbc_gsn) & 0xfff;
+
+		links_map = ahvif->links_map;
+		for_each_set_bit(link_id, &links_map,
+				 IEEE80211_MLD_MAX_NUM_LINKS) {
+			tmp_arvif = rcu_dereference(ahvif->link[link_id]);
+			if (!tmp_arvif || !tmp_arvif->is_up)
+				continue;
+
+			tmp_ar = tmp_arvif->ar;
+			msdu_copied = skb_copy(skb, GFP_ATOMIC);
+			if (!msdu_copied) {
+				ath12k_err(ar->ab,
+					   "skb copy failure link_id 0x%X vdevid 0x%X\n",
+					   link_id, tmp_arvif->vdev_id);
+				continue;
+			}
+
+			ath12k_mlo_mcast_update_tx_link_address(vif, link_id,
+								msdu_copied,
+								info_flags);
+
+			skb_cb = ATH12K_SKB_CB(msdu_copied);
+			info = IEEE80211_SKB_CB(msdu_copied);
+			skb_cb->link_id = link_id;
+
+			ret = ath12k_dp_tx(tmp_ar, tmp_arvif,
+					   msdu_copied, true, mcbc_gsn);
+
+			if (unlikely(ret)) {
+				if (ret == -ENOMEM) {
+					/* Drops are expected during heavy multicast
+					 * frame flood. Print with debug log
+					 * level to avoid lot of console prints
+					 */
+					ath12k_dbg(ar->ab, ATH12K_DBG_MAC,
+						   "failed to transmit frame %d\n",
+						   ret);
+				} else {
+					ath12k_warn(ar->ab,
+						    "failed to transmit frame %d\n",
+						    ret);
+				}
+
+				dev_kfree_skb_any(msdu_copied);
+			}
+		}
+		ieee80211_free_txskb(ar->ah->hw, skb);
 	}
 }
 
@@ -10944,6 +11024,8 @@ static int ath12k_mac_hw_register(struct ath12k_hw *ah)
 		ath12k_iftypes_ext_capa[2].eml_capabilities = cap->eml_cap;
 		ath12k_iftypes_ext_capa[2].mld_capa_and_ops = cap->mld_cap;
 		wiphy->flags |= WIPHY_FLAG_SUPPORTS_MLO;
+
+		ieee80211_hw_set(hw, MLO_MCAST_MULTI_LINK_TX);
 	}
 
 	hw->queues = ATH12K_HW_MAX_QUEUES;
-- 
2.34.1

Re: [PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Jeff Johnson]

On 1/9/2025 10:49 AM, Balamurugan Mahalingam wrote:
> For MLO netdevice, the broadcast frame should be transmitted with the
> same sequence number on all the links.
> 
> Per IEEE 802.11be-2024 section 10.3.2.14.2 "Transmitter requirements",
> An AP MLD shall use SNS11 in Table 10-5 (Transmitter sequence number
> spaces) maintained by the MLD to determine the sequence number of a
> group addressed data frame that is transmitted by an AP affiliated
> with the AP MLD so that the same group addressed Data frame
> transmitted over multiple links by the AP MLD uses the same
> sequence number for transmission on each link.
> 
> Currently the MLO multicast handling is done in the mac80211 layer.
> Enable support for handling MLO Multicast in the driver to update the
> hardware descriptors in a custom way to handle the multicast frames.
> Firmware expects the MLO multicast frames to the submitted to the
> hardware with special vdev_id (actual vdev_id + 128) to recognize it as
> a host inspected frame to avoid using the reinjected path and it also
> uses the multicast global sequence number (GSN) provided by the host
> in the HTT metadata to process and transmit it with the same sequence
> number.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Balamurugan Mahalingam <quic_bmahalin@quicinc.com>

Acked-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com>

Re: [PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Nicolas Escande]

On Thu Jan 9, 2025 at 7:49 PM CET, Balamurugan Mahalingam wrote:
[...]
> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> index 76626d925adf..9d05fe7a870c 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> @@ -217,7 +217,7 @@ static int ath12k_dp_tx_align_payload(struct ath12k_base *ab,
>  }
>  
>  int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
> -		 struct sk_buff *skb)
> +		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn)
>  {
>  	struct ath12k_base *ab = ar->ab;
>  	struct ath12k_dp *dp = &ab->dp;
> @@ -290,13 +290,24 @@ int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>  		msdu_ext_desc = true;
>  	}
>  
> +	if (gsn_valid) {
> +		ti.meta_data_flags =
> +			u32_encode_bits(HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM,
> +					HTT_TCL_META_DATA_TYPE) |
> +			u32_encode_bits(mcbc_gsn, HTT_TCL_META_DATA_GLOBAL_SEQ_NUM);
> +	}
> +
Hello,

As ti is zeroed initilised isn't safer to do 'ti.meta_data_flags |='
instead of 'ti.meta_data_flags ='. This way if we add another flag in
meta_data_flag in the same func for another reason, we will have less risk of
overriding the said flags ?

>  	ti.encap_type = ath12k_dp_tx_get_encap_type(arvif, skb);
>  	ti.addr_search_flags = arvif->hal_addr_search_flags;
>  	ti.search_type = arvif->search_type;
>  	ti.type = HAL_TCL_DESC_TYPE_BUFFER;
>  	ti.pkt_offset = 0;
>  	ti.lmac_id = ar->lmac_id;
> -	ti.vdev_id = arvif->vdev_id;
> +	if (gsn_valid)
> +		ti.vdev_id = arvif->vdev_id +
> +			     HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;
> +	else
> +		ti.vdev_id = arvif->vdev_id;
>  	ti.bss_ast_hash = arvif->ast_hash;
>  	ti.bss_ast_idx = arvif->ast_idx;
>  	ti.dscp_tid_tbl_idx = 0;

Re: [PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Balamurugan Mahalingam]

On 1/24/2025 3:45 AM, Nicolas Escande wrote:
> On Thu Jan 9, 2025 at 7:49 PM CET, Balamurugan Mahalingam wrote:
> [...]
>> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
>> index 76626d925adf..9d05fe7a870c 100644
>> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
>> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
>> @@ -217,7 +217,7 @@ static int ath12k_dp_tx_align_payload(struct ath12k_base *ab,
>>   }
>>   
>>   int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>> -		 struct sk_buff *skb)
>> +		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn)
>>   {
>>   	struct ath12k_base *ab = ar->ab;
>>   	struct ath12k_dp *dp = &ab->dp;
>> @@ -290,13 +290,24 @@ int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>>   		msdu_ext_desc = true;
>>   	}
>>   
>> +	if (gsn_valid) {
>> +		ti.meta_data_flags =
>> +			u32_encode_bits(HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM,
>> +					HTT_TCL_META_DATA_TYPE) |
>> +			u32_encode_bits(mcbc_gsn, HTT_TCL_META_DATA_GLOBAL_SEQ_NUM);
>> +	}
>> +
> Hello,
> 
> As ti is zeroed initilised isn't safer to do 'ti.meta_data_flags |='
> instead of 'ti.meta_data_flags ='. This way if we add another flag in
> meta_data_flag in the same func for another reason, we will have less risk of
> overriding the said flags ?
> 

Hello Nicolas,
In this case, this is an intentional reset of any previous residual 
value in ti.meta_data_flags field.
The meta_data_flags bit definition is different depending upon the 
META_DATA_TYPE bit's value.

>>   	ti.encap_type = ath12k_dp_tx_get_encap_type(arvif, skb);
>>   	ti.addr_search_flags = arvif->hal_addr_search_flags;
>>   	ti.search_type = arvif->search_type;
>>   	ti.type = HAL_TCL_DESC_TYPE_BUFFER;
>>   	ti.pkt_offset = 0;
>>   	ti.lmac_id = ar->lmac_id;
>> -	ti.vdev_id = arvif->vdev_id;
>> +	if (gsn_valid)
>> +		ti.vdev_id = arvif->vdev_id +
>> +			     HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;
>> +	else
>> +		ti.vdev_id = arvif->vdev_id;
>>   	ti.bss_ast_hash = arvif->ast_hash;
>>   	ti.bss_ast_idx = arvif->ast_idx;
>>   	ti.dscp_tid_tbl_idx = 0;

Re: [PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Jeff Johnson]

On 1/24/2025 1:59 PM, Balamurugan Mahalingam wrote:
> On 1/24/2025 3:45 AM, Nicolas Escande wrote:
>> On Thu Jan 9, 2025 at 7:49 PM CET, Balamurugan Mahalingam wrote:
>> [...]
>>> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
>>> index 76626d925adf..9d05fe7a870c 100644
>>> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
>>> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
>>> @@ -217,7 +217,7 @@ static int ath12k_dp_tx_align_payload(struct ath12k_base *ab,
>>>   }
>>>   
>>>   int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>>> -		 struct sk_buff *skb)
>>> +		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn)
>>>   {
>>>   	struct ath12k_base *ab = ar->ab;
>>>   	struct ath12k_dp *dp = &ab->dp;
>>> @@ -290,13 +290,24 @@ int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>>>   		msdu_ext_desc = true;
>>>   	}
>>>   
>>> +	if (gsn_valid) {
>>> +		ti.meta_data_flags =
>>> +			u32_encode_bits(HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM,
>>> +					HTT_TCL_META_DATA_TYPE) |
>>> +			u32_encode_bits(mcbc_gsn, HTT_TCL_META_DATA_GLOBAL_SEQ_NUM);
>>> +	}
>>> +
>> Hello,
>>
>> As ti is zeroed initilised isn't safer to do 'ti.meta_data_flags |='
>> instead of 'ti.meta_data_flags ='. This way if we add another flag in
>> meta_data_flag in the same func for another reason, we will have less risk of
>> overriding the said flags ?
>>
> 
> Hello Nicolas,
> In this case, this is an intentional reset of any previous residual 
> value in ti.meta_data_flags field.
> The meta_data_flags bit definition is different depending upon the 
> META_DATA_TYPE bit's value.

So in this case there can never be any conflicting information that came from
the assignment:
	ti.meta_data_flags = arvif->tcl_metadata;
??

Perhaps add a comment that states you are intentionally overriding the
information that came from arvif->tcl_metadata?

/jeff

> 
>>>   	ti.encap_type = ath12k_dp_tx_get_encap_type(arvif, skb);
>>>   	ti.addr_search_flags = arvif->hal_addr_search_flags;
>>>   	ti.search_type = arvif->search_type;
>>>   	ti.type = HAL_TCL_DESC_TYPE_BUFFER;
>>>   	ti.pkt_offset = 0;
>>>   	ti.lmac_id = ar->lmac_id;
>>> -	ti.vdev_id = arvif->vdev_id;
>>> +	if (gsn_valid)
>>> +		ti.vdev_id = arvif->vdev_id +
>>> +			     HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;
>>> +	else
>>> +		ti.vdev_id = arvif->vdev_id;
>>>   	ti.bss_ast_hash = arvif->ast_hash;
>>>   	ti.bss_ast_idx = arvif->ast_idx;
>>>   	ti.dscp_tid_tbl_idx = 0;
> 

Re: [PATCH 2/2] wifi: ath12k: Add support for MLO Multicast handling in driver

[Nicolas Escande]

On Thu Jan 9, 2025 at 7:49 PM CET, Balamurugan Mahalingam wrote:
[...]
> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> index 76626d925adf..9d05fe7a870c 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> @@ -217,7 +217,7 @@ static int ath12k_dp_tx_align_payload(struct ath12k_base *ab,
>  }
>  
>  int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
> -		 struct sk_buff *skb)
> +		 struct sk_buff *skb, bool gsn_valid, int mcbc_gsn)
>  {
>  	struct ath12k_base *ab = ar->ab;
>  	struct ath12k_dp *dp = &ab->dp;
> @@ -290,13 +290,24 @@ int ath12k_dp_tx(struct ath12k *ar, struct ath12k_link_vif *arvif,
>  		msdu_ext_desc = true;
>  	}
>  
> +	if (gsn_valid) {
> +		ti.meta_data_flags =
> +			u32_encode_bits(HTT_TCL_META_DATA_TYPE_GLOBAL_SEQ_NUM,
> +					HTT_TCL_META_DATA_TYPE) |
> +			u32_encode_bits(mcbc_gsn, HTT_TCL_META_DATA_GLOBAL_SEQ_NUM);
> +	}
> +
>  	ti.encap_type = ath12k_dp_tx_get_encap_type(arvif, skb);
>  	ti.addr_search_flags = arvif->hal_addr_search_flags;
>  	ti.search_type = arvif->search_type;
>  	ti.type = HAL_TCL_DESC_TYPE_BUFFER;
>  	ti.pkt_offset = 0;
>  	ti.lmac_id = ar->lmac_id;
> -	ti.vdev_id = arvif->vdev_id;
> +	if (gsn_valid)
> +		ti.vdev_id = arvif->vdev_id +
> +			     HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;
> +	else
> +		ti.vdev_id = arvif->vdev_id;
>  	ti.bss_ast_hash = arvif->ast_hash;
>  	ti.bss_ast_idx = arvif->ast_idx;
>  	ti.dscp_tid_tbl_idx = 0;

And this is nit picking but I find the following would be even more readable:
	ti.vdev_id = arvif->vdev_id;
	if (gsn_valid)
		ti.vdev_id += HTT_TX_MLO_MCAST_HOST_REINJECT_BASE_VDEV_ID;

On the plus side, it's even less lines of modified code :)