[PATCH v2 0/2] ath10k: firmware memory debugfs interface

[PATCH v2 0/2] ath10k: firmware memory debugfs interface

[Kalle Valo]

Hi,

this is v2 of Yanbo's firmware memory debugfs interface.

Changelog:

v2:

* split patches into two

* simplify the commit logs

* remove extra #include "hif.h" from debug.c

---

Yanbo Li (2):
      ath10k: add register access debugfs interface
      ath10k: add memory dump debugfs interface


 drivers/net/wireless/ath/ath10k/core.h  |    2 
 drivers/net/wireless/ath/ath10k/debug.c |  188 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/hif.h   |   37 ++++++
 drivers/net/wireless/ath/ath10k/pci.c   |    3 
 4 files changed, 230 insertions(+)

[PATCH v2 1/2] ath10k: add register access debugfs interface

[Kalle Valo]

From: Yanbo Li <yanbol@qti.qualcomm.com>

Debugfs files reg_addr and reg_val are used for reading and writing to the
firmware (target) registers. reg_addr contains the address to be accessed,
which also needs to be set first, and reg_value is when used for reading and
writing the actual value in ASCII.

To read a value from the firmware register 0x100000:

# echo 0x100000 > reg_addr
# cat reg_value
0x00100000:0x000002d3

To write value 0x2400 to address 0x100000:

# echo 0x100000 > reg_addr
# echo  0x2400 > reg_value
#

