On 10/29/2024 9:01 AM, Jeff Johnson wrote:
> 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?
>>
>> 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 *tlv* header
> the length param
also note there is the existing inconsistency that some WMI params structs
include the tlv header and some do not. IMO that lack of consistency will also
impact maintainability.
again something for the TODO list.
