* [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup
@ 2023-05-26 11:55 Christian Marangi
2023-05-26 14:27 ` Mukesh Ojha
2023-05-27 4:00 ` Bjorn Andersson
0 siblings, 2 replies; 3+ messages in thread
From: Christian Marangi @ 2023-05-26 11:55 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Dmitry Baryshkov,
linux-arm-msm, linux-kernel
Cc: Christian Marangi, Robert Marko, stable
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
---
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:
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup
2023-05-26 11:55 [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup Christian Marangi
@ 2023-05-26 14:27 ` Mukesh Ojha
2023-05-27 4:00 ` Bjorn Andersson
1 sibling, 0 replies; 3+ messages in thread
From: Mukesh Ojha @ 2023-05-26 14:27 UTC (permalink / raw)
To: Christian Marangi, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Baryshkov, linux-arm-msm, linux-kernel
Cc: Robert Marko, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup
2023-05-26 11:55 [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup Christian Marangi
2023-05-26 14:27 ` Mukesh Ojha
@ 2023-05-27 4:00 ` Bjorn Andersson
1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Andersson @ 2023-05-27 4:00 UTC (permalink / raw)
To: Christian Marangi, linux-kernel, linux-arm-msm, Dmitry Baryshkov,
Konrad Dybcio, Andy Gross
Cc: Robert Marko, stable
On Fri, 26 May 2023 13:55:11 +0200, 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.
>
> [...]
Applied, thanks!
[1/1] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup
commit: bcb889891371c3cf767f2b9e8768cfe2fdd3810f
Best regards,
--
Bjorn Andersson <andersson@kernel.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-27 3:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 11:55 [PATCH] soc: qcom: mdt_loader: Fix unconditional call to scm_pas_mem_setup Christian Marangi
2023-05-26 14:27 ` Mukesh Ojha
2023-05-27 4:00 ` Bjorn Andersson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.