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

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 1/7/2025 11:45 AM, Kalle Valo wrote:
> Roopni Devanathan <quic_rdevanat@quicinc.com> writes:
>> From: Pradeep Kumar Chitrapu <quic_pradeepc@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