[PATCH v4] wifi: ath12k: Add firmware coredump collection support

[Sowmiya Sree Elavalagan]

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 at quicinc.com>
>> ---
>> 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