Received-SPF: pass (google.com: domain of linux-wireless+bounces-7869-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::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 1/5] wifi: ath12k: Add support to enable
 debugfs_htt_stats
References: <20240510050806.514126-1-quic_rgnanase@quicinc.com>
	<20240510050806.514126-2-quic_rgnanase@quicinc.com>
Date: Tue, 21 May 2024 10:49:36 +0300
In-Reply-To: <20240510050806.514126-2-quic_rgnanase@quicinc.com> (Ramya
	Gnanasekar's message of "Fri, 10 May 2024 10:38:02 +0530")
Message-ID: <87y183d2pb.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>
>
> Create debugfs_htt_stats file when ath12k debugfs support is enabled.
> Add basic ath12k_debugfs_htt_stats_init and handle htt_stats_type
> file operations.
>
> 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>

[...]

> +struct ath12k_dbg_htt_stats {
> +	enum ath12k_dbg_htt_ext_stats_type type;
> +	u32 cfg_param[4];
> +	/* protects shared stats req buffer */
> +	spinlock_t lock;
> +};

Is there a specific reason why a new lock is needed? Why not just use
struct ath12k::data_lock?

> +
>  struct ath12k_debug {
>  	struct dentry *debugfs_pdev;
> +	struct ath12k_dbg_htt_stats htt_stats;
>  };
>  
>  struct ath12k_per_peer_tx_stats {
> diff --git a/drivers/net/wireless/ath/ath12k/debugfs.c b/drivers/net/wireless/ath/ath12k/debugfs.c
> index 8d8ba951093b..30a80f04d824 100644
> --- a/drivers/net/wireless/ath/ath12k/debugfs.c
> +++ b/drivers/net/wireless/ath/ath12k/debugfs.c
> @@ -6,6 +6,7 @@
>  
>  #include "core.h"
>  #include "debugfs.h"
> +#include "debugfs_htt_stats.h"
>  
>  static ssize_t ath12k_write_simulate_radar(struct file *file,
>  					   const char __user *user_buf,
> @@ -87,4 +88,6 @@ void ath12k_debugfs_register(struct ath12k *ar)
>  				    ar->debug.debugfs_pdev, ar,
>  				    &fops_simulate_radar);
>  	}
> +
> +	ath12k_debugfs_htt_stats_init(ar);

Let's try to have consistent naming: ath12k_debugfs_htt_stats_register()

> +static ssize_t ath12k_read_htt_stats_type(struct file *file,
> +					  char __user *user_buf,
> +					  size_t count, loff_t *ppos)
> +{
> +	struct ath12k *ar = file->private_data;
> +	char buf[32];
> +	size_t len;
> +
> +	len = scnprintf(buf, sizeof(buf), "%u\n", ar->debug.htt_stats.type);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}

Access to ar->debug.htt_stats.type isn't protected in any way.

> +
> +static ssize_t ath12k_write_htt_stats_type(struct file *file,
> +					   const char __user *user_buf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct ath12k *ar = file->private_data;
> +	enum ath12k_dbg_htt_ext_stats_type type;
> +	unsigned int cfg_param[4] = {0};
> +	int num_args;
> +
> +	char *buf __free(kfree) = kzalloc(count, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(buf, user_buf, count))
> +		return -EFAULT;
> +
> +	num_args = sscanf(buf, "%u %u %u %u %u\n", &type, &cfg_param[0],
> +			  &cfg_param[1], &cfg_param[2], &cfg_param[3]);
> +	if (!num_args || num_args > 5)
> +		return -EINVAL;
> +
> +	if (type >= ATH12K_DBG_HTT_NUM_EXT_STATS)
> +		return -E2BIG;
> +
> +	if (type == ATH12K_DBG_HTT_EXT_STATS_RESET)
> +		return -EPERM;
> +
> +	ar->debug.htt_stats.type = type;
> +	ar->debug.htt_stats.cfg_param[0] = cfg_param[0];
> +	ar->debug.htt_stats.cfg_param[1] = cfg_param[1];
> +	ar->debug.htt_stats.cfg_param[2] = cfg_param[2];
> +	ar->debug.htt_stats.cfg_param[3] = cfg_param[3];
> +
> +	return count;
> +}

Same here with both type and cfg_param. Maybe it's ok to skip
protection, I didn't do analysis yet, but this makes me suspicious.

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

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