Re: [PATCH v2 1/5] wifi: ath12k: Add support to enable debugfs_htt_stats

[Kalle Valo <kvalo@kernel.org>]

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