Signed-off-by: Yanbo Li <yanbol@qti.qualcomm.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |    2 +
 drivers/net/wireless/ath/ath10k/debug.c |  102 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/hif.h   |   26 ++++++++
 drivers/net/wireless/ath/ath10k/pci.c   |    2 +
 4 files changed, 132 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 8f86bd34e823..5cb4fe97b4ff 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -321,6 +321,8 @@ struct ath10k_debug {
 	u32 fw_dbglog_mask;
 	u32 pktlog_filter;
 
+	u32 reg_addr;
+
 	u8 htt_max_amsdu;
 	u8 htt_max_ampdu;
 
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index f10721da4980..b500cd619c05 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -928,6 +928,102 @@ static const struct file_operations fops_fw_crash_dump = {
 	.llseek = default_llseek,
 };
 
+static ssize_t ath10k_reg_addr_read(struct file *file,
+				    char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u8 buf[32];
+	unsigned int len = 0;
+	u32 reg_addr;
+
+	spin_lock_bh(&ar->data_lock);
+	reg_addr = ar->debug.reg_addr;
+	spin_unlock_bh(&ar->data_lock);
+
+	len += scnprintf(buf + len, sizeof(buf) - len, "0x%x\n", reg_addr);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t ath10k_reg_addr_write(struct file *file,
+				     const char __user *user_buf,
+				     size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u32 reg_addr;
+	int ret;
+
+	ret = kstrtou32_from_user(user_buf, count, 0, &reg_addr);
+	if (ret)
+		return ret;
+
+	if (!IS_ALIGNED(reg_addr, 4))
+		return -EFAULT;
+
+	spin_lock_bh(&ar->data_lock);
+	ar->debug.reg_addr = reg_addr;
+	spin_unlock_bh(&ar->data_lock);
+
+	return count;
+}
+
+static const struct file_operations fops_reg_addr = {
+	.read = ath10k_reg_addr_read,
+	.write = ath10k_reg_addr_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
+static ssize_t ath10k_reg_value_read(struct file *file,
+				     char __user *user_buf,
+				     size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u8 buf[48];
+	unsigned int len;
+	u32 reg_addr, reg_val;
+
+	spin_lock_bh(&ar->data_lock);
+	reg_addr = ar->debug.reg_addr;
+	spin_unlock_bh(&ar->data_lock);
+
+	reg_val = ath10k_hif_read32(ar, reg_addr);
+	len = scnprintf(buf, sizeof(buf), "0x%08x:0x%08x\n", reg_addr, reg_val);
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t ath10k_reg_value_write(struct file *file,
+				      const char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u32 reg_addr, reg_val;
+	int ret;
+
+	spin_lock_bh(&ar->data_lock);
+	reg_addr = ar->debug.reg_addr;
+	spin_unlock_bh(&ar->data_lock);
+
+	ret = kstrtou32_from_user(user_buf, count, 0, &reg_val);
+	if (ret)
+		return ret;
+
+	ath10k_hif_write32(ar, reg_addr, reg_val);
+
+	return count;
+}
+
+static const struct file_operations fops_reg_value = {
+	.read = ath10k_reg_value_read,
+	.write = ath10k_reg_value_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 static int ath10k_debug_htt_stats_req(struct ath10k *ar)
 {
 	u64 cookie;
@@ -1629,6 +1725,12 @@ int ath10k_debug_register(struct ath10k *ar)
 	debugfs_create_file("fw_crash_dump", S_IRUSR, ar->debug.debugfs_phy,
 			    ar, &fops_fw_crash_dump);
 
+	debugfs_create_file("reg_addr", S_IRUSR | S_IWUSR,
+			    ar->debug.debugfs_phy, ar, &fops_reg_addr);
+
+	debugfs_create_file("reg_value", S_IRUSR | S_IWUSR,
+			    ar->debug.debugfs_phy, ar, &fops_reg_value);
+
 	debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
 			    ar, &fops_chip_id);
 
diff --git a/drivers/net/wireless/ath/ath10k/hif.h b/drivers/net/wireless/ath/ath10k/hif.h
index 30301f5b6051..bad071906540 100644
--- a/drivers/net/wireless/ath/ath10k/hif.h
+++ b/drivers/net/wireless/ath/ath10k/hif.h
@@ -20,6 +20,7 @@
 
 #include <linux/kernel.h>
 #include "core.h"
+#include "debug.h"
 
 struct ath10k_hif_sg_item {
 	u16 transfer_id;
@@ -84,6 +85,10 @@ struct ath10k_hif_ops {
 
 	u16 (*get_free_queue_number)(struct ath10k *ar, u8 pipe_id);
 
+	u32 (*read32)(struct ath10k *ar, u32 address);
+
+	void (*write32)(struct ath10k *ar, u32 address, u32 value);
+
 	/* Power up the device and enter BMI transfer mode for FW download */
 	int (*power_up)(struct ath10k *ar);
 
@@ -187,4 +192,25 @@ static inline int ath10k_hif_resume(struct ath10k *ar)
 	return ar->hif.ops->resume(ar);
 }
 
+static inline u32 ath10k_hif_read32(struct ath10k *ar, u32 address)
+{
+	if (!ar->hif.ops->read32) {
+		ath10k_warn(ar, "hif read32 not supported\n");
+		return 0xdeaddead;
+	}
+
+	return ar->hif.ops->read32(ar, address);
+}
+
+static inline void ath10k_hif_write32(struct ath10k *ar,
+				      u32 address, u32 data)
+{
+	if (!ar->hif.ops->write32) {
+		ath10k_warn(ar, "hif write32 not supported\n");
+		return;
+	}
+
+	ar->hif.ops->write32(ar, address, data);
+}
+
 #endif /* _HIF_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 3a6b8a5ca96c..328ab6c5d053 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1998,6 +1998,8 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
 	.get_free_queue_number	= ath10k_pci_hif_get_free_queue_number,
 	.power_up		= ath10k_pci_hif_power_up,
 	.power_down		= ath10k_pci_hif_power_down,
+	.read32			= ath10k_pci_read32,
+	.write32		= ath10k_pci_write32,
 #ifdef CONFIG_PM
 	.suspend		= ath10k_pci_hif_suspend,
 	.resume			= ath10k_pci_hif_resume,

[PATCH v2 2/2] ath10k: add memory dump debugfs interface

[Kalle Valo]

From: Yanbo Li <yanbol@qti.qualcomm.com>

Add mem_val debugfs file for dumping the firmware (target) memory and also for
writing to the memory. The firmware memory is accessed through one file which
uses position of the file as the firmware memory address. For example, with dd
use skip parameter for the address.

Beucase target memory width is 32 bits it's strongly recommended to use
blocksize divisable with 4 when using this interface. For example, when using
dd use bs=4 to set the block size to 4 and remember to divide both count and
skip values with four.

To read 4 kB chunk from address 0x400000:

dd if=mem_value bs=4 count=1024 skip=1048576 | xxd -g1

To write value 0x01020304 to address 0x400400:

echo 0x01020304 | xxd -r | dd of=mem_value bs=4 seek=1048832

To read 4 KB chunk of memory and then write back after edit:

dd if=mem_value of=tmp.bin bs=4 count=1024 skip=1048576
emacs tmp.bin
dd if=tmp.bin of=mem_value bs=4 count=1024 seek=1048576

Signed-off-by: Yanbo Li <yanbol@qti.qualcomm.com>
Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/debug.c |   86 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/hif.h   |   11 ++++
 drivers/net/wireless/ath/ath10k/pci.c   |    1 
 3 files changed, 98 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index b500cd619c05..3988220e3e66 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1024,6 +1024,89 @@ static const struct file_operations fops_reg_value = {
 	.llseek = default_llseek,
 };
 
+static ssize_t ath10k_mem_value_read(struct file *file,
+				     char __user *user_buf,
+				     size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u8 *buf;
+	int ret;
+
+	if (*ppos < 0)
+		return -EINVAL;
+
+	if (!count)
+		return 0;
+
+	buf = vmalloc(count);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ath10k_hif_diag_read(ar, *ppos, buf, count);
+	if (ret) {
+		ath10k_warn(ar, "failed to read address 0x%08x via diagnose window from debugfs: %d\n",
+			    (u32)(*ppos), ret);
+		goto read_err;
+	}
+
+	ret = copy_to_user(user_buf, buf, count);
+	if (ret == count)
+		return -EFAULT;
+
+	count -= ret;
+	*ppos += count;
+	ret = count;
+
+read_err:
+	vfree(buf);
+	return ret;
+}
+
+static ssize_t ath10k_mem_value_write(struct file *file,
+				      const char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	u8 *buf;
+	int ret;
+
+	if (*ppos < 0)
+		return -EINVAL;
+
+	if (!count)
+		return 0;
+
+	buf = vmalloc(count);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = copy_from_user(buf, user_buf, count);
+	if (ret)
+		goto err_free_copy;
+
+	ret = ath10k_hif_diag_write(ar, *ppos, buf, count);
+	if (ret) {
+		ath10k_warn(ar, "failed to write address 0x%08x via diagnose window from debugfs: %d\n",
+			    (u32)(*ppos), ret);
+		goto err_free_copy;
+	}
+
+	*ppos += count;
+	ret = count;
+
+err_free_copy:
+	vfree(buf);
+	return ret;
+}
+
+static const struct file_operations fops_mem_value = {
+	.read = ath10k_mem_value_read,
+	.write = ath10k_mem_value_write,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 static int ath10k_debug_htt_stats_req(struct ath10k *ar)
 {
 	u64 cookie;
@@ -1731,6 +1814,9 @@ int ath10k_debug_register(struct ath10k *ar)
 	debugfs_create_file("reg_value", S_IRUSR | S_IWUSR,
 			    ar->debug.debugfs_phy, ar, &fops_reg_value);
 
+	debugfs_create_file("mem_value", S_IRUSR | S_IWUSR,
+			    ar->debug.debugfs_phy, ar, &fops_mem_value);
+
 	debugfs_create_file("chip_id", S_IRUSR, ar->debug.debugfs_phy,
 			    ar, &fops_chip_id);
 
diff --git a/drivers/net/wireless/ath/ath10k/hif.h b/drivers/net/wireless/ath/ath10k/hif.h
index bad071906540..6ac552304546 100644
--- a/drivers/net/wireless/ath/ath10k/hif.h
+++ b/drivers/net/wireless/ath/ath10k/hif.h
@@ -48,6 +48,8 @@ struct ath10k_hif_ops {
 	int (*diag_read)(struct ath10k *ar, u32 address, void *buf,
 			 size_t buf_len);
 
+	int (*diag_write)(struct ath10k *ar, u32 address, const void *data,
+			  int nbytes);
 	/*
 	 * API to handle HIF-specific BMI message exchanges, this API is
 	 * synchronous and only allowed to be called from a context that
@@ -113,6 +115,15 @@ static inline int ath10k_hif_diag_read(struct ath10k *ar, u32 address, void *buf
 	return ar->hif.ops->diag_read(ar, address, buf, buf_len);
 }
 
+static inline int ath10k_hif_diag_write(struct ath10k *ar, u32 address,
+					const void *data, int nbytes)
+{
+	if (!ar->hif.ops->diag_write)
+		return -EOPNOTSUPP;
+
+	return ar->hif.ops->diag_write(ar, address, data, nbytes);
+}
+
 static inline int ath10k_hif_exchange_bmi_msg(struct ath10k *ar,
 					      void *request, u32 request_len,
 					      void *response, u32 *response_len)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 328ab6c5d053..0816098af578 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1988,6 +1988,7 @@ static int ath10k_pci_hif_resume(struct ath10k *ar)
 static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
 	.tx_sg			= ath10k_pci_hif_tx_sg,
 	.diag_read		= ath10k_pci_hif_diag_read,
+	.diag_write		= ath10k_pci_diag_write_mem,
 	.exchange_bmi_msg	= ath10k_pci_hif_exchange_bmi_msg,
 	.start			= ath10k_pci_hif_start,
 	.stop			= ath10k_pci_hif_stop,

Re: [PATCH v2 2/2] ath10k: add memory dump debugfs interface

[Kalle Valo]

Kalle Valo <kvalo@qca.qualcomm.com> writes:

> +	ret = copy_from_user(buf, user_buf, count);
> +	if (ret)
> +		goto err_free_copy;
> +
> +	ret = ath10k_hif_diag_write(ar, *ppos, buf, count);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to write address 0x%08x via diagnose window from debugfs: %d\n",
> +			    (u32)(*ppos), ret);
> +		goto err_free_copy;
> +	}
> +
> +	*ppos += count;
> +	ret = count;
> +
> +err_free_copy:
> +	vfree(buf);
> +	return ret;

This introduces a new smatch warning:

drivers/net/wireless/ath/ath10k/debug.c:1100 ath10k_mem_value_write()
warn: maybe return -EFAULT instead of the bytes remaining?

I think it should be like this:

ret = copy_from_user(buf, user_buf, count);
if (ret) {
        ret = -EFAULT;
	goto err_free_copy;
}

Comments?

-- 
Kalle Valo

Re: [PATCH v2 1/2] ath10k: add register access debugfs interface

[Kalle Valo]

Kalle Valo <kvalo@qca.qualcomm.com> writes:

> +static ssize_t ath10k_reg_value_read(struct file *file,
> +				     char __user *user_buf,
> +				     size_t count, loff_t *ppos)
> +{
> +	struct ath10k *ar = file->private_data;
> +	u8 buf[48];
> +	unsigned int len;
> +	u32 reg_addr, reg_val;
> +
> +	spin_lock_bh(&ar->data_lock);
> +	reg_addr = ar->debug.reg_addr;
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	reg_val = ath10k_hif_read32(ar, reg_addr);
> +	len = scnprintf(buf, sizeof(buf), "0x%08x:0x%08x\n", reg_addr, reg_val);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}

I just realised that we need to check ar->state to make sure that
firmware is running. Because of that I'll need to change the data_lock
to conf_mutex as well.

> +static ssize_t ath10k_reg_value_write(struct file *file,
> +				      const char __user *user_buf,
> +				      size_t count, loff_t *ppos)
> +{
> +	struct ath10k *ar = file->private_data;
> +	u32 reg_addr, reg_val;
> +	int ret;
> +
> +	spin_lock_bh(&ar->data_lock);
> +	reg_addr = ar->debug.reg_addr;
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	ret = kstrtou32_from_user(user_buf, count, 0, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ath10k_hif_write32(ar, reg_addr, reg_val);
> +
> +	return count;
> +}

And same here as well. I'll send v3.

-- 
Kalle Valo

Re: [PATCH v2 2/2] ath10k: add memory dump debugfs interface

[Kalle Valo]

Kalle Valo <kvalo@qca.qualcomm.com> writes:

> +static ssize_t ath10k_mem_value_read(struct file *file,
> +				     char __user *user_buf,
> +				     size_t count, loff_t *ppos)
> +{
> +	struct ath10k *ar = file->private_data;
> +	u8 *buf;
> +	int ret;
> +
> +	if (*ppos < 0)
> +		return -EINVAL;
> +
> +	if (!count)
> +		return 0;
> +
> +	buf = vmalloc(count);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = ath10k_hif_diag_read(ar, *ppos, buf, count);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to read address 0x%08x via diagnose window from debugfs: %d\n",
> +			    (u32)(*ppos), ret);
> +		goto read_err;
> +	}
> +

[...]

> +static ssize_t ath10k_mem_value_write(struct file *file,
> +				      const char __user *user_buf,
> +				      size_t count, loff_t *ppos)
> +{
> +	struct ath10k *ar = file->private_data;
> +	u8 *buf;
> +	int ret;
> +
> +	if (*ppos < 0)
> +		return -EINVAL;
> +
> +	if (!count)
> +		return 0;
> +
> +	buf = vmalloc(count);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = copy_from_user(buf, user_buf, count);
> +	if (ret)
> +		goto err_free_copy;
> +
> +	ret = ath10k_hif_diag_write(ar, *ppos, buf, count);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to write address 0x%08x via diagnose window from debugfs: %d\n",
> +			    (u32)(*ppos), ret);
> +		goto err_free_copy;
> +	}

In these two functions we also need to check ar->state.

-- 
Kalle Valo

Re: [PATCH v2 1/2] ath10k: add register access debugfs interface

[Michal Kazior]

On 25 November 2014 at 09:39, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Kalle Valo <kvalo@qca.qualcomm.com> writes:
>
>> +static ssize_t ath10k_reg_value_read(struct file *file,
>> +                                  char __user *user_buf,
>> +                                  size_t count, loff_t *ppos)
>> +{
>> +     struct ath10k *ar = file->private_data;
>> +     u8 buf[48];
>> +     unsigned int len;
>> +     u32 reg_addr, reg_val;
>> +
>> +     spin_lock_bh(&ar->data_lock);
>> +     reg_addr = ar->debug.reg_addr;
>> +     spin_unlock_bh(&ar->data_lock);
>> +
>> +     reg_val = ath10k_hif_read32(ar, reg_addr);
>> +     len = scnprintf(buf, sizeof(buf), "0x%08x:0x%08x\n", reg_addr, reg_val);
>> +
>> +     return simple_read_from_buffer(user_buf, count, ppos, buf, len);
>> +}
>
> I just realised that we need to check ar->state to make sure that
> firmware is running. Because of that I'll need to change the data_lock
> to conf_mutex as well.

Hmm.. It could actually work without firmware running. It just reads MMIO.

The problem I see now is the target may not be awake (Bartosz recently
fixed suspend bug by moving ath10k_pci_wake/sleep back to
hif_power_up) and accessing target registers without it being fully
awake can lead to host memory corruption. I guess ar->state will do
for now because getting this to work otherwise is going to be a little
tricky.


Michał

Re: [PATCH v2 2/2] ath10k: add memory dump debugfs interface

[Michal Kazior]

On 25 November 2014 at 09:41, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Kalle Valo <kvalo@qca.qualcomm.com> writes:
>
>> +static ssize_t ath10k_mem_value_read(struct file *file,
>> +                                  char __user *user_buf,
>> +                                  size_t count, loff_t *ppos)
>> +{
[...]
>> +static ssize_t ath10k_mem_value_write(struct file *file,
>> +                                   const char __user *user_buf,
>> +                                   size_t count, loff_t *ppos)
[...]
> In these two functions we also need to check ar->state.

It should work but the pci_wake (I've mentioned in the other mail) is
tricky though.


Michał