2016-10-05 20:24:22

by Marty Faltesek

[permalink] [raw]
Subject: [PATCH v2] ath10k: cache calibration data when the core is stopped

Caching calibration data allows it to be accessed when the
device is not active.

---
drivers/net/wireless/ath/ath10k/core.h | 1 +
drivers/net/wireless/ath/ath10k/debug.c | 72 ++++++++++++++-------------------
2 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 6e5aa2d..7274ebe 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -452,6 +452,7 @@ struct ath10k_debug {
u32 nf_cal_period;

struct ath10k_fw_crash_data *fw_crash_data;
+ void *cal_data;
};

enum ath10k_state {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 832da6e..668074c 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1451,75 +1451,58 @@ 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(&ar->conf_mutex);
-
- if (ar->state != ATH10K_STATE_ON &&
- ar->state != ATH10K_STATE_UTF) {
- ret = -ENETDOWN;
- goto err;
- }
-
- buf = vmalloc(ar->hw_params.cal_data_len);
- if (!buf) {
- ret = -ENOMEM;
- goto err;
- }
-
hi_addr = host_interest_item_address(HI_ITEM(hi_board_data));

ret = ath10k_hif_diag_read(ar, hi_addr, &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(&ar->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(&ar->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(&ar->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 ath10k *ar = file->private_data;
- void *buf = file->private_data;

return simple_read_from_buffer(user_buf, count, ppos,
- buf, ar->hw_params.cal_data_len);
-}
-
-static int ath10k_debug_cal_data_release(struct inode *inode,
- struct file *file)
-{
- vfree(file->private_data);
-
- return 0;
+ ar->debug.cal_data,
+ ar->hw_params.cal_data_len);
}

static ssize_t ath10k_write_ani_enable(struct file *file,
@@ -1580,7 +1563,6 @@ static const struct file_operations fops_ani_enable = {
static const struct file_operations fops_cal_data = {
.open = ath10k_debug_cal_data_open,
.read = ath10k_debug_cal_data_read,
- .release = ath10k_debug_cal_data_release,
.owner = THIS_MODULE,
.llseek = default_llseek,
};
@@ -1932,6 +1914,8 @@ void ath10k_debug_stop(struct ath10k *ar)
{
lockdep_assert_held(&ar->conf_mutex);

+ ath10k_debug_cal_data_fetch(ar);
+
/* Must not use _sync to avoid deadlock, we do that in
* ath10k_debug_destroy(). The check for htt_stats_mask is to avoid
* warning from del_timer(). */
@@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
if (!ar->debug.fw_crash_data)
return -ENOMEM;
-
+ ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
+ if (!ar->debug.cal_data)
+ return -ENOMEM;
INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
@@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
void ath10k_debug_destroy(struct ath10k *ar)
{
vfree(ar->debug.fw_crash_data);
+ vfree(ar->debug.cal_data);
ar->debug.fw_crash_data = NULL;
+ ar->debug.cal_data = NULL;

ath10k_debug_fw_stats_reset(ar);

--
2.8.0.rc3.226.g39d4020


2016-10-10 14:57:16

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

Marty Faltesek <[email protected]> writes:

> Caching calibration data allows it to be accessed when the
> device is not active.
>
> ---

Signed-off-by missing. Can you send it as a reply to this message and
I'll add it to v3?

> drivers/net/wireless/ath/ath10k/core.h | 1 +
> drivers/net/wireless/ath/ath10k/debug.c | 72 ++++++++++++++-------------=
------
> 2 files changed, 31 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireles=
s/ath/ath10k/core.h
> index 6e5aa2d..7274ebe 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -452,6 +452,7 @@ struct ath10k_debug {
> u32 nf_cal_period;
> =20
> struct ath10k_fw_crash_data *fw_crash_data;
> + void *cal_data;

To properly document locking I'll move this under the "protected by
conf_mutex" comment.

> -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 =3D inode->i_private;
> - void *buf;
> u32 hi_addr;
> __le32 addr;
> int ret;
> =20
> - mutex_lock(&ar->conf_mutex);

I'll add lockdep_assert_held() here.

> 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 ath10k *ar =3D file->private_data;
> - void *buf =3D file->private_data;
> =20
> return simple_read_from_buffer(user_buf, count, ppos,
> - buf, ar->hw_params.cal_data_len);
> -}

I'll add locking to this function.

> @@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
> ar->debug.fw_crash_data =3D vzalloc(sizeof(*ar->debug.fw_crash_data));
> if (!ar->debug.fw_crash_data)
> return -ENOMEM;
> -
> + ar->debug.cal_data =3D vzalloc(ar->hw_params.cal_data_len);
> + if (!ar->debug.cal_data)
> + return -ENOMEM;
> INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
> INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
> INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
> @@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
> void ath10k_debug_destroy(struct ath10k *ar)
> {
> vfree(ar->debug.fw_crash_data);
> + vfree(ar->debug.cal_data);
> ar->debug.fw_crash_data =3D NULL;
> + ar->debug.cal_data =3D NULL;

Actually I gave you a bad advice, this won't work as
ar->hw_params.cal_data_len is not yet initialised, we initialise it only
after we have probed the capabilities during the first firmware boot. I
changed the allocation to a fixed length buffer for now, it's only few
kBs virtual memory anyway so it shouldn't matter to anyone.

I have now v3 ready and tested. I'll just need your Signed-off-by and I
can then submit it.

--=20
Kalle Valo=

2016-10-10 15:01:24

by Marty Faltesek

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

ath10k: cache calibration data when the core is stopped

Caching calibration data allows it to be accessed when the
device is not active.

Signed-off-by: Marty Faltesek <[email protected]>

On Mon, Oct 10, 2016 at 10:54 AM, Valo, Kalle <[email protected]> wrote:
> Marty Faltesek <[email protected]> writes:
>
>> Caching calibration data allows it to be accessed when the
>> device is not active.
>>
>> ---
>
> Signed-off-by missing. Can you send it as a reply to this message and
> I'll add it to v3?
>
>> drivers/net/wireless/ath/ath10k/core.h | 1 +
>> drivers/net/wireless/ath/ath10k/debug.c | 72 ++++++++++++++-------------------
>> 2 files changed, 31 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index 6e5aa2d..7274ebe 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -452,6 +452,7 @@ struct ath10k_debug {
>> u32 nf_cal_period;
>>
>> struct ath10k_fw_crash_data *fw_crash_data;
>> + void *cal_data;
>
> To properly document locking I'll move this under the "protected by
> conf_mutex" comment.
>
>> -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(&ar->conf_mutex);
>
> I'll add lockdep_assert_held() here.
>
>> 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 ath10k *ar = file->private_data;
>> - void *buf = file->private_data;
>>
>> return simple_read_from_buffer(user_buf, count, ppos,
>> - buf, ar->hw_params.cal_data_len);
>> -}
>
> I'll add locking to this function.
>
>> @@ -2343,7 +2327,9 @@ int ath10k_debug_create(struct ath10k *ar)
>> ar->debug.fw_crash_data = vzalloc(sizeof(*ar->debug.fw_crash_data));
>> if (!ar->debug.fw_crash_data)
>> return -ENOMEM;
>> -
>> + ar->debug.cal_data = vzalloc(ar->hw_params.cal_data_len);
>> + if (!ar->debug.cal_data)
>> + return -ENOMEM;
>> INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
>> INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
>> INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
>> @@ -2355,7 +2341,9 @@ int ath10k_debug_create(struct ath10k *ar)
>> void ath10k_debug_destroy(struct ath10k *ar)
>> {
>> vfree(ar->debug.fw_crash_data);
>> + vfree(ar->debug.cal_data);
>> ar->debug.fw_crash_data = NULL;
>> + ar->debug.cal_data = NULL;
>
> Actually I gave you a bad advice, this won't work as
> ar->hw_params.cal_data_len is not yet initialised, we initialise it only
> after we have probed the capabilities during the first firmware boot. I
> changed the allocation to a fixed length buffer for now, it's only few
> kBs virtual memory anyway so it shouldn't matter to anyone.
>
> I have now v3 ready and tested. I'll just need your Signed-off-by and I
> can then submit it.
>
> --
> Kalle Valo

2016-10-10 16:00:33

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: cache calibration data when the core is stopped

Marty Faltesek <[email protected]> writes:

> ath10k: cache calibration data when the core is stopped
>
> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek <[email protected]>

Thanks, I'll now send v3.

--=20
Kalle Valo=