Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] remoteproc: qcom: pas: Adjust the phys addr wrt the mem
 region
To:     Yogesh Lal <quic_ylal@quicinc.com>, <bjorn.andersson@linaro.org>
CC:     <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1654200007-5453-1-git-send-email-quic_ylal@quicinc.com>
From:   Sibi Sankar <quic_sibis@quicinc.com>
Message-ID: <ed665e94-dab9-9d6c-87d7-aa1efd157755@quicinc.com>
Date:   Fri, 24 Jun 2022 16:49:11 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <1654200007-5453-1-git-send-email-quic_ylal@quicinc.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hey Yogesh,
Thanks for the patch.

On 6/3/22 1:30 AM, Yogesh Lal wrote:
> The minidump table in the toc contains physical addresses that may lie
> before the physical address of the first elf segment in relocatable

doesn't this apply to full coredumps as well? Do you plan to address
that in a separate patch?

> images. This change adds a custom dump function for minidumps which
> calculates the offset into the carveout region using the start of
> the physical address instead of the start of the first elf segment.
> 
> Signed-off-by: Yogesh Lal <quic_ylal@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.c   |  9 +++++----
>   drivers/remoteproc/qcom_common.h   |  5 ++++-
>   drivers/remoteproc/qcom_q6v5_pas.c | 21 ++++++++++++++++++++-
>   3 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 246e716..503326b 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -101,7 +101,8 @@ static void qcom_minidump_cleanup(struct rproc *rproc)
>   	}
>   }
>   
> -static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsystem *subsystem)
> +static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsystem *subsystem,
> +									rproc_dumpfn_t dumpfn)
>   {
>   	struct minidump_region __iomem *ptr;
>   	struct minidump_region region;
> @@ -131,7 +132,7 @@ static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsy
>   			}
>   			da = le64_to_cpu(region.address);
>   			size = le32_to_cpu(region.size);
> -			rproc_coredump_add_custom_segment(rproc, da, size, NULL, name);
> +			rproc_coredump_add_custom_segment(rproc, da, size, dumpfn, name);
>   		}
>   	}
>   
> @@ -139,7 +140,7 @@ static int qcom_add_minidump_segments(struct rproc *rproc, struct minidump_subsy
>   	return 0;
>   }
>   
> -void qcom_minidump(struct rproc *rproc, unsigned int minidump_id)
> +void qcom_minidump(struct rproc *rproc, unsigned int minidump_id, rproc_dumpfn_t dumpfn)
>   {
>   	int ret;
>   	struct minidump_subsystem *subsystem;
> @@ -179,7 +180,7 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id)
>   
>   	rproc_coredump_cleanup(rproc);
>   
> -	ret = qcom_add_minidump_segments(rproc, subsystem);
> +	ret = qcom_add_minidump_segments(rproc, subsystem, dumpfn);
>   	if (ret) {
>   		dev_err(&rproc->dev, "Failed with error: %d while adding minidump entries\n", ret);
>   		goto clean_minidump;
> diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> index c35adf7..29e528b 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -33,7 +33,10 @@ struct qcom_rproc_ssr {
>   	struct qcom_ssr_subsystem *info;
>   };
>   
> -void qcom_minidump(struct rproc *rproc, unsigned int minidump_id);
> +typedef void (*rproc_dumpfn_t)(struct rproc *rproc, struct rproc_dump_segment *segment,
> +			void *dest, size_t offset, size_t size);

you can perhaps stick with not using typedef like how it is handled in
remoteproc_coredump.

> +
> +void qcom_minidump(struct rproc *rproc, unsigned int minidump_id, rproc_dumpfn_t dumpfn);
>   
>   void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink,
>   			   const char *ssr_name);
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 6e5cbca..9c6cb0b 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -83,11 +83,30 @@ struct qcom_adsp {
>   	struct qcom_sysmon *sysmon;
>   };
>   
> +void adsp_segment_dump(struct rproc *rproc, struct rproc_dump_segment *segment,
> +						void *dest, size_t offset, size_t size)
> +{
> +	struct qcom_adsp *adsp = rproc->priv;
> +	int total_offset;
> +
> +	total_offset = segment->da + segment->offset + offset - adsp->mem_phys;
> +	if (total_offset < 0 || total_offset + size > adsp->mem_size) {
> +		dev_err(adsp->dev,
> +			"invalid copy request for segment %pad with offset %zu and size %zu)\n",
> +			&segment->da, offset, size);
> +		memset(dest, 0xff, size);
> +		return;
> +	}
> +
> +	memcpy_fromio(dest, adsp->mem_region + total_offset, size);
> +}
> +
> +
remove additional empty line.

>   static void adsp_minidump(struct rproc *rproc)
>   {
>   	struct qcom_adsp *adsp = rproc->priv;
>   
> -	qcom_minidump(rproc, adsp->minidump_id);
> +	qcom_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
>   }
>   
>   static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
>