[PATCH 2/2] wifi: ath12k: Support Transmit Buffer OFDMA Stats

[Jeff Johnson]

On 1/8/2025 9:38 PM, Roopni Devanathan wrote:
> 
> 
> On 1/8/2025 5:08 AM, Jeff Johnson wrote:
>> On 1/7/2025 11:45 AM, Kalle Valo wrote:
>>> Roopni Devanathan <quic_rdevanat at quicinc.com> writes:
>>>> From: Pradeep Kumar Chitrapu <quic_pradeepc at quicinc.com>
>>>> +	len--;
>>>> +	*(buf + len) = '\0';
>>>
>>> Please avoid pointer arithmetic, this is simpler:
>>>
>>> buf[len] = '\0';
>>>
>>> Or should it be just 0 instead of '\0'? Don't know which one is
>>> preferred.
>>>
>>
>> well my take on this is we have a lot of similar pointer arithmetic already in
>> this and other debugfs files, including all of the scnprintf(buf + len, ...)
>> calls which could be scnprintf(&buf[len], ...). And we have existing instances
>> of trailing comma suppression in:
>> print_array_to_buf_index()
>> ath12k_htt_print_txbf_ofdma_ax_ndpa_stats_tlv()
>> ath12k_htt_print_txbf_ofdma_ax_ndp_stats_tlv()
>> and others, all of which assign '\0'
>>
>> so this code looks ok to me.
>>
>> that said, while i've been reviewing the debugfs implementations I thought the
>> code would look much cleaner if we had a wrapper function (or macro).
>>
>> Note that the input to the current debugfs functions includes a struct
>> debug_htt_stats_req *stats_req that maintains the buffer pointer and filled
>> length. what if we had a wrapper that took just that, the format string, and
>> the optional arguments, and all of the pointer arithmetic is encapsulated in
>> that function, rather than being open coded?
>>
>> so instead of:
>> 	len += scnprintf(buf + len, buf_len - len, "some string"");
>> have simply:
>> 	ath12k_scnprintf(stats_req, "some string");
>>
>> that function would extract the buffer pointer and current filled length from
>> stats_req, calculate where to write the next string, call scnprintf() or
>> vscnprintf() to write the string, and then update the current filled length in
>> the stats_req. That would eliminate all of this housekeeping from the callers:
>> buf + len
>> buf_len - len
>> len += scnprintf(..)
>>
>> /jeff
>>
> 
> Since there are a lot of instances where this change is needed, can we raise a
> separate patch for this? If that is okay, we can go ahead with the current
> implementation in this patch.

Yes, I'm taking the current implementation to ath-next

/jeff