Re: [PATCH v4 1/4] wifi: ath12k: Support Downlink Pager Stats

[Kalle Valo <kvalo@kernel.org>]

Roopni Devanathan <quic_rdevanat@quicinc.com> writes:

> From: Dinesh Karthikeyan <quic_dinek@quicinc.com>
>
> Add support to request downlink pager stats from firmware through HTT
> stats type 36. These stats give paging information like number of pages,
> their timestamp, number of locked and free pages, synchronous and
> asynchronous locked pages.
>
> Note: MCC firmware version -
> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4 responds to
> the event requesting stats, but it does not give any data.
>
> Sample output:
> -------------
> echo 36 > /sys/kernel/debug/ath12k/pci-0000\:06\:00.0/mac0/htt_stats_type
> cat /sys/kernel/debug/ath12k/pci-0000\:06\:00.0/mac0/htt_stats
> HTT_DLPAGER_STATS_TLV:
> ASYNC locked pages = 2
> SYNC locked pages = 0
> Total locked pages = 2
> Total free pages = 127
>
> LOCKED PAGES HISTORY
> last_locked_page_idx = 0
> Index - 0 ; Page Number - 8495 ; Num of pages - 1 ; Timestamp - 4031009360us
> Index - 1 ; Page Number - 7219 ; Num of pages - 2 ; Timestamp - 885379515us
> Index - 2 ; Page Number - 0 ; Num of pages - 0 ; Timestamp - 0us
> Index - 3 ; Page Number - 0 ; Num of pages - 0 ; Timestamp - 0us
> .....
> UNLOCKED PAGES HISTORY
> last_unlocked_page_idx = 0
> Index - 0 ; Page Number - 7144 ; Num of pages - 2 ; Timestamp - 4032070008us
> Index - 1 ; Page Number - 7214 ; Num of pages - 2 ; Timestamp - 885379512us
> Index - 2 ; Page Number - 0 ; Num of pages - 0 ; Timestamp - 0us
> Index - 3 ; Page Number - 0 ; Num of pages - 0 ; Timestamp - 0us
> .....
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Dinesh Karthikeyan <quic_dinek@quicinc.com>
> Signed-off-by: Roopni Devanathan <quic_rdevanat@quicinc.com>

[...]

> +static void ath12k_htt_print_dlpager_entry(const struct ath12k_htt_pgs_info *pg_info,
> +					   int idx, char *str_buf)
> +{
> +	u32 ts_lo;
> +	u32 ts_hi;
> +	u64 page_timestamp;
> +	u16 index = 0;

Nitpicking but please strive for reverse xmas style and no need to have
just one variable per line:

        u64 page_timestamp;
        u32 ts_lo, ts_hi;
        u16 index = 0;

> +static void
> +ath12k_htt_print_dlpager_stats_tlv(const void *tag_buf, u16 tag_len,
> +				   struct debug_htt_stats_req *stats_req)
> +{
> +	const struct ath12k_htt_dl_pager_stats_tlv *stat_buf = tag_buf;
> +	u8 *buf = stats_req->buf;
> +	u32 len = stats_req->buf_len;
> +	u32 buf_len = ATH12K_HTT_STATS_BUF_SIZE;
> +	u32 info0;
> +	u32 info1;
> +	u32 info2;
> +	u32 dword_lock;
> +	u32 dword_unlock;
> +	u8 pg_locked;
> +	u8 pg_unlock;
> +	int i;
> +	char str_buf[ATH12K_HTT_MAX_STRING_LEN] = {0};

Same here. And maybe initialise buf_len separately to keep the
declarations clean?


> +	if (tag_len < sizeof(*stat_buf))
> +		return;
> +
> +	info0 = le32_to_cpu(stat_buf->info0);
> +	info1 = le32_to_cpu(stat_buf->info1);
> +	info2 = le32_to_cpu(stat_buf->info2);
> +	dword_lock = u32_get_bits(info2, ATH12K_HTT_DLPAGER_TOTAL_LOCK_PAGES_INFO2);
> +	dword_unlock = u32_get_bits(info2, ATH12K_HTT_DLPAGER_TOTAL_FREE_PAGES_INFO2);

There's le32_get_bits() so you can simplify this function quite a lot.

> +	pg_locked = ATH12K_HTT_STATS_PAGE_LOCKED;
> +	pg_unlock = ATH12K_HTT_STATS_PAGE_UNLOCKED;
> +
> +	len += scnprintf(buf + len, buf_len - len, "HTT_DLPAGER_STATS_TLV:\n");
> +	len += scnprintf(buf + len, buf_len - len, "ASYNC locked pages = %u\n",
> +			 u32_get_bits(info0, ATH12K_HTT_DLPAGER_ASYNC_LOCK_PG_CNT_INFO0));
> +	len += scnprintf(buf + len, buf_len - len, "SYNC locked pages = %u\n",
> +			 u32_get_bits(info0, ATH12K_HTT_DLPAGER_SYNC_LOCK_PG_CNT_INFO0));
> +	len += scnprintf(buf + len, buf_len - len, "Total locked pages = %u\n",
> +			 u32_get_bits(info1, ATH12K_HTT_DLPAGER_TOTAL_LOCK_PAGES_INFO1));
> +	len += scnprintf(buf + len, buf_len - len, "Total free pages = %u\n",
> +			 u32_get_bits(info1, ATH12K_HTT_DLPAGER_TOTAL_FREE_PAGES_INFO1));
> +
> +	len += scnprintf(buf + len, buf_len - len, "\nLOCKED PAGES HISTORY\n");
> +	len += scnprintf(buf + len, buf_len - len, "last_locked_page_idx = %u\n",
> +			 dword_lock ? dword_lock - 1 : (ATH12K_PAGER_MAX - 1));
> +	for (i = 0; i < ATH12K_PAGER_MAX; i++) {

Empty line before for.

> +		memset(str_buf, 0x0, ATH12K_HTT_MAX_STRING_LEN);
> +		ath12k_htt_print_dlpager_entry(&stat_buf->pgs_info[pg_locked][i],
> +					       i, str_buf);
> +		len += scnprintf(buf + len, buf_len - len, "%s", str_buf);
> +	}
> +
> +	len += scnprintf(buf + len, buf_len - len, "\nUNLOCKED PAGES HISTORY\n");
> +	len += scnprintf(buf + len, buf_len - len, "last_unlocked_page_idx = %u\n",
> +			 dword_unlock ? dword_unlock - 1 : ATH12K_PAGER_MAX - 1);
> +	for (i = 0; i < ATH12K_PAGER_MAX; i++) {

Empty line before for.

> +		memset(str_buf, 0x0, ATH12K_HTT_MAX_STRING_LEN);
> +		ath12k_htt_print_dlpager_entry(&stat_buf->pgs_info[pg_unlock][i],
> +					       i, str_buf);
> +		len += scnprintf(buf + len, buf_len - len, "%s", str_buf);
> +	}
> +	len += scnprintf(buf + len, buf_len - len, "\n");

Empty line after '}'.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches