[PATCH 0/3] remoteproc: qcom_q6v5: Misc fixes to prepare for reusing the "lite" ADSP FW

[PATCH 0/3] remoteproc: qcom_q6v5: Misc fixes to prepare for reusing the "lite" ADSP FW

[Stephan Gerhold]

On X1E, the boot firmware already loads a "lite" ADSP firmware that
provides essential functionality such as charging, battery status and USB-C
detection. Only the audio functionality is missing. Since the full ADSP
firmware is device-specific and needs to be manually copied by the user, it
would be useful if we could provide the basic functionality even without
having the full firmware present.

I have a working prototype for this that I will post soon. To keep that
series smaller, this series contains some misc fixes for minor issues
I noticed while working on this feature. The issues are present even
without my additional patches, so the fixes can be picked up independently.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
Stephan Gerhold (3):
      remoteproc: qcom_q6v5: Avoid disabling handover IRQ twice
      remoteproc: qcom_q6v5: Avoid handling handover twice
      remoteproc: qcom_q6v5_pas: Shutdown lite ADSP DTB on X1E

 drivers/remoteproc/qcom_q6v5.c     | 8 +++++---
 drivers/remoteproc/qcom_q6v5_pas.c | 8 +++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250814-rproc-qcom-q6v5-fixes-9882ad571097

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

[PATCH 1/3] remoteproc: qcom_q6v5: Avoid disabling handover IRQ twice

[Stephan Gerhold]

enable_irq() and disable_irq() are reference counted, so we must make sure
that each enable_irq() is always paired with a single disable_irq(). If we
call disable_irq() twice followed by just a single enable_irq(), the IRQ
will remain disabled forever.

For the error handling path in qcom_q6v5_wait_for_start(), disable_irq()
will end up being called twice, because disable_irq() also happens in
qcom_q6v5_unprepare() when rolling back the call to qcom_q6v5_prepare().

Fix this by dropping disable_irq() in qcom_q6v5_wait_for_start(). Since
qcom_q6v5_prepare() is the function that calls enable_irq(), it makes more
sense to have the rollback handled always by qcom_q6v5_unprepare().

Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource handling")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/remoteproc/qcom_q6v5.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 4ee5e67a9f03f5f766f04396b9a3e45f77293764..769c6d6d6a731672eca9f960b05c68f6d4d77af2 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -156,9 +156,6 @@ int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout)
 	int ret;
 
 	ret = wait_for_completion_timeout(&q6v5->start_done, timeout);
-	if (!ret)
-		disable_irq(q6v5->handover_irq);
-
 	return !ret ? -ETIMEDOUT : 0;
 }
 EXPORT_SYMBOL_GPL(qcom_q6v5_wait_for_start);

-- 
2.50.1

Re: [PATCH 1/3] remoteproc: qcom_q6v5: Avoid disabling handover IRQ twice

[Dmitry Baryshkov]

On Tue, Aug 19, 2025 at 01:08:02PM +0200, Stephan Gerhold wrote:
> enable_irq() and disable_irq() are reference counted, so we must make sure
> that each enable_irq() is always paired with a single disable_irq(). If we
> call disable_irq() twice followed by just a single enable_irq(), the IRQ
> will remain disabled forever.
> 
> For the error handling path in qcom_q6v5_wait_for_start(), disable_irq()
> will end up being called twice, because disable_irq() also happens in
> qcom_q6v5_unprepare() when rolling back the call to qcom_q6v5_prepare().
> 
> Fix this by dropping disable_irq() in qcom_q6v5_wait_for_start(). Since
> qcom_q6v5_prepare() is the function that calls enable_irq(), it makes more
> sense to have the rollback handled always by qcom_q6v5_unprepare().
> 
> Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource handling")

Didn't earlier versions also have the same behaviour?

> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/remoteproc/qcom_q6v5.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index 4ee5e67a9f03f5f766f04396b9a3e45f77293764..769c6d6d6a731672eca9f960b05c68f6d4d77af2 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -156,9 +156,6 @@ int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout)
>  	int ret;
>  
>  	ret = wait_for_completion_timeout(&q6v5->start_done, timeout);
> -	if (!ret)
> -		disable_irq(q6v5->handover_irq);
> -
>  	return !ret ? -ETIMEDOUT : 0;
>  }
>  EXPORT_SYMBOL_GPL(qcom_q6v5_wait_for_start);
> 
> -- 
> 2.50.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] remoteproc: qcom_q6v5: Avoid disabling handover IRQ twice

[Stephan Gerhold]

