| Message ID | 20240510050806.514126-2-quic_rgnanase@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: ath12k: Add support to enable debugfs_htt_stats | expand |
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>
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.
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 --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