Ramya Gnanasekar <quic_rgnanase@xxxxxxxxxxx> writes:
> From: Dinesh Karthikeyan <quic_dinek@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Co-developed-by: Ramya Gnanasekar <quic_rgnanase@xxxxxxxxxxx>
> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@xxxxxxxxxxx>
[...]
> +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
