| Message ID | 1473801118-103112-1-git-send-email-mfaltesek@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Marty Faltesek <mfaltesek@google.com> writes: > Caching calibration data allows it to be accessed when the > device is not active. > > Signed-off-by: Marty Faltesek <mfaltesek@google.com> No comma in the title, please. What tree did you use as the baseline? This doesn't seem to apply to ath.git: Applying: ath10k: cache calibration data when the core is stopped. fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/core.c). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 ath10k: cache calibration data when the core is stopped. > --- a/drivers/net/wireless/ath/ath10k/core.c > +++ b/drivers/net/wireless/ath/ath10k/core.c > @@ -1227,6 +1227,42 @@ success: > return 0; > } > > +int > +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) > +{ > + u32 hi_addr; > + __le32 addr; > + int ret; I think this function should be in debug.c. That way the code is not wasting memory if DEBUGFS is disabled. > + vfree(*buf); > + *buf = vmalloc(QCA988X_CAL_DATA_LEN); > + if (!*buf) { > + return -EAGAIN; > + } Is it really necessary to allocate memory every time? What if allocate it only once when module is loaded, just like with ar->debug.fw_crash_data? Also please note that this patch (which I'm queuing to 4.9) touches the same area: ath10k: fix debug cal data file https://patchwork.kernel.org/patch/9340953/ > + 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); > + } else { > + ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), *buf, > + QCA988X_CAL_DATA_LEN); > + if (ret) { > + ath10k_warn(ar, "failed to read calibration data: %d\n", ret); > + } > + } We try to avoid using else branches so that only error handling is indented and the main code flow is not. > + /* > + * We are up now, so no need to cache calibration data. > + */ > + vfree(ar->cal_data); > + ar->cal_data = NULL; Indentation looks wrongs here. > @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar) > lockdep_assert_held(&ar->conf_mutex); > ath10k_debug_stop(ar); > > + /* > + * Cache caclibration data while stopped. > + */ > + ath10k_cal_data_alloc(ar, &ar->cal_data); I like the idea that the calibration data is copied during stop(), but can you do this from debug.c? For example, add ath10k_debug_stop() and call it from there? I don't think any of this should be in core.c.
Marty Faltesek <mfaltesek@google.com> writes: > Caching calibration data allows it to be accessed when the > device is not active. > > Signed-off-by: Marty Faltesek <mfaltesek@google.com> [...] > +int > +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) > +{ [...] > + ath10k_cal_data_alloc(ar, &ar->cal_data); [...] > + ret = ath10k_cal_data_alloc(ar, &file->private_data); Pointer to pointer parameters can be a source of problems and if we could use one shared buffer for both of these cases when it would simplify the code and we would need the buf parameter at all.
On 13 September 2016 at 23:11, Marty Faltesek <mfaltesek@google.com> wrote: [...] > +int > +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) > +{ > + u32 hi_addr; > + __le32 addr; > + int ret; > + > + vfree(*buf); > + *buf = vmalloc(QCA988X_CAL_DATA_LEN); Shouldn't you use ar->hw_params to get hw-specific caldata length? [...] > @@ -1714,6 +1750,12 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode) > > INIT_LIST_HEAD(&ar->arvifs); > > + /* > + * We are up now, so no need to cache calibration data. > + */ The comment style is: /* comment */ If it's multi-line it should be: /* comment * comment */ Ditto for other instances. [...] > @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar) > lockdep_assert_held(&ar->conf_mutex); > ath10k_debug_stop(ar); > > + /* > + * Cache caclibration data while stopped. typo. "calibration" Michał
On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: > Marty Faltesek <mfaltesek@google.com> writes: > >> Caching calibration data allows it to be accessed when the >> device is not active. >> >> Signed-off-by: Marty Faltesek <mfaltesek@google.com> > > No comma in the title, please. > > What tree did you use as the baseline? This doesn't seem to apply to > ath.git: We use backports 20160122 which has not been updated since earlier this year. I can forward port it to your tree, and make sure it builds but won't be able to test it. Will that be OK? > > Applying: ath10k: cache calibration data when the core is stopped. > fatal: sha1 information is lacking or useless (drivers/net/wireless/ath/ath10k/core.c). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 ath10k: cache calibration data when the core is stopped. > >> --- a/drivers/net/wireless/ath/ath10k/core.c >> +++ b/drivers/net/wireless/ath/ath10k/core.c >> @@ -1227,6 +1227,42 @@ success: >> return 0; >> } >> >> +int >> +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) >> +{ >> + u32 hi_addr; >> + __le32 addr; >> + int ret; > > I think this function should be in debug.c. That way the code is not > wasting memory if DEBUGFS is disabled. ok. > >> + vfree(*buf); >> + *buf = vmalloc(QCA988X_CAL_DATA_LEN); >> + if (!*buf) { >> + return -EAGAIN; >> + } > > Is it really necessary to allocate memory every time? What if allocate > it only once when module is loaded, just like with > ar->debug.fw_crash_data? yup, good suggestion. > > Also please note that this patch (which I'm queuing to 4.9) touches the > same area: > > ath10k: fix debug cal data file > > https://patchwork.kernel.org/patch/9340953/ I've modified this too, and this won't be necessary, so can you drop it? If not, let me know and I'll pull it in and make sure I'm based off it too.
On Mon, Oct 3, 2016 at 4:02 AM, Michal Kazior <michal.kazior@tieto.com> wrote: > On 13 September 2016 at 23:11, Marty Faltesek <mfaltesek@google.com> wrote: > [...] >> +int >> +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) >> +{ >> + u32 hi_addr; >> + __le32 addr; >> + int ret; >> + >> + vfree(*buf); >> + *buf = vmalloc(QCA988X_CAL_DATA_LEN); > > Shouldn't you use ar->hw_params to get hw-specific caldata length? yup, it was because we were based on backports from earlier this year. > > > [...] >> @@ -1714,6 +1750,12 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode) >> >> INIT_LIST_HEAD(&ar->arvifs); >> >> + /* >> + * We are up now, so no need to cache calibration data. >> + */ > > The comment style is: > > /* comment */ > > If it's multi-line it should be: > > /* comment > * comment > */ > > Ditto for other instances. fixed, thanks. > > > [...] >> @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar) >> lockdep_assert_held(&ar->conf_mutex); >> ath10k_debug_stop(ar); >> >> + /* >> + * Cache caclibration data while stopped. > > typo. "calibration" > > > Michał fixed thanks.
Marty Faltesek <mfaltesek@google.com> writes: > On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: >> Marty Faltesek <mfaltesek@google.com> writes: >> >>> Caching calibration data allows it to be accessed when the >>> device is not active. >>> >>> Signed-off-by: Marty Faltesek <mfaltesek@google.com> >> >> No comma in the title, please. >> >> What tree did you use as the baseline? This doesn't seem to apply to >> ath.git: > > We use backports 20160122 which has not been updated since earlier this year. > I can forward port it to your tree, and make sure > it builds but won't be able to test it. Will that be OK? Sure, I can test it. >> Also please note that this patch (which I'm queuing to 4.9) touches the >> same area: >> >> ath10k: fix debug cal data file >> >> https://patchwork.kernel.org/patch/9340953/ > > I've modified this too, and this won't be necessary, so can you drop > it? If not, let me know and I'll pull it in and make sure I'm based > off it too. Actually I was first planning to push 9340953 to 4.9 and take your patch to 4.10. But your patch is a cleaner approach to this and maybe I should push that to 4.9 instead? Need to think a bit more.
OK the v2 patch is not based on 9340953. If you queue this in 4.10 as planned then just let me know and I'll rev v3 based on top of 9340953. On Thu, Oct 6, 2016 at 3:40 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: > Marty Faltesek <mfaltesek@google.com> writes: > >> On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote: >>> Marty Faltesek <mfaltesek@google.com> writes: >>> >>>> Caching calibration data allows it to be accessed when the >>>> device is not active. >>>> >>>> Signed-off-by: Marty Faltesek <mfaltesek@google.com> >>> >>> No comma in the title, please. >>> >>> What tree did you use as the baseline? This doesn't seem to apply to >>> ath.git: >> >> We use backports 20160122 which has not been updated since earlier this year. >> I can forward port it to your tree, and make sure >> it builds but won't be able to test it. Will that be OK? > > Sure, I can test it. > >>> Also please note that this patch (which I'm queuing to 4.9) touches the >>> same area: >>> >>> ath10k: fix debug cal data file >>> >>> https://patchwork.kernel.org/patch/9340953/ >> >> I've modified this too, and this won't be necessary, so can you drop >> it? If not, let me know and I'll pull it in and make sure I'm based >> off it too. > > Actually I was first planning to push 9340953 to 4.9 and take your patch > to 4.10. But your patch is a cleaner approach to this and maybe I should > push that to 4.9 instead? Need to think a bit more. > > -- > Kalle Valo
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index c0b9797..c99ad9e 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1227,6 +1227,42 @@ success: return 0; } +int +ath10k_cal_data_alloc(struct ath10k *ar, void **buf) +{ + u32 hi_addr; + __le32 addr; + int ret; + + vfree(*buf); + *buf = vmalloc(QCA988X_CAL_DATA_LEN); + if (!*buf) { + return -EAGAIN; + } + + 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); + } else { + ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), *buf, + QCA988X_CAL_DATA_LEN); + if (ret) { + ath10k_warn(ar, "failed to read calibration data: %d\n", ret); + } + } + + if (ret) { + vfree(*buf); + *buf = NULL; + } + + return ret; +} + + static int ath10k_download_cal_data(struct ath10k *ar) { int ret; @@ -1714,6 +1750,12 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode) INIT_LIST_HEAD(&ar->arvifs); + /* + * We are up now, so no need to cache calibration data. + */ + vfree(ar->cal_data); + ar->cal_data = NULL; + return 0; err_hif_stop: @@ -1757,6 +1799,11 @@ void ath10k_core_stop(struct ath10k *ar) lockdep_assert_held(&ar->conf_mutex); ath10k_debug_stop(ar); + /* + * Cache caclibration data while stopped. + */ + ath10k_cal_data_alloc(ar, &ar->cal_data); + /* try to suspend target */ if (ar->state != ATH10K_STATE_RESTARTING && ar->state != ATH10K_STATE_UTF) diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 4d3f002..cce61c1 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -635,6 +635,8 @@ struct ath10k { struct ath10k_htc htc; struct ath10k_htt htt; + void *cal_data; + struct ath10k_hw_params { u32 id; u16 dev_id; diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c index 8b01e3e..7de0eb4 100644 --- a/drivers/net/wireless/ath/ath10k/debug.c +++ b/drivers/net/wireless/ath/ath10k/debug.c @@ -1427,53 +1427,26 @@ static const struct file_operations fops_fw_dbglog = { static int ath10k_debug_cal_data_open(struct inode *inode, struct file *file) { 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(QCA988X_CAL_DATA_LEN); - if (!buf) { - ret = -ENOMEM; - goto err; + ar->state != ATH10K_STATE_UTF && ! ar->cal_data) { + mutex_unlock(&ar->conf_mutex); + return (-ENETDOWN); } - 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; - } - - ret = ath10k_hif_diag_read(ar, le32_to_cpu(addr), buf, - QCA988X_CAL_DATA_LEN); - if (ret) { - ath10k_warn(ar, "failed to read calibration data: %d\n", ret); - goto err_vfree; + if (ar->cal_data) { + file->private_data = ar->cal_data; + mutex_unlock(&ar->conf_mutex); + return 0; } - file->private_data = buf; - - mutex_unlock(&ar->conf_mutex); - - return 0; - -err_vfree: - vfree(buf); - -err: + ret = ath10k_cal_data_alloc(ar, &file->private_data); mutex_unlock(&ar->conf_mutex); - return ret; + return (ret); } static ssize_t ath10k_debug_cal_data_read(struct file *file, @@ -1489,7 +1462,11 @@ static ssize_t ath10k_debug_cal_data_read(struct file *file, static int ath10k_debug_cal_data_release(struct inode *inode, struct file *file) { - vfree(file->private_data); + struct ath10k *ar = inode->i_private; + + /* Free if it's not the cached version */ + if (ar->cal_data != file->private_data) + vfree(file->private_data); return 0; } diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h index 3900934..1ae757c 100644 --- a/drivers/net/wireless/ath/ath10k/debug.h +++ b/drivers/net/wireless/ath/ath10k/debug.h @@ -59,6 +59,7 @@ enum ath10k_dbg_aggr_mode { #define ATH10K_FW_STATS_BUF_SIZE (1024*1024) extern unsigned int ath10k_debug_mask; +extern int ath10k_cal_data_alloc(struct ath10k *ar, void **buf); __printf(2, 3) void ath10k_info(struct ath10k *ar, const char *fmt, ...); __printf(2, 3) void ath10k_err(struct ath10k *ar, const char *fmt, ...); diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index eb36859..88fb27f 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -6924,6 +6924,7 @@ struct ath10k *ath10k_mac_create(size_t priv_size) void ath10k_mac_destroy(struct ath10k *ar) { + vfree(ar->cal_data); ieee80211_free_hw(ar->hw); }
Caching calibration data allows it to be accessed when the device is not active. Signed-off-by: Marty Faltesek <mfaltesek@google.com> --- drivers/net/wireless/ath/ath10k/core.c | 47 ++++++++++++++++++++++++++++++ drivers/net/wireless/ath/ath10k/core.h | 2 ++ drivers/net/wireless/ath/ath10k/debug.c | 51 +++++++++------------------------ drivers/net/wireless/ath/ath10k/debug.h | 1 + drivers/net/wireless/ath/ath10k/mac.c | 1 + 5 files changed, 65 insertions(+), 37 deletions(-)