On 7/17/2024 4:42 AM, Jeff Johnson wrote:
> On 7/15/2024 11:39 PM, Sowmiya Sree Elavalagan wrote:
>> In case of firmware assert snapshot of firmware memory is essential for
>> debugging. Add firmware coredump collection support for PCI bus.
>> Collect RDDM and firmware paging dumps from MHI and pack them in TLV
>> format and also pack various memory shared during QMI phase in separate
>> TLVs. Add necessary header and share the dumps to user space using dev
>> coredump framework. Coredump collection is disabled by default and can
>> be enabled using menuconfig. Dump collected for a radio is 55 MB
>> approximately.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
>> Tested-on: WCN7850 WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>>
>> Signed-off-by: Sowmiya Sree Elavalagan <quic_ssreeela@xxxxxxxxxxx>
>> ---
>> v4:
>> - Fixed Kasan warning vmalloc-out-of-bounds in ath12k_pci_coredump_download
>> - Rebased on ToT
>> v3:
>> - Fixed SPDX comment style for coredump.c file
>> Changed Kconfig description.
>> v2:
>> - Fixed errors shown by ath12k-check
>> ---
> ...
>> + dump_tlv = buf;
>> + dump_tlv->type = cpu_to_le32(FW_CRASH_DUMP_RDDM_DATA);
>> + dump_tlv->tlv_len = cpu_to_le32(dump_seg_sz[FW_CRASH_DUMP_RDDM_DATA]);
>> + buf += COREDUMP_TLV_HDR_SIZE;
>> +
>> + /* append all segments together as they are all part of a single contiguous
>> + * block of memory
>> + */
>> + for (i = 0; i < rddm_img->entries; i++) {
>> + if (!rddm_img->mhi_buf[i].buf)
>> + continue;
>> +
>> + memcpy_fromio(buf, (void const __iomem *)rddm_img->mhi_buf[i].buf,
>> + rddm_img->mhi_buf[i].len);
>> + buf += rddm_img->mhi_buf[i].len;
>> + }
>> +
>> + mem_idx = FW_CRASH_DUMP_REMOTE_MEM_DATA;
>> + for (; mem_idx < FW_CRASH_DUMP_TYPE_MAX; mem_idx++) {
>> + if (!dump_seg_sz[i] || mem_idx == FW_CRASH_DUMP_NONE)
>
> this looks really strange testing dump_seg_size[i]
>
> the first time through the loop i will be set to the value of
> rddm_img->entries since that is the value it will have from:
> for (i = 0; i < rddm_img->entries; i++) {
>
> but subsequent times through the loop i will be set to the value of
> ab->qmi.mem_seg_count since that is the value it will have from:
> for (i = 0; i < ab->qmi.mem_seg_count; i++) {
>
> did you really want to test dump_seg_size[mem_idx]?
>
>> + continue;
>> +
>> + dump_tlv = buf;
>> + dump_tlv->type = cpu_to_le32(mem_idx);
>> + dump_tlv->tlv_len = cpu_to_le32(dump_seg_sz[mem_idx]);
>> + buf += COREDUMP_TLV_HDR_SIZE;
>> +
>> + for (i = 0; i < ab->qmi.mem_seg_count; i++) {
>> + mem_type = ath12k_coredump_get_dump_type
>> + (ab->qmi.target_mem[i].type);
>> +
>> + if (mem_type != mem_idx)
>> + continue;
>> +
>> + if (!ab->qmi.target_mem[i].paddr) {
>> + ath12k_dbg(ab, ATH12K_DBG_PCI,
>> + "Skipping mem region type %d",
>> + ab->qmi.target_mem[i].type);
>> + continue;
>> + }
>> +
>> + memcpy_fromio(buf, ab->qmi.target_mem[i].v.ioaddr,
>> + ab->qmi.target_mem[i].size);
>> + buf += ab->qmi.target_mem[i].size;
>> + }
>> + }
>> +
>> + queue_work(ab->workqueue, &ab->dump_work);
>> +}
>
Hi Jeff,
My Bad, posted wrong version of my changes. Thanks for catching it.
Yes, my intention was to check dump_seg_size[mem_idx]. I will update and send the next version.
Thanks,
Sowmiya Sree