On Tue, Aug 19, 2025 at 02:44:26PM +0300, Dmitry Baryshkov wrote:
> On Tue, Aug 19, 2025 at 01:08:02PM +0200, Stephan Gerhold wrote:
> > enable_irq() and disable_irq() are reference counted, so we must make sure
> > that each enable_irq() is always paired with a single disable_irq(). If we
> > call disable_irq() twice followed by just a single enable_irq(), the IRQ
> > will remain disabled forever.
> > 
> > For the error handling path in qcom_q6v5_wait_for_start(), disable_irq()
> > will end up being called twice, because disable_irq() also happens in
> > qcom_q6v5_unprepare() when rolling back the call to qcom_q6v5_prepare().
> > 
> > Fix this by dropping disable_irq() in qcom_q6v5_wait_for_start(). Since
> > qcom_q6v5_prepare() is the function that calls enable_irq(), it makes more
> > sense to have the rollback handled always by qcom_q6v5_unprepare().
> > 
> > Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource handling")
> 
> Didn't earlier versions also have the same behaviour?
> 

I don't think so. The "extracted common resource handling" came from
qcom_q6v5_pil.c, but q6v5_start() just had most of this code inline in a
single function [1]. The handling of enable_irq()/disable_irq() through
the goto labels looks correct there.

Thanks,
Stephan

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_pil.c?id=0e622e80191e75c99b6ecc265c140a37d81e7a63#n795

Re: [PATCH 1/3] remoteproc: qcom_q6v5: Avoid disabling handover IRQ twice

[Dmitry Baryshkov]

On Tue, Aug 19, 2025 at 04:55:09PM +0200, Stephan Gerhold wrote:
> On Tue, Aug 19, 2025 at 02:44:26PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Aug 19, 2025 at 01:08:02PM +0200, Stephan Gerhold wrote:
> > > enable_irq() and disable_irq() are reference counted, so we must make sure
> > > that each enable_irq() is always paired with a single disable_irq(). If we
> > > call disable_irq() twice followed by just a single enable_irq(), the IRQ
> > > will remain disabled forever.
> > > 
> > > For the error handling path in qcom_q6v5_wait_for_start(), disable_irq()
> > > will end up being called twice, because disable_irq() also happens in
> > > qcom_q6v5_unprepare() when rolling back the call to qcom_q6v5_prepare().
> > > 
> > > Fix this by dropping disable_irq() in qcom_q6v5_wait_for_start(). Since
> > > qcom_q6v5_prepare() is the function that calls enable_irq(), it makes more
> > > sense to have the rollback handled always by qcom_q6v5_unprepare().
> > > 
> > > Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource handling")
> > 
> > Didn't earlier versions also have the same behaviour?
> > 
> 
> I don't think so. The "extracted common resource handling" came from
> qcom_q6v5_pil.c, but q6v5_start() just had most of this code inline in a
> single function [1]. The handling of enable_irq()/disable_irq() through
> the goto labels looks correct there.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

[PATCH 2/3] remoteproc: qcom_q6v5: Avoid handling handover twice

[Stephan Gerhold]

A remoteproc could theoretically signal handover twice. This is unexpected
and would break the reference counting for the handover resources (power
domains, clocks, regulators, etc), so add a check to prevent that from
happening.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/remoteproc/qcom_q6v5.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 769c6d6d6a731672eca9f960b05c68f6d4d77af2..58d5b85e58cdadabdd3e23d39c06a39196c3a194 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -164,6 +164,11 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data)
 {
 	struct qcom_q6v5 *q6v5 = data;
 
+	if (q6v5->handover_issued) {
+		dev_err(q6v5->dev, "Handover signaled, but it already happened\n");
+		return IRQ_HANDLED;
+	}
+
 	if (q6v5->handover)
 		q6v5->handover(q6v5);
 

-- 
2.50.1

Re: [PATCH 2/3] remoteproc: qcom_q6v5: Avoid handling handover twice

[Dmitry Baryshkov]

On Tue, Aug 19, 2025 at 01:08:03PM +0200, Stephan Gerhold wrote:
> A remoteproc could theoretically signal handover twice. This is unexpected

theoretically or practically?

> and would break the reference counting for the handover resources (power
> domains, clocks, regulators, etc), so add a check to prevent that from
> happening.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/remoteproc/qcom_q6v5.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index 769c6d6d6a731672eca9f960b05c68f6d4d77af2..58d5b85e58cdadabdd3e23d39c06a39196c3a194 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -164,6 +164,11 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *data)
>  {
>  	struct qcom_q6v5 *q6v5 = data;
>  
> +	if (q6v5->handover_issued) {
> +		dev_err(q6v5->dev, "Handover signaled, but it already happened\n");
> +		return IRQ_HANDLED;
> +	}
> +
>  	if (q6v5->handover)
>  		q6v5->handover(q6v5);
>  
> 
> -- 
> 2.50.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] remoteproc: qcom_q6v5: Avoid handling handover twice

