[PATCH v4 4/5] wifi: ath12k: Add support to parse requested stats_type

[Jeff Johnson]

On 6/2/2024 9:42 PM, Ramya Gnanasekar wrote:
> From: Dinesh Karthikeyan <quic_dinek at quicinc.com>
> 
> Add extended htt stats parser and print the corresponding TLVs associated
> with the requested htt_stats_type.
> Add support for TX PDEV related htt stats.
> 

while reviewing some other patches in the internal pipeline I discovered I had
overlooked some things in this series, so pointing them out now...

> ---
>  .../wireless/ath/ath12k/debugfs_htt_stats.c   | 315 ++++++++++++++++++
>  .../wireless/ath/ath12k/debugfs_htt_stats.h   | 211 ++++++++++++
>  drivers/net/wireless/ath/ath12k/dp_rx.c       |  10 +-
>  drivers/net/wireless/ath/ath12k/dp_rx.h       |   4 +
>  4 files changed, 536 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
> index d98b971cde7d..e0a0acd5eec8 100644
> --- a/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
> +++ b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
> @@ -9,6 +9,321 @@
>  #include "debug.h"
>  #include "debugfs_htt_stats.h"
>  #include "dp_tx.h"
> +#include "dp_rx.h"
> +
> +static u32
> +print_array_to_buf(u8 *buf, u32 offset, const char *header,
> +		   const __le32 *array, u32 array_len)
> +{
> +	int index = 0;
> +	u8 i;
> +
> +	if (header) {
> +		index += scnprintf(buf + offset,
> +				   ATH12K_HTT_STATS_BUF_SIZE - offset,
> +				   "%s = ", header);
> +	}
> +	for (i = 0; i < array_len; i++) {
> +		index += scnprintf(buf + offset + index,
> +				   (ATH12K_HTT_STATS_BUF_SIZE - offset) - index,
> +				   " %u:%u,", i, le32_to_cpu(array[i]));
> +	}

note that this will end up with a trailing comma, so suggest you add:

	/* have the next print overwrite the last trailing comma */
	index--;

> +	index += scnprintf(buf + offset + index,
> +			   (ATH12K_HTT_STATS_BUF_SIZE - offset) - index,
> +			   "\n\n");
> +	return index;
> +}
> +
> +static void
> +htt_print_tx_pdev_stats_cmn_tlv(const void *tag_buf, u16 tag_len,
> +				struct debug_htt_stats_req *stats_req)
> +{
> +	const struct ath12k_htt_tx_pdev_stats_cmn_tlv *htt_stats_buf = tag_buf;
> +	u8 *buf = stats_req->buf;
> +	u32 len = stats_req->buf_len;
> +	u32 buf_len = ATH12K_HTT_STATS_BUF_SIZE;
> +	u32 mac_id_word = le32_to_cpu(htt_stats_buf->mac_id__word);

this dereference has to occur after the length check, otherwise you may have a
buffer overread

> +
> +	if (tag_len < sizeof(struct ath12k_htt_tx_pdev_stats_cmn_tlv))

sizeof(*htt_stats_buf) is preferred

> +		return;
> +

that concludes my new comments