Received-SPF: pass (google.com: domain of linux-wireless+bounces-8629-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <7c7ea029-6ef9-4e7b-ac45-bdf341a8256a@quicinc.com>
Date: Thu, 6 Jun 2024 09:42:07 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 4/5] wifi: ath12k: Add support to parse requested
 stats_type
Content-Language: en-US
To: Ramya Gnanasekar <quic_rgnanase@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Dinesh Karthikeyan
	<quic_dinek@quicinc.com>
References: <20240603044214.960261-1-quic_rgnanase@quicinc.com>
 <20240603044214.960261-5-quic_rgnanase@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240603044214.960261-5-quic_rgnanase@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 6/2/2024 9:42 PM, Ramya Gnanasekar wrote:
> From: Dinesh Karthikeyan <quic_dinek@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