Marty Faltesek <mfaltesek@xxxxxxxxxx> writes:
> Caching calibration data allows it to be accessed when the
> device is not active.
>
> Signed-off-by: Marty Faltesek <mfaltesek@xxxxxxxxxx>
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.
--
Kalle Valo