[Stephan Gerhold]

On Tue, Aug 19, 2025 at 02:41:55PM +0300, Dmitry Baryshkov wrote:
> On Tue, Aug 19, 2025 at 01:08:03PM +0200, Stephan Gerhold wrote:
> > A remoteproc could theoretically signal handover twice. This is unexpected
> 
> theoretically or practically?
> 

You could easily trigger handover again from a custom remoteproc
firmware by setting the handover state to 0 and then back to 1. However,
if you find a firmware version doing this, you might want to have a
serious conversation with the firmware developer. It makes no sense to
do that. :-)

In other words, on technical level it is practical. From a conceptual
point of view it is just theoretical.

In any case, if it happens, we shouldn't mess up reference counters in
my opinion (or risk dereferencing invalid pointers etc).

Thanks,
Stephan

Re: [PATCH 2/3] remoteproc: qcom_q6v5: Avoid handling handover twice

[Dmitry Baryshkov]

On Tue, Aug 19, 2025 at 05:05:13PM +0200, Stephan Gerhold wrote:
> On Tue, Aug 19, 2025 at 02:41:55PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Aug 19, 2025 at 01:08:03PM +0200, Stephan Gerhold wrote:
> > > A remoteproc could theoretically signal handover twice. This is unexpected
> > 
> > theoretically or practically?
> > 
> 
> You could easily trigger handover again from a custom remoteproc
> firmware by setting the handover state to 0 and then back to 1. However,
> if you find a firmware version doing this, you might want to have a
> serious conversation with the firmware developer. It makes no sense to
> do that. :-)
> 
> In other words, on technical level it is practical. From a conceptual
> point of view it is just theoretical.
> 
> In any case, if it happens, we shouldn't mess up reference counters in
> my opinion (or risk dereferencing invalid pointers etc).

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

[PATCH 3/3] remoteproc: qcom_q6v5_pas: Shutdown lite ADSP DTB on X1E

[Stephan Gerhold]

The ADSP firmware on X1E has separate firmware binaries for the main
firmware and the DTB. The same applies for the "lite" firmware loaded by
the boot firmware.

When preparing to load the new ADSP firmware we shutdown the lite_pas_id
for the main firmware, but we don't shutdown the corresponding lite pas_id
for the DTB. The fact that we're leaving it "running" forever becomes
obvious if you try to reuse (or just access) the memory region used by the
"lite" firmware: The &adsp_boot_mem is accessible, but accessing the
&adsp_boot_dtb_mem results in a crash.

We don't support reusing the memory regions currently, but nevertheless we
should not keep part of the lite firmware running. Fix this by adding the
lite_dtb_pas_id and shutting it down as well.

We don't have a way to detect if the lite firmware is actually running yet,
so ignore the return status of qcom_scm_pas_shutdown() for now. This was
already the case before, the assignment to "ret" was not used anywhere.

