Received-SPF: pass (google.com: domain of linux-wireless+bounces-8086-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <9117445e-10b8-4872-903d-fa23f7733f8f@quicinc.com>
Date: Mon, 27 May 2024 16:12:25 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/5] wifi: ath12k: Add support to enable
 debugfs_htt_stats
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        Dinesh
 Karthikeyan <quic_dinek@quicinc.com>
References: <20240510050806.514126-1-quic_rgnanase@quicinc.com>
 <20240510050806.514126-2-quic_rgnanase@quicinc.com>
 <87y183d2pb.fsf@kernel.org>
Content-Language: en-US
From: Ramya Gnanasekar <quic_rgnanase@quicinc.com>
In-Reply-To: <87y183d2pb.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit


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.