Re: [PATCH v3]wifi: ath12k: Add firmware coredump collection support

[Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>]

On 6/27/2024 9:16 PM, Sidhanta Sahu wrote:
> 
> 
> On 3/25/2024 11:34 AM, 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
>>
>> Signed-off-by: Sowmiya Sree Elavalagan <quic_ssreeela@quicinc.com>
>> ---
>> v2:
>>    - Fixed errors shown by ath12k-check
>> v3:
>>    - Fixed SPDX comment style for coredump.c file
>>      Changed Kconfig description.
>> ---
>>   drivers/net/wireless/ath/ath12k/Kconfig    |  10 ++
>>   drivers/net/wireless/ath/ath12k/Makefile   |   1 +
>>   drivers/net/wireless/ath/ath12k/core.c     |   2 +
>>   drivers/net/wireless/ath/ath12k/core.h     |   5 +
>>   drivers/net/wireless/ath/ath12k/coredump.c |  51 ++++++
>>   drivers/net/wireless/ath/ath12k/coredump.h |  80 +++++++++
>>   drivers/net/wireless/ath/ath12k/hif.h      |   9 +-
>>   drivers/net/wireless/ath/ath12k/hw.c       |   4 +-
>>   drivers/net/wireless/ath/ath12k/mhi.c      |   5 +
>>   drivers/net/wireless/ath/ath12k/mhi.h      |   4 +-
>>   drivers/net/wireless/ath/ath12k/pci.c      | 185 +++++++++++++++++++++
>>   11 files changed, 351 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/net/wireless/ath/ath12k/coredump.c
>>   create mode 100644 drivers/net/wireless/ath/ath12k/coredump.h
>>
> 
> 
>> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
>> index 391b6fb2bd42..f0cc4959faf5 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/core.c
>> @@ -1121,6 +1121,7 @@ static void ath12k_core_reset(struct work_struct *work)
>>       reinit_completion(&ab->recovery_start);
>>       atomic_set(&ab->recovery_count, 0);
>>   +    ath12k_coredump_collect(ab);
> 
> 1. Can we confirm if the operation guarantees that the coredump has been successfully uploaded upon its completion? It would be helpful to understand the expected behavior of the system in the event of a successful or unsuccessful upload.
> 
  You would see the print "Uploading coredump" on the console upon successful upload of the coredump. In case if the dump collection fails on the driver side itself you would not see the debug message.

> 2. Is it safe to proceed with memory cleanup operations immediately after this call? Understanding the dependencies and potential risks associated with memory cleanup post-upload would be beneficial for maintaining system stability and avoiding potential issues.
> 
  If you see the implementation completely, we take a copy of the memory to be dumped in the above call and queue the work of uploading to user space. All this happens before we proceed with clean up.  We cannot wait for clean up until coredump gets uploaded as it may delay the recovery process which is not accepted.

>>       ath12k_core_pre_reconfigure_recovery(ab);
>>         reinit_completion(&ab->reconfigure_complete);
>> @@ -1220,6 +1221,7 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
>>       INIT_WORK(&ab->restart_work, ath12k_core_restart);
>>       INIT_WORK(&ab->reset_work, ath12k_core_reset);
>>       INIT_WORK(&ab->rfkill_work, ath12k_rfkill_work);
>> +    INIT_WORK(&ab->dump_work, ath12k_coredump_upload);
>>         timer_setup(&ab->rx_replenish_retry, ath12k_ce_rx_replenish_retry, 0);
>>       init_completion(&ab->htc_suspend);

Hi Sidhanta,

Please find my response inline.

Thanks,
Sowmiya Sree