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