On Mon, May 6, 2019 at 7:26 AM Govind Singh <govinds@xxxxxxxxxxxxxx> wrote:
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1586,6 +1587,72 @@ static int ath10k_hw_power_off(struct ath10k *ar)
> return ret;
> }
>
> +static void ath10k_msa_dump_memory(struct ath10k *ar,
> + struct ath10k_fw_crash_data *crash_data)
> +{
> + struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
> + const struct ath10k_hw_mem_layout *mem_layout;
> + const struct ath10k_mem_region *current_region;
> + struct ath10k_dump_ram_data_hdr *hdr;
> + size_t buf_len;
> + u8 *buf;
> +
> + lockdep_assert_held(&ar->data_lock);
I believe that's the wrong lock now. See below.
> +
> + if (!crash_data && !crash_data->ramdump_buf)
> + return;
> +
> + mem_layout = ath10k_coredump_get_mem_layout(ar);
> + if (!mem_layout)
> + return;
> +
> + current_region = &mem_layout->region_table.regions[0];
> +
> + buf = crash_data->ramdump_buf;
> + buf_len = crash_data->ramdump_buf_len;
> + memset(buf, 0, buf_len);
> +
> + /* Reserve space for the header. */
> + hdr = (void *)buf;
> + buf += sizeof(*hdr);
> + buf_len -= sizeof(*hdr);
> +
> + hdr->region_type = cpu_to_le32(current_region->type);
> + hdr->start = cpu_to_le32(ar_snoc->qmi->msa_va);
> + hdr->length = cpu_to_le32(ar_snoc->qmi->msa_mem_size);
> +
> + if (current_region->len < ar_snoc->qmi->msa_mem_size) {
> + memcpy(buf, ar_snoc->qmi->msa_va, current_region->len);
> + ath10k_warn(ar, "msa dump length is less than msa size %x, %x\n",
> + current_region->len, ar_snoc->qmi->msa_mem_size);
> + } else {
> + memcpy(buf, ar_snoc->qmi->msa_va, ar_snoc->qmi->msa_mem_size);
> + }
> +}
> +
> +void ath10k_snoc_fw_crashed_dump(struct ath10k *ar)
> +{
> + struct ath10k_fw_crash_data *crash_data;
> + char guid[UUID_STRING_LEN + 1];
> +
> + spin_lock_bh(&ar->data_lock);
> +
> + ar->stats.fw_crash_counter++;
> +
> + crash_data = ath10k_coredump_new(ar);
This will (for good reason) spit a lockdep warning after this, I think:
38faed150438 ath10k: perform crash dump collection in workqueue
You need to hold 'dump_mutex' now. I believe you only need to hold
'data_lock' for the sake of the crash counter.
Brian
> +
> + if (crash_data)
> + scnprintf(guid, sizeof(guid), "%pUl", &crash_data->guid);
> + else
> + scnprintf(guid, sizeof(guid), "n/a");
> +
> + ath10k_err(ar, "firmware crashed! (guid %s)\n", guid);
> + ath10k_print_driver_info(ar);
> + ath10k_msa_dump_memory(ar, crash_data);
> +
> + spin_unlock_bh(&ar->data_lock);
> +}
