On 5/21/2024 1:19 PM, Kalle Valo wrote:
> 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?
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.
