diff mbox series

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

Message ID 20240510050806.514126-2-quic_rgnanase@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Add support to enable debugfs_htt_stats | expand

Commit Message

Ramya Gnanasekar May 10, 2024, 5:08 a.m. UTC 
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>
---
 drivers/net/wireless/ath/ath12k/Makefile      |  2 +-
 drivers/net/wireless/ath/ath12k/core.h        |  9 +++
 drivers/net/wireless/ath/ath12k/debugfs.c     |  3 +
 .../wireless/ath/ath12k/debugfs_htt_stats.c   | 74 +++++++++++++++++++
 .../wireless/ath/ath12k/debugfs_htt_stats.h   | 20 +++++
 5 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
 create mode 100644 drivers/net/wireless/ath/ath12k/debugfs_htt_stats.h

Comments

Jeff Johnson May 10, 2024, 7:53 p.m. UTC | #1 
On 5/9/2024 10:08 PM, Ramya Gnanasekar wrote:
> 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>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Kalle Valo May 21, 2024, 7:49 a.m. UTC | #2 
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.
Ramya Gnanasekar May 27, 2024, 10:42 a.m. UTC | #3 
On 5/21/2024 1:19 PM, Kalle Valo wrote:
> 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?

We can use ath12k::data_lock as well since that is also per radio. I
will check and address in next version.

> 
>> +
>>  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()

Sure Kalle. I will take care in next version.
> 
>> +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.

I will check and address this.
> 
>> +
>> +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.>
Sure Kalle. I will check and address the same.
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/Makefile b/drivers/net/wireless/ath/ath12k/Makefile
index d42480db7463..3491b8b8a1e2 100644
--- a/drivers/net/wireless/ath/ath12k/Makefile
+++ b/drivers/net/wireless/ath/ath12k/Makefile
@@ -23,7 +23,7 @@  ath12k-y += core.o \
 	    fw.o \
 	    p2p.o
 
-ath12k-$(CONFIG_ATH12K_DEBUGFS) += debugfs.o
+ath12k-$(CONFIG_ATH12K_DEBUGFS) += debugfs.o debugfs_htt_stats.o
 ath12k-$(CONFIG_ACPI) += acpi.o
 ath12k-$(CONFIG_ATH12K_TRACING) += trace.o
 
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index bb6c1b562baf..3919bc828620 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -27,6 +27,7 @@ 
 #include "dbring.h"
 #include "fw.h"
 #include "acpi.h"
+#include "debugfs_htt_stats.h"
 
 #define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)
 
@@ -473,8 +474,16 @@  struct ath12k_fw_stats {
 	struct list_head bcn;
 };
 
+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;
+};
+
 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);
 }
diff --git a/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
new file mode 100644
index 000000000000..4a58362bdaea
--- /dev/null
+++ b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.c
@@ -0,0 +1,74 @@ 
+// SPDX-License-Identifier: BSD-3-Clause-Clear
+/*
+ * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/vmalloc.h>
+#include "core.h"
+#include "debug.h"
+#include "debugfs_htt_stats.h"
+
+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);
+}
+
+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;
+}
+
+static const struct file_operations fops_htt_stats_type = {
+	.read = ath12k_read_htt_stats_type,
+	.write = ath12k_write_htt_stats_type,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+void ath12k_debugfs_htt_stats_init(struct ath12k *ar)
+{
+	spin_lock_init(&ar->debug.htt_stats.lock);
+	debugfs_create_file("htt_stats_type", 0600, ar->debug.debugfs_pdev,
+			    ar, &fops_htt_stats_type);
+}
diff --git a/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.h b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.h
new file mode 100644
index 000000000000..6bd34d72b407
--- /dev/null
+++ b/drivers/net/wireless/ath/ath12k/debugfs_htt_stats.h
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause-Clear */
+/*
+ * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef DEBUG_HTT_STATS_H
+#define DEBUG_HTT_STATS_H
+
+void ath12k_debugfs_htt_stats_init(struct ath12k *ar);
+
+/* htt_dbg_ext_stats_type */
+enum ath12k_dbg_htt_ext_stats_type {
+	ATH12K_DBG_HTT_EXT_STATS_RESET,
+
+	/* keep this last */
+	ATH12K_DBG_HTT_NUM_EXT_STATS,
+};
+
+#endif