Received-SPF: pass (google.com: domain of linux-wireless+bounces-7873-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Kalle Valo <kvalo@kernel.org>
To: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
Cc: ath12k@lists.infradead.org,  linux-wireless@vger.kernel.org,  Dinesh
 Karthikeyan <quic_dinek@quicinc.com>
Subject: Re: [PATCH v2 2/5] wifi: ath12k: Add htt_stats_dump file ops support
References: <20240510050806.514126-1-quic_rgnanase@quicinc.com>
	<20240510050806.514126-3-quic_rgnanase@quicinc.com>
Date: Tue, 21 May 2024 10:53:59 +0300
In-Reply-To: <20240510050806.514126-3-quic_rgnanase@quicinc.com> (Ramya
	Gnanasekar's message of "Fri, 10 May 2024 10:38:03 +0530")
Message-ID: <87ttird2i0.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:

> From: Dinesh Karthikeyan <quic_dinek@quicinc.com>
>
> Add dump_htt_stats file operation to dump the stats value requested
> for the requested stats_type.
> Stats sent from firmware will be cumulative. Hence add debugfs to reset
> the requested stats type.
>
> Example with one ath12k device:
>
> ath12k
> `-- pci-0000:06:00.0
>     |-- mac0
>         `-- htt_stats
>         |-- htt_stats_type
>         |-- htt_stats_reset
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Dinesh Karthikeyan <quic_dinek@quicinc.com>
> Co-developed-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/core.h        |   2 +
>  .../wireless/ath/ath12k/debugfs_htt_stats.c   | 202 ++++++++++++++++++
>  .../wireless/ath/ath12k/debugfs_htt_stats.h   |  30 +++
>  3 files changed, 234 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 3919bc828620..77c0842e5ab0 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -477,6 +477,8 @@ struct ath12k_fw_stats {
>  struct ath12k_dbg_htt_stats {
>  	enum ath12k_dbg_htt_ext_stats_type type;
>  	u32 cfg_param[4];
> +	u8 reset;
> +	struct debug_htt_stats_req *stats_req;

Please document with a code comment how these are protected

>  	/* protects shared stats req buffer */
>  	spinlock_t lock;

I'm guessing "stats req buffer" refers to struct
ath12k_dbg_htt_stats::stats_req. But at least in
ath12k_release_htt_stats() you are using conf_mutex for protection:

static int ath12k_release_htt_stats(struct inode *inode,
				    struct file *file)
{
	struct ath12k *ar = inode->i_private;

	mutex_lock(&ar->conf_mutex);
	kfree(file->private_data);
	ar->debug.htt_stats.stats_req = NULL;
	mutex_unlock(&ar->conf_mutex);

	return 0;
}

> +static int ath12k_debugfs_htt_stats_req(struct ath12k *ar)
> +{
> +	struct debug_htt_stats_req *stats_req = ar->debug.htt_stats.stats_req;
> +	enum ath12k_dbg_htt_ext_stats_type type = stats_req->type;
> +	u64 cookie;
> +	int ret, pdev_id = ar->pdev->pdev_id;
> +	struct htt_ext_stats_cfg_params cfg_params = { 0 };
> +
> +	init_completion(&stats_req->htt_stats_rcvd);
> +
> +	stats_req->done = false;
> +	stats_req->pdev_id = pdev_id;

The locking design is not clear for me yet but I suspect this function
needs:

	lockdep_assert_held(&ar->conf_mutex);

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

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