| Message ID | 1a548f4e7373c12159f8d866dc0e12d224460036.1662995608.git.quic_gokukris@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Handle coprocessor crash | expand |
> 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 <quic_gokukris@quicinc.com> > --- > 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
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 <quic_gokukris@quicinc.com> > --- > 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 >
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; }
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 <quic_gokukris@quicinc.com> --- drivers/remoteproc/qcom_q6v5.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)