Fixes: 62210f7509e1 ("remoteproc: qcom_q6v5_pas: Unload lite firmware on ADSP")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 02e29171cbbee2d305827365ef7d2241b6eb786b..6faedae8d32ef6c3c2071975f2f1e37a9ffd8abe 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -42,6 +42,7 @@ struct qcom_pas_data {
 	int pas_id;
 	int dtb_pas_id;
 	int lite_pas_id;
+	int lite_dtb_pas_id;
 	unsigned int minidump_id;
 	bool auto_boot;
 	bool decrypt_shutdown;
@@ -80,6 +81,7 @@ struct qcom_pas {
 	int pas_id;
 	int dtb_pas_id;
 	int lite_pas_id;
+	int lite_dtb_pas_id;
 	unsigned int minidump_id;
 	int crash_reason_smem;
 	unsigned int smem_host_id;
@@ -225,7 +227,9 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
 	pas->firmware = fw;
 
 	if (pas->lite_pas_id)
-		ret = qcom_scm_pas_shutdown(pas->lite_pas_id);
+		qcom_scm_pas_shutdown(pas->lite_pas_id);
+	if (pas->lite_dtb_pas_id)
+		qcom_scm_pas_shutdown(pas->lite_dtb_pas_id);
 
 	if (pas->dtb_pas_id) {
 		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
@@ -722,6 +726,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
 	pas->minidump_id = desc->minidump_id;
 	pas->pas_id = desc->pas_id;
 	pas->lite_pas_id = desc->lite_pas_id;
+	pas->lite_dtb_pas_id = desc->lite_dtb_pas_id;
 	pas->info_name = desc->sysmon_name;
 	pas->smem_host_id = desc->smem_host_id;
 	pas->decrypt_shutdown = desc->decrypt_shutdown;
@@ -1085,6 +1090,7 @@ static const struct qcom_pas_data x1e80100_adsp_resource = {
 	.pas_id = 1,
 	.dtb_pas_id = 0x24,
 	.lite_pas_id = 0x1f,
+	.lite_dtb_pas_id = 0x29,
 	.minidump_id = 5,
 	.auto_boot = true,
 	.proxy_pd_names = (char*[]){

-- 
2.50.1

Re: [PATCH 3/3] remoteproc: qcom_q6v5_pas: Shutdown lite ADSP DTB on X1E

[Dmitry Baryshkov]

On Tue, Aug 19, 2025 at 01:08:04PM +0200, Stephan Gerhold wrote:
> The ADSP firmware on X1E has separate firmware binaries for the main
> firmware and the DTB. The same applies for the "lite" firmware loaded by
> the boot firmware.
> 
> When preparing to load the new ADSP firmware we shutdown the lite_pas_id
> for the main firmware, but we don't shutdown the corresponding lite pas_id
> for the DTB. The fact that we're leaving it "running" forever becomes
> obvious if you try to reuse (or just access) the memory region used by the
> "lite" firmware: The &adsp_boot_mem is accessible, but accessing the
> &adsp_boot_dtb_mem results in a crash.
> 
> We don't support reusing the memory regions currently, but nevertheless we
> should not keep part of the lite firmware running. Fix this by adding the
> lite_dtb_pas_id and shutting it down as well.
> 
> We don't have a way to detect if the lite firmware is actually running yet,
> so ignore the return status of qcom_scm_pas_shutdown() for now. This was
> already the case before, the assignment to "ret" was not used anywhere.
> 
> Fixes: 62210f7509e1 ("remoteproc: qcom_q6v5_pas: Unload lite firmware on ADSP")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 02e29171cbbee2d305827365ef7d2241b6eb786b..6faedae8d32ef6c3c2071975f2f1e37a9ffd8abe 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -42,6 +42,7 @@ struct qcom_pas_data {
>  	int pas_id;
>  	int dtb_pas_id;
>  	int lite_pas_id;
> +	int lite_dtb_pas_id;
>  	unsigned int minidump_id;
>  	bool auto_boot;
>  	bool decrypt_shutdown;
> @@ -80,6 +81,7 @@ struct qcom_pas {
>  	int pas_id;
>  	int dtb_pas_id;
>  	int lite_pas_id;
> +	int lite_dtb_pas_id;
>  	unsigned int minidump_id;
>  	int crash_reason_smem;
>  	unsigned int smem_host_id;
> @@ -225,7 +227,9 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>  	pas->firmware = fw;
>  
>  	if (pas->lite_pas_id)
> -		ret = qcom_scm_pas_shutdown(pas->lite_pas_id);
> +		qcom_scm_pas_shutdown(pas->lite_pas_id);

Unrelated change. With it being split to a separate commit:


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>



> +	if (pas->lite_dtb_pas_id)
> +		qcom_scm_pas_shutdown(pas->lite_dtb_pas_id);
>  
>  	if (pas->dtb_pas_id) {
>  		ret = request_firmware(&pas->dtb_firmware, pas->dtb_firmware_name, pas->dev);
> @@ -722,6 +726,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
>  	pas->minidump_id = desc->minidump_id;
>  	pas->pas_id = desc->pas_id;
>  	pas->lite_pas_id = desc->lite_pas_id;
> +	pas->lite_dtb_pas_id = desc->lite_dtb_pas_id;
>  	pas->info_name = desc->sysmon_name;
>  	pas->smem_host_id = desc->smem_host_id;
>  	pas->decrypt_shutdown = desc->decrypt_shutdown;
> @@ -1085,6 +1090,7 @@ static const struct qcom_pas_data x1e80100_adsp_resource = {
>  	.pas_id = 1,
>  	.dtb_pas_id = 0x24,
>  	.lite_pas_id = 0x1f,
> +	.lite_dtb_pas_id = 0x29,
>  	.minidump_id = 5,
>  	.auto_boot = true,
>  	.proxy_pd_names = (char*[]){
> 
> -- 
> 2.50.1
> 

-- 
With best wishes
Dmitry