Re: [v3] ath10k: cache calibration data when the core is stopped

[Kalle Valo]

Kalle Valo  wrote:
> From: Marty Faltesek 
> 
> Commit 0b8e3c4ca29f ("ath10k: move cal data len to hw_params") broke 
> retrieving
> the calibration data from cal_data debugfs file. The length of file was always
> zero. The reason is:
> 
> static ssize_t ath10k_debug_cal_data_read(struct file *file,
>   char __user *user_buf,
>   size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> void *buf = file->private_data;
> 
> 
> This is obviously bogus, private_data cannot contain both struct ath10k and 
> the
> buffer. Fix it by caching calibration data to ar->debug.cal_data. This also
> allows it to be accessed when the device is not active (interface is down).
> 
> The cal_data buffer is fixed size because during the first firmware probe we
> don't yet know what will be the lenght of the calibration data. It was 
> simplest
> just to use a fixed length. There's a WARN_ON() in
> ath10k_debug_cal_data_fetch() if the buffer is too small.
> 
> Tested with qca988x and firmware 10.2.4.70.56.
> 
> Reported-by: Nikolay Martynov 
> Fixes: 0b8e3c4ca29f ("ath10k: move cal data len to hw_params")
> Cc: sta...@vger.kernel.org # 4.7+
> Signed-off-by: Marty Faltesek 
> [kv...@qca.qualcomm.com: improve commit log and minor other changes]
> Signed-off-by: Kalle Valo 

Patch applied to ath-current branch of ath.git, thanks.

f67b107d4ced ath10k: cache calibration data when the core is stopped

-- 
https://patchwork.kernel.org/patch/9369685/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [v3] ath10k: cache calibration data when the core is stopped

[Kalle Valo]

Kalle Valo  wrote:
> From: Marty Faltesek 
> 
> Commit 0b8e3c4ca29f ("ath10k: move cal data len to hw_params") broke 
> retrieving
> the calibration data from cal_data debugfs file. The length of file was always
> zero. The reason is:
> 
> static ssize_t ath10k_debug_cal_data_read(struct file *file,
>   char __user *user_buf,
>   size_t count, loff_t *ppos)
> {
> struct ath10k *ar = file->private_data;
> void *buf = file->private_data;
> 
> 
> This is obviously bogus, private_data cannot contain both struct ath10k and 
> the
> buffer. Fix it by caching calibration data to ar->debug.cal_data. This also
> allows it to be accessed when the device is not active (interface is down).
> 
> The cal_data buffer is fixed size because during the first firmware probe we
> don't yet know what will be the lenght of the calibration data. It was 
> simplest
> just to use a fixed length. There's a WARN_ON() in
> ath10k_debug_cal_data_fetch() if the buffer is too small.
> 
> Tested with qca988x and firmware 10.2.4.70.56.
> 
> Reported-by: Nikolay Martynov 
> Fixes: 0b8e3c4ca29f ("ath10k: move cal data len to hw_params")
> Cc: sta...@vger.kernel.org # 4.7+
> Signed-off-by: Marty Faltesek 
> [kv...@qca.qualcomm.com: improve commit log and minor other changes]
> Signed-off-by: Kalle Valo 

There were few checkpatch warnings, I fixed those in the pending branch.

drivers/net/wireless/ath/ath10k/debug.c:1472: code indent should use tabs where 
possible
drivers/net/wireless/ath/ath10k/debug.c:1504: code indent should use tabs where 
possible
drivers/net/wireless/ath/ath10k/debug.c:1504: please, no spaces at the start of 
a line
drivers/net/wireless/ath/ath10k/debug.c:1505: code indent should use tabs where 
possible
drivers/net/wireless/ath/ath10k/debug.c:1505: please, no spaces at the start of 
a line

-- 
https://patchwork.kernel.org/patch/9369685/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v3] ath10k: cache calibration data when the core is stopped

[Kalle Valo]

From: Marty Faltesek 

Commit 0b8e3c4ca29f ("ath10k: move cal data len to hw_params") broke retrieving
the calibration data from cal_data debugfs file. The length of file was always
zero. The reason is:

static ssize_t ath10k_debug_cal_data_read(struct file *file,
  char __user *user_buf,
  size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
void *buf = file->private_data;


This is obviously bogus, private_data cannot contain both struct ath10k and the
buffer. Fix it by caching calibration data to ar->debug.cal_data. This also
allows it to be accessed when the device is not active (interface is down).

The cal_data buffer is fixed size because during the first firmware probe we
don't yet know what will be the lenght of the calibration data. It was simplest
just to use a fixed length. There's a WARN_ON() in
ath10k_debug_cal_data_fetch() if the buffer is too small.

Tested with qca988x and firmware 10.2.4.70.56.

Reported-by: Nikolay Martynov 
Fixes: 0b8e3c4ca29f ("ath10k: move cal data len to hw_params")
Cc: sta...@vger.kernel.org # 4.7+
Signed-off-by: Marty Faltesek 
[kv...@qca.qualcomm.com: improve commit log and minor other changes]
Signed-off-by: Kalle Valo 
---
 drivers/net/wireless/ath/ath10k/core.h  |1 +
 drivers/net/wireless/ath/ath10k/debug.c |   79 ---
 2 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h 
b/drivers/net/wireless/ath/ath10k/core.h
index 6e5aa2d09699..b7067cc9328d 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -450,6 +450,7 @@ struct ath10k_debug {
u32 pktlog_filter;
u32 reg_addr;
u32 nf_cal_period;
+   void *cal_data;
 
struct ath10k_fw_crash_data *fw_crash_data;
 };
diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index 832da6ed9f13..5d508efa0b31 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -30,6 +30,8 @@
 /* ms */
 #define ATH10K_DEBUG_HTT_STATS_INTERVAL 1000
 
+#define ATH10K_DEBUG_CAL_DATA_LEN 12064
+
 #define ATH10K_FW_CRASH_DUMP_VERSION 1
 
 /**
@@ -1451,75 +1453,68 @@ static const struct file_operations fops_fw_dbglog = {
.llseek = default_llseek,
 };
 
-static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+static int ath10k_debug_cal_data_fetch(struct ath10k *ar)
 {
-   struct ath10k *ar = inode->i_private;
-   void *buf;
u32 hi_addr;
__le32 addr;
int ret;
 
-   mutex_lock(>conf_mutex);
-
-   if (ar->state != ATH10K_STATE_ON &&
-   ar->state != ATH10K_STATE_UTF) {
-   ret = -ENETDOWN;
-   goto err;
-   }
+   lockdep_assert_held(>conf_mutex);
 
-   buf = vmalloc(ar->hw_params.cal_data_len);
-   if (!buf) {
-   ret = -ENOMEM;
-   goto err;
-   }
+   if (WARN_ON(ar->hw_params.cal_data_len > ATH10K_DEBUG_CAL_DATA_LEN))
+   return -EINVAL;
 
hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));
 
ret = ath10k_hif_diag_read(ar, hi_addr, , sizeof(addr));
if (ret) {
-   ath10k_warn(ar, "failed to read hi_board_data address: %d\n", 
ret);
-   goto err_vfree;
+   ath10k_warn(ar, "failed to read hi_board_data address: %d\n",
+   ret);
+   return ret;
}
 
-   ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), buf,
+   ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), ar->debug.cal_data,
   ar->hw_params.cal_data_len);
if (ret) {
ath10k_warn(ar, "failed to read calibration data: %d\n", ret);
-   goto err_vfree;
+   return ret;
}
 
-   file->private_data = buf;
+   return 0;
+}
 
-   mutex_unlock(>conf_mutex);
+static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file)
+{
+   struct ath10k *ar = inode->i_private;
 
-   return 0;
+   mutex_lock(>conf_mutex);
 
-err_vfree:
-   vfree(buf);
+   if (ar->state == ATH10K_STATE_ON ||
+   ar->state == ATH10K_STATE_UTF) {
+   ath10k_debug_cal_data_fetch(ar);
+   }
 
-err:
+   file->private_data = ar;
mutex_unlock(>conf_mutex);
 
-   return ret;
+   return 0;
 }
 
 static ssize_t ath10k_debug_cal_data_read(struct file *file,
- char __user *user_buf,
- size_t count, loff_t *ppos)
+  char __user *user_buf,
+  size_t count, loff_t *ppos)
 {
struct