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@xxxxxxxxxxx>
>>
>> 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.
