On 11/1/2024 7:06 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> writes:
>
>> On 10/29/2024 8:54 AM, Kalle Valo wrote:
>>> Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> 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
