On Fri, May 03, 2024 at 01:48:07PM GMT, Mukesh Ojha wrote:
> Currently, Qualcomm Minidump is being used to collect mini version of
> remoteproc coredump with the help of boot firmware however, Minidump
> as a feature is not limited to be used only for remote processor and
> can also be used for Application processors. So, in preparation of
> using it move the Minidump related data structure and its function
> to its own file under drivers/soc/qcom with qcom_rproc_minidump.c
> which clearly says it is only for remoteproc minidump.
>
> Extra changes made apart from the movement is,
> 1. Adds new config, kernel headers and module macros to get this
> module compiled.
> 2. Guards the qcom_minidump() with CONFIG_QCOM_RPROC_MINIDUMP.
> 3. Selects this QCOM_RPROC_MINIDUMP config when QCOM_RPROC_COMMON
> enabled.
> 4. Added new header qcom_minidump.h .
>
> Signed-off-by: Mukesh Ojha <quic_mo...@quicinc.com>
I wouldn't be able to merge this without anything depending on it...
[..]
> diff --git a/drivers/soc/qcom/qcom_rproc_minidump.c
> b/drivers/soc/qcom/qcom_rproc_minidump.c
[..]
> +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) {
> + return rproc_coredump(rproc);
> + }
> +
> + if (le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE)
> {
> + dev_err(&rproc->dev, "Minidump not ready, skipping\n");
> + return;
> + }
> +
> + /**
> + * Clear out the dump segments populated by parse_fw before
> + * re-populating them with minidump segments.
> + */
> + rproc_coredump_cleanup(rproc);
I don't think this should be invoked outside drivers/remoteproc, and the
comment talks about a remoteproc-internal concern...
> +
> + ret = qcom_add_minidump_segments(rproc, subsystem, rproc_dumpfn_t);
This function changes the internal state of the remoteproc and relies on
other operations to clean things up.
I think we could come up with a better design of this, and I don't think
we should spread this outside of the remoteproc framework.
Regards,
Bjorn
> + if (ret) {
> + dev_err(&rproc->dev, "Failed with error: %d while adding
> minidump entries\n", ret);
> + goto clean_minidump;
> + }
> + rproc_coredump_using_sections(rproc);
> +clean_minidump:
> + qcom_minidump_cleanup(rproc);
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump);
> +
> +MODULE_DESCRIPTION("Qualcomm Remoteproc Minidump helper module");
> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/qcom/qcom_minidump.h
> b/include/soc/qcom/qcom_minidump.h
> new file mode 100644
> index 000000000000..0fe156066bc0
> --- /dev/null
> +++ b/include/soc/qcom/qcom_minidump.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _QCOM_MINIDUMP_H_
> +#define _QCOM_MINIDUMP_H_
> +
> +struct rproc;
> +struct rproc_dump_segment;
> +
> +#if IS_ENABLED(CONFIG_QCOM_RPROC_MINIDUMP)
> +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));
> +#else
> +static inline 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)) { }
> +#endif /* CONFIG_QCOM_RPROC_MINIDUMP */
> +#endif /* _QCOM_MINIDUMP_H_ */
> --
> 2.7.4
>
