Re: [PATCH v4 13/21] remoterproc: qcom: refactor to leverage exported minidump symbol

Mukesh Ojha
2023-06-29T09:21:10Z

On 6/28/2023 9:21 PM, Pavan Kondeti wrote:
> On Wed, Jun 28, 2023 at 06:04:40PM +0530, Mukesh Ojha wrote:
>> -static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsystem *subsystem,
>> - void (*rproc_dumpfn_t)(struct rproc *rproc, struct rproc_dump_segment *segment,
>> - void *dest, size_t offset, size_t size))
>> +void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
>> + void (*rproc_dumpfn_t)(struct rproc *rproc,
>> + struct rproc_dump_segment *segment, void *dest, size_t offset,
>> + size_t size))
>> {
>> - struct minidump_region __iomem *ptr;
>> - struct minidump_region region;
>> - int seg_cnt, i;
>> dma_addr_t da;
>> size_t size;
>> + int seg_cnt;
>> char *name;
>> + void *ptr;
>> + int ret;
>> + int i;
>>
>> if (WARN_ON(!list_empty(&rproc->dump_segments))) {
>> dev_err(&rproc->dev, "dump segment list already populated\n");
>> - return -EUCLEAN;
>> + return;
>> }
>>
>> - seg_cnt = le32_to_cpu(subsystem->region_count);
>> - ptr = ioremap((unsigned long)le64_to_cpu(subsystem->regions_baseptr),
>> - seg_cnt * sizeof(struct minidump_region));
>> + ptr = qcom_ss_md_mapped_base(minidump_id, &seg_cnt);
>> if (!ptr)
>> - return -EFAULT;
>> + return;
>>
>> for (i = 0; i < seg_cnt; i++) {
>> - memcpy_fromio(&region, ptr + i, sizeof(region));
>> - if (le32_to_cpu(region.valid) == MINIDUMP_REGION_VALID) {
>> - name = kstrndup(region.name, MAX_REGION_NAME_LENGTH - 1, GFP_KERNEL);
>> - if (!name) {
>> - iounmap(ptr);
>> - return -ENOMEM;
>> - }
>> - da = le64_to_cpu(region.address);
>> - size = le64_to_cpu(region.size);
>> - rproc_coredump_add_custom_segment(rproc, da, size, rproc_dumpfn_t, name);
>> + ret = qcom_ss_valid_segment_info(ptr, i, &name, &da, &size);
>> + if (ret < 0) {
>> + iounmap(ptr);
>> + dev_err(&rproc->dev,
>> + "Failed with error: %d while adding minidump entries\n",
>> + ret);
>> + goto clean_minidump;
>> }
>> - }
>> -
>> - iounmap(ptr);
>> - return 0;
>> -}
>> -
>> -void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
>> - void (*rproc_dumpfn_t)(struct rproc *rproc,
>> - struct rproc_dump_segment *segment, void *dest, size_t offset,
>> - size_t size))
>> -{
>> - int ret;
>> - struct minidump_subsystem *subsystem;
>> - struct minidump_global_toc *toc;
>> -
>> - /* Get Global minidump ToC*/
>> - toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
>> -
>> - /* check if global table pointer exists and init is set */
>> - if (IS_ERR(toc) || !toc->status) {
>> - dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>> - return;
>> - }
>>
>> - /* Get subsystem table of contents using the minidump id */
>> - subsystem = &toc->subsystems[minidump_id];
>> -
>> - /**
>> - * Collect minidump if SS ToC is valid and segment table
>> - * is initialized in memory and encryption status is set.
>> - */
>> - if (subsystem->regions_baseptr == 0 ||
>> - le32_to_cpu(subsystem->status) != 1 ||
>> - le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>> - le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE) {
>> - dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>> - return;
>> + /* if it is a valid segment */
>> + if (!ret)
>> + rproc_coredump_add_custom_segment(rproc, da, size,
>> + rproc_dumpfn_t, name);
>> }
>>
>> - ret = qcom_add_minidump_segments(rproc, subsystem, rproc_dumpfn_t);
>> - if (ret) {
>> - dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
>> - goto clean_minidump;
>> - }
>> + iounmap(ptr);
>> rproc_coredump_using_sections(rproc);
>> +
>> clean_minidump:
>> qcom_minidump_cleanup(rproc);
>> }
>
> I like the idea of moving minidump pieces to drivers/soc/qcom/*minidump*.
>
> Is it possible to accept one function callback from remoteproc and do
> all of this in one function exported by minidump?
>
> qcom_ss_valid_segment_info() seems a low level function to be exported..

It was ending up with circular dependency due to
rproc_coredump_add_custom_segment()

rproc_coredump => qcom_common => qcom_minidump_smem => rproc_coredump

-Mukesh

>
> Thanks,
> Pavan
>