Re: [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup

[Mukesh Ojha <quic_mojha@quicinc.com>]

On 5/26/2023 5:25 PM, Christian Marangi wrote:
> Commit ebeb20a9cd3f ("soc: qcom: mdt_loader: Always invoke PAS
> mem_setup") dropped the relocate check and made pas_mem_setup run
> unconditionally. The code was later moved with commit f4e526ff7e38
> ("soc: qcom: mdt_loader: Extract PAS operations") to
> qcom_mdt_pas_init() effectively losing track of what was actually
> done.
> 
> The assumption that PAS mem_setup can be done anytime was effectively
> wrong, with no good reason and this caused regression on some SoC
> that use remoteproc to bringup ath11k. One example is IPQ8074 SoC that
> effectively broke resulting in remoteproc silently die and ath11k not
> working.
> 
> On this SoC FW relocate is not enabled and PAS mem_setup was correctly
> skipped in previous kernel version resulting in correct bringup and
> function of remoteproc and ath11k.
> 
> To fix the regression, reintroduce the relocate check in
> qcom_mdt_pas_init() and correctly skip PAS mem_setup where relocate is
> not enabled.
> 
> Fixes: ebeb20a9cd3f ("soc: qcom: mdt_loader: Always invoke PAS mem_setup")
> Tested-by: Robert Marko <robimarko@gmail.com>
> Co-developed-by: Robert Marko <robimarko@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org

Thanks.

> ---
>   drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 33dd8c315eb7..46820bcdae98 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -210,6 +210,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   	const struct elf32_hdr *ehdr;
>   	phys_addr_t min_addr = PHYS_ADDR_MAX;
>   	phys_addr_t max_addr = 0;
> +	bool relocate = false;
>   	size_t metadata_len;
>   	void *metadata;
>   	int ret;
> @@ -224,6 +225,9 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   		if (!mdt_phdr_valid(phdr))
>   			continue;
>   
> +		if (phdr->p_flags & QCOM_MDT_RELOCATABLE)
> +			relocate = true;
> +
>   		if (phdr->p_paddr < min_addr)
>   			min_addr = phdr->p_paddr;
>   
> @@ -246,11 +250,13 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   		goto out;
>   	}
>   
> -	ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
> -	if (ret) {
> -		/* Unable to set up relocation */
> -		dev_err(dev, "error %d setting up firmware %s\n", ret, fw_name);
> -		goto out;
> +	if (relocate) {
> +		ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr);
> +		if (ret) {
> +			/* Unable to set up relocation */
> +			dev_err(dev, "error %d setting up firmware %s\n", ret, fw_name);
> +			goto out;
> +		}
>   	}
>   
>   out:

LGTM, We still carry this in our downstream kernel for legacy reason.
Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>

-- Mukesh