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

[Kalle Valo <kvalo@kernel.org>]

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().

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

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