diff mbox series

[6/8] wifi: ath12k: add multi-link flag in peer create command

Message ID 20241023133004.2253830-7-kvalo@kernel.org
State Superseded
Headers show
Series wifi: ath12k: MLO support part 2 | expand

Commit Message

Kalle Valo Oct. 23, 2024, 1:30 p.m. UTC 
From: Sriram R <quic_srirrama@quicinc.com>

Driver should indicate to firmware whether a peer is multi-link or not in peer
create command using multi-link flag. Add changes to support
WMI_TAG_MLO_PEER_CREATE_PARAMS in WMI_PEER_CREATE_CMDID.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-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: Sriram R <quic_srirrama@quicinc.com>
Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/mac.c |  5 +++--
 drivers/net/wireless/ath/ath12k/wmi.c | 27 +++++++++++++++++++++++----
 drivers/net/wireless/ath/ath12k/wmi.h |  6 ++++++
 3 files changed, 32 insertions(+), 6 deletions(-)

Comments

Ping-Ke Shih Oct. 24, 2024, 1:22 a.m. UTC | #1 
Kalle Valo <kvalo@kernel.org> wrote:
> @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
>         struct ath12k_vif *ahvif = arvif->ahvif;
>         struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
>         struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
> -       struct ath12k_wmi_peer_create_arg peer_param;
> +       struct ath12k_wmi_peer_create_arg peer_param = {0};

Using '= {}' as initializer would be better, no matter first field of struct
will be changed in the future.
Kalle Valo Oct. 29, 2024, 3:54 p.m. UTC | #2 
Jeff Johnson <quic_jjohnson@quicinc.com> writes:

>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>  	cmd->peer_type = cpu_to_le32(arg->peer_type);
>>  	cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>  
>> +	ptr = skb->data + sizeof(*cmd);
>> +	tlv = ptr;
>> +	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>> +					 sizeof(*ml_param));
>
> using the same TLV size both here and for the TLV that follows doesn't seem
> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?

I have forgotten the details of WMI voodoo so I can't really comment
right now :)

>> +	ptr += TLV_HDR_SIZE;
>> +	ml_param = ptr;
>> +	ml_param->tlv_header =
>> +			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>> +					       sizeof(*ml_param));

But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
reduces the header size:

static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
{
	return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
}
Jeff Johnson Oct. 29, 2024, 4:01 p.m. UTC | #3 
On 10/29/2024 8:54 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> 
>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>  	cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>  	cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>  
>>> +	ptr = skb->data + sizeof(*cmd);
>>> +	tlv = ptr;
>>> +	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>> +					 sizeof(*ml_param));
>>
>> using the same TLV size both here and for the TLV that follows doesn't seem
>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
> 
> I have forgotten the details of WMI voodoo so I can't really comment
> right now :)
> 
>>> +	ptr += TLV_HDR_SIZE;
>>> +	ml_param = ptr;
>>> +	ml_param->tlv_header =
>>> +			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>> +					       sizeof(*ml_param));
> 
> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
> reduces the header size:
> 
> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
> {
> 	return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
> }
> 

Yes, I missed that since that is evil to use the _cmd_ TLV function on
something that isn't the command TLV.

Please fix to use the standard function and subtract the thv header size from
the length param
Kalle Valo Nov. 1, 2024, 2:06 p.m. UTC | #4 
Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> 
>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>>  	cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>>  	cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>>  
>>>> +	ptr = skb->data + sizeof(*cmd);
>>>> +	tlv = ptr;
>>>> +	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>>> +					 sizeof(*ml_param));
>>>
>>> using the same TLV size both here and for the TLV that follows doesn't seem
>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?

So I assume you are referring to this:

	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
					 sizeof(*ml_param));
	ptr += TLV_HDR_SIZE;
	ml_param = ptr;
	ml_param->tlv_header =
			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
					       sizeof(*ml_param));

I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work
but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any
ideas?

>>>> +	ptr += TLV_HDR_SIZE;
>>>> +	ml_param = ptr;
>>>> +	ml_param->tlv_header =
>>>> +			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>>> +					       sizeof(*ml_param));
>> 
>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
>> reduces the header size:
>> 
>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
>> {
>> 	return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
>> }
>> 
>
> Yes, I missed that since that is evil to use the _cmd_ TLV function on
> something that isn't the command TLV.

Ok, so you are saying that we should have a identical function but with
name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think
that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with
several WMI params, like in ath12k_wmi_wow_add_pattern().

> Please fix to use the standard function and subtract the thv header size from
> the length param

I'm not a fan of manually subtracting lengths, as then it's easy to miss
something. I would prefer to have functions for handling the length
calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr().
Jeff Johnson Nov. 1, 2024, 3:37 p.m. UTC | #5 
On 11/1/2024 7:06 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> 
>> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>>
>>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
>>>>>  	cmd->peer_type = cpu_to_le32(arg->peer_type);
>>>>>  	cmd->vdev_id = cpu_to_le32(arg->vdev_id);
>>>>>  
>>>>> +	ptr = skb->data + sizeof(*cmd);
>>>>> +	tlv = ptr;
>>>>> +	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
>>>>> +					 sizeof(*ml_param));
>>>>
>>>> using the same TLV size both here and for the TLV that follows doesn't seem
>>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header?
> 
> So I assume you are referring to this:
> 
> 	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
> 					 sizeof(*ml_param));
> 	ptr += TLV_HDR_SIZE;
> 	ml_param = ptr;
> 	ml_param->tlv_header =
> 			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
> 					       sizeof(*ml_param));
> 
> I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work
> but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any
> ideas?
> 
>>>>> +	ptr += TLV_HDR_SIZE;
>>>>> +	ml_param = ptr;
>>>>> +	ml_param->tlv_header =
>>>>> +			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
>>>>> +					       sizeof(*ml_param));
>>>
>>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it
>>> reduces the header size:
>>>
>>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len)
>>> {
>>> 	return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE);
>>> }
>>>
>>
>> Yes, I missed that since that is evil to use the _cmd_ TLV function on
>> something that isn't the command TLV.
> 
> Ok, so you are saying that we should have a identical function but with
> name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think
> that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with
> several WMI params, like in ath12k_wmi_wow_add_pattern().
> 
>> Please fix to use the standard function and subtract the thv header size from
>> the length param
> 
> I'm not a fan of manually subtracting lengths, as then it's easy to miss
> something. I would prefer to have functions for handling the length
> calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr().
> 

Whether it is manually subtracting lengths or using tlv_hdr vs tlv_param_hdr,
both are easy to miss due to the lack of consistency in the params struct
definitions. IMO the only way to avoid this is to consistently either always
have the tlv header or to never have the tlv header in the params structs.
This lack of consistency is the real underlying issue since whether or not you
have the tlv header in the params struct is what dictates whether or not you
need to account for the header when filling the TLV length, as well as if you
have to separately account for it when you are determining the buffer
allocation size and when you are populating the TLV header in the buffer.

But the current code actually allocates and fills the data as firmware expects
it for this command, so let's keep the current code for now, and we can
discuss if it is appropriate to later update to achieve the consistency mentioned.

/jeff
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 019a1a6c6777..b628bc2fd0f5 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -4981,8 +4981,9 @@  static int ath12k_mac_station_add(struct ath12k *ar,
 	}
 
 	peer_param.vdev_id = arvif->vdev_id;
-	peer_param.peer_addr = sta->addr;
+	peer_param.peer_addr = arsta->addr;
 	peer_param.peer_type = WMI_PEER_TYPE_DEFAULT;
+	peer_param.ml_enabled = sta->mlo;
 
 	ret = ath12k_peer_create(ar, arvif, sta, &peer_param);
 	if (ret) {
@@ -7016,7 +7017,7 @@  int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif)
 	struct ath12k_vif *ahvif = arvif->ahvif;
 	struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif);
 	struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
-	struct ath12k_wmi_peer_create_arg peer_param;
+	struct ath12k_wmi_peer_create_arg peer_param = {0};
 	struct ieee80211_bss_conf *link_conf;
 	u32 param_id, param_value;
 	u16 nss;
diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
index e089b58bbea1..0583d832fac7 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.c
+++ b/drivers/net/wireless/ath/ath12k/wmi.c
@@ -1230,9 +1230,14 @@  int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
 	struct ath12k_wmi_pdev *wmi = ar->wmi;
 	struct wmi_peer_create_cmd *cmd;
 	struct sk_buff *skb;
-	int ret;
+	int ret, len;
+	struct wmi_peer_create_mlo_params *ml_param;
+	void *ptr;
+	struct wmi_tlv *tlv;
 
-	skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd));
+	len = sizeof(*cmd) + TLV_HDR_SIZE + sizeof(*ml_param);
+
+	skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len);
 	if (!skb)
 		return -ENOMEM;
 
@@ -1244,9 +1249,23 @@  int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar,
 	cmd->peer_type = cpu_to_le32(arg->peer_type);
 	cmd->vdev_id = cpu_to_le32(arg->vdev_id);
 
+	ptr = skb->data + sizeof(*cmd);
+	tlv = ptr;
+	tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT,
+					 sizeof(*ml_param));
+	ptr += TLV_HDR_SIZE;
+	ml_param = ptr;
+	ml_param->tlv_header =
+			ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS,
+					       sizeof(*ml_param));
+	if (arg->ml_enabled)
+		ml_param->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED);
+
+	ptr += sizeof(*ml_param);
+
 	ath12k_dbg(ar->ab, ATH12K_DBG_WMI,
-		   "WMI peer create vdev_id %d peer_addr %pM\n",
-		   arg->vdev_id, arg->peer_addr);
+		   "WMI peer create vdev_id %d peer_addr %pM ml_flags 0x%x\n",
+		   arg->vdev_id, arg->peer_addr, ml_param->flags);
 
 	ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_CREATE_CMDID);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
index 33b9643644c6..07bd275608bf 100644
--- a/drivers/net/wireless/ath/ath12k/wmi.h
+++ b/drivers/net/wireless/ath/ath12k/wmi.h
@@ -3026,6 +3026,12 @@  struct ath12k_wmi_peer_create_arg {
 	const u8 *peer_addr;
 	u32 peer_type;
 	u32 vdev_id;
+	bool ml_enabled;
+};
+
+struct wmi_peer_create_mlo_params {
+	__le32 tlv_header;
+	__le32 flags;
 };
 
 struct ath12k_wmi_pdev_set_regdomain_arg {