Received-SPF: pass (google.com: domain of linux-wireless+bounces-8654-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <b57a06e3-6dcf-4205-83e8-14857c4b7258@quicinc.com>
Date: Fri, 7 Jun 2024 08:25:01 +0530
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
To: Jeff Johnson <quic_jjohnson@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>
 <7c7ea029-6ef9-4e7b-ac45-bdf341a8256a@quicinc.com>
Content-Language: en-US
From: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
In-Reply-To: <7c7ea029-6ef9-4e7b-ac45-bdf341a8256a@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


On 6/6/2024 10:12 PM, Jeff Johnson wrote:
> 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--;

Thanks. Will address the same.
> 
>> +	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
> 

Thanks Jeff. I will address the same.