Subject: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled

In case recovery is disabled, do not report the rproc crash
to the framework. If recovery is enabled after we start the
crash handler we may end up in a weird state by informing
clients of a crash twice, resulting in undefined behaviour.

Change-Id: If0d9bf5aa2c6f9e25adcefaca14b2de60fcb1a7a
Signed-off-by: Gokul krishna Krishnakumar <[email protected]>
---
drivers/remoteproc/qcom_q6v5.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 89f5384..1b9e1e1 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -103,6 +103,8 @@ static void qcom_q6v5_crash_handler_work(struct work_struct *work)

mutex_lock(&rproc->lock);

+ rproc->state = RPROC_CRASHED;
+
list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
if (subdev->stop)
subdev->stop(subdev, true);
@@ -139,8 +141,8 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
q6v5->running = false;
if (q6v5->rproc->recovery_disabled)
schedule_work(&q6v5->crash_handler);
-
- rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
+ else
+ rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);

return IRQ_HANDLED;
}
@@ -163,8 +165,8 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data)
q6v5->running = false;
if (q6v5->rproc->recovery_disabled)
schedule_work(&q6v5->crash_handler);
-
- rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
+ else
+ rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);

return IRQ_HANDLED;
}
--
2.7.4


2022-09-23 07:38:34

by Peng Fan

[permalink] [raw]
Subject: RE: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled

> Subject: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR
> is disabled
>
> In case recovery is disabled, do not report the rproc crash to the framework.
> If recovery is enabled after we start the crash handler we may end up in a
> weird state by informing clients of a crash twice, resulting in undefined
> behaviour.
>
> Change-Id: If0d9bf5aa2c6f9e25adcefaca14b2de60fcb1a7a

This should be removed.

> Signed-off-by: Gokul krishna Krishnakumar <[email protected]>
> ---
> drivers/remoteproc/qcom_q6v5.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5.c
> b/drivers/remoteproc/qcom_q6v5.c index 89f5384..1b9e1e1 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -103,6 +103,8 @@ static void qcom_q6v5_crash_handler_work(struct
> work_struct *work)
>
> mutex_lock(&rproc->lock);
>
> + rproc->state = RPROC_CRASHED;
> +
> list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> if (subdev->stop)
> subdev->stop(subdev, true);
> @@ -139,8 +141,8 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void
> *data)
> q6v5->running = false;
> if (q6v5->rproc->recovery_disabled)
> schedule_work(&q6v5->crash_handler);
> -
> - rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
> + else
> + rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
>
> return IRQ_HANDLED;
> }
> @@ -163,8 +165,8 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void
> *data)
> q6v5->running = false;
> if (q6v5->rproc->recovery_disabled)
> schedule_work(&q6v5->crash_handler);
> -
> - rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
> + else
> + rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
>
> return IRQ_HANDLED;
> }
> --
> 2.7.4
Regards,
Peng

2022-12-28 16:54:04

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled

On Mon, Sep 19, 2022 at 09:00:39AM -0700, Gokul krishna Krishnakumar wrote:
> In case recovery is disabled, do not report the rproc crash
> to the framework. If recovery is enabled after we start the
> crash handler we may end up in a weird state by informing
> clients of a crash twice, resulting in undefined behaviour.
>

Afaict rproc_report_crash() schedules work which does nothing useful if
!rproc->recovery_disabled.

Can you please help me understand the issue you're seeing, and
preferably spell out what the resulting weird state is.

Thanks,
Bjorn

> Change-Id: If0d9bf5aa2c6f9e25adcefaca14b2de60fcb1a7a
> Signed-off-by: Gokul krishna Krishnakumar <[email protected]>
> ---
> drivers/remoteproc/qcom_q6v5.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index 89f5384..1b9e1e1 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -103,6 +103,8 @@ static void qcom_q6v5_crash_handler_work(struct work_struct *work)
>
> mutex_lock(&rproc->lock);
>
> + rproc->state = RPROC_CRASHED;
> +
> list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> if (subdev->stop)
> subdev->stop(subdev, true);
> @@ -139,8 +141,8 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
> q6v5->running = false;
> if (q6v5->rproc->recovery_disabled)
> schedule_work(&q6v5->crash_handler);
> -
> - rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
> + else
> + rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
>
> return IRQ_HANDLED;
> }
> @@ -163,8 +165,8 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data)
> q6v5->running = false;
> if (q6v5->rproc->recovery_disabled)
> schedule_work(&q6v5->crash_handler);
> -
> - rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
> + else
> + rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
>
> return IRQ_HANDLED;
> }
> --
> 2.7.4
>