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

[Roopni Devanathan]

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.