On 10/29/2024 9:05 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> writes:
>
>> On 10/23/2024 6:30 AM, Kalle Valo wrote:
>>
>>> + rcu_read_lock();
>>> +
>>> + i = 0;
>>
>> nit: setting i=0 doesn't need to be RCU protected
>
> Yeah, but that doesn't cause any issues and this way it's closer to the
> for loop where it's used:
>
>>> + for_each_set_bit(link_id, &links, IEEE80211_MLD_MAX_NUM_LINKS) {
>>> + if (i >= ATH12K_WMI_MLO_MAX_LINKS)
>>> + break;
>
> [...]
>
>>> @@ -2243,12 +2251,38 @@ int ath12k_wmi_send_peer_assoc_cmd(struct ath12k *ar,
>>> ptr += sizeof(*he_mcs);
>>> }
>>>
>>> - /* MLO header tag with 0 length */
>>> - len = 0;
>>> tlv = ptr;
>>> + len = arg->ml.enabled ? sizeof(*ml_params) : 0;
>>> tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, len);
>>> ptr += TLV_HDR_SIZE;
>>> + if (!len)
>>> + goto skip_ml_params;
>>>
>>> + ml_params = ptr;
>>> + ml_params->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_ASSOC_PARAMS,
>>> + len);
>>
>> this is another instance where we are using the same length for two
>> consecutive TLVs -- that doesn't seem right
>
> This is also a similar case of _hdr() vs _cmd_hdr(), does that look ok?
>
here again this is evil. please change to _hdr() and explicitly subtract out
the tlv header size
