* [PATCH v1 0/3] Handle coprocessor crash
@ 2022-09-13 18:35 Gokul krishna Krishnakumar
2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Gokul krishna Krishnakumar @ 2022-09-13 18:35 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier
Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni,
Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman,
Guru Das Srinagesh, Gokul krishna Krishnakumar
Make the following changes in case of coprocessor crash:
1. Send subdevice notifications before panic
2. Do not report crash if SSR is disabled
3. Avoid setting smem bit in case of crash
Gokul krishna Krishnakumar (3):
remoteproc: qcom: q6v5: Send subdevice notifications before panic
remoteproc: qcom: q6v5: Do not report crash if SSR is disabled
remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash
shutdown
drivers/remoteproc/qcom_q6v5.c | 43 ++++++++++++++++++++++++++++++++++++++----
drivers/remoteproc/qcom_q6v5.h | 2 ++
2 files changed, 41 insertions(+), 4 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar @ 2022-09-13 18:35 ` Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar ` (2 more replies) 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar ` (3 subsequent siblings) 4 siblings, 3 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-13 18:35 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar Subdevice notifications after a remoteproc has crashed are useful to any clients that might want to preserve data pertaining to the driver after the remoteproc crashed. Sending subdevice notifications before triggering a kernel panic gives these drivers the time to do collect this information. Change-Id: Id6e55fb038b70f54ff5854d2adff72b74b6a9570 Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> --- drivers/remoteproc/qcom_q6v5.c | 31 +++++++++++++++++++++++++++++++ drivers/remoteproc/qcom_q6v5.h | 2 ++ 2 files changed, 33 insertions(+) diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 497acfb..89f5384 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -15,6 +15,7 @@ #include <linux/soc/qcom/smem.h> #include <linux/soc/qcom/smem_state.h> #include <linux/remoteproc.h> +#include <linux/delay.h> #include "qcom_common.h" #include "qcom_q6v5.h" @@ -94,6 +95,29 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) } EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); +static void qcom_q6v5_crash_handler_work(struct work_struct *work) +{ + struct qcom_q6v5 *q6v5 = container_of(work, struct qcom_q6v5, crash_handler); + struct rproc *rproc = q6v5->rproc; + struct rproc_subdev *subdev; + + mutex_lock(&rproc->lock); + + list_for_each_entry_reverse(subdev, &rproc->subdevs, node) { + if (subdev->stop) + subdev->stop(subdev, true); + } + + mutex_unlock(&rproc->lock); + + /* + * Temporary workaround until ramdump userspace application calls + * sync() and fclose() on attempting the dump. + */ + msleep(100); + panic("Panicking, remoteproc %s crashed\n", q6v5->rproc->name); +} + static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) { struct qcom_q6v5 *q6v5 = data; @@ -113,6 +137,9 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) dev_err(q6v5->dev, "watchdog without message\n"); q6v5->running = false; + if (q6v5->rproc->recovery_disabled) + schedule_work(&q6v5->crash_handler); + rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG); return IRQ_HANDLED; @@ -134,6 +161,9 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data) dev_err(q6v5->dev, "fatal error without message\n"); q6v5->running = false; + if (q6v5->rproc->recovery_disabled) + schedule_work(&q6v5->crash_handler); + rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR); return IRQ_HANDLED; @@ -354,6 +384,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, if (IS_ERR(q6v5->path)) return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), "failed to acquire interconnect path\n"); + INIT_WORK(&q6v5->crash_handler, qcom_q6v5_crash_handler_work); return 0; } diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h index 5a859c4..b1654be 100644 --- a/drivers/remoteproc/qcom_q6v5.h +++ b/drivers/remoteproc/qcom_q6v5.h @@ -29,6 +29,8 @@ struct qcom_q6v5 { int handover_irq; int stop_irq; + struct work_struct crash_handler; + bool handover_issued; struct completion start_done; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar @ 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:03 ` Peng Fan 2022-12-28 16:20 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-19 16:00 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar Subdevice notifications after a remoteproc has crashed are useful to any clients that might want to preserve data pertaining to the driver after the remoteproc crashed. Sending subdevice notifications before triggering a kernel panic gives these drivers the time to do collect this information. Change-Id: Id6e55fb038b70f54ff5854d2adff72b74b6a9570 Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> --- drivers/remoteproc/qcom_q6v5.c | 31 +++++++++++++++++++++++++++++++ drivers/remoteproc/qcom_q6v5.h | 2 ++ 2 files changed, 33 insertions(+) diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 497acfb..89f5384 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -15,6 +15,7 @@ #include <linux/soc/qcom/smem.h> #include <linux/soc/qcom/smem_state.h> #include <linux/remoteproc.h> +#include <linux/delay.h> #include "qcom_common.h" #include "qcom_q6v5.h" @@ -94,6 +95,29 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) } EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); +static void qcom_q6v5_crash_handler_work(struct work_struct *work) +{ + struct qcom_q6v5 *q6v5 = container_of(work, struct qcom_q6v5, crash_handler); + struct rproc *rproc = q6v5->rproc; + struct rproc_subdev *subdev; + + mutex_lock(&rproc->lock); + + list_for_each_entry_reverse(subdev, &rproc->subdevs, node) { + if (subdev->stop) + subdev->stop(subdev, true); + } + + mutex_unlock(&rproc->lock); + + /* + * Temporary workaround until ramdump userspace application calls + * sync() and fclose() on attempting the dump. + */ + msleep(100); + panic("Panicking, remoteproc %s crashed\n", q6v5->rproc->name); +} + static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) { struct qcom_q6v5 *q6v5 = data; @@ -113,6 +137,9 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) dev_err(q6v5->dev, "watchdog without message\n"); q6v5->running = false; + if (q6v5->rproc->recovery_disabled) + schedule_work(&q6v5->crash_handler); + rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG); return IRQ_HANDLED; @@ -134,6 +161,9 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data) dev_err(q6v5->dev, "fatal error without message\n"); q6v5->running = false; + if (q6v5->rproc->recovery_disabled) + schedule_work(&q6v5->crash_handler); + rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR); return IRQ_HANDLED; @@ -354,6 +384,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, if (IS_ERR(q6v5->path)) return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), "failed to acquire interconnect path\n"); + INIT_WORK(&q6v5->crash_handler, qcom_q6v5_crash_handler_work); return 0; } diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h index 5a859c4..b1654be 100644 --- a/drivers/remoteproc/qcom_q6v5.h +++ b/drivers/remoteproc/qcom_q6v5.h @@ -29,6 +29,8 @@ struct qcom_q6v5 { int handover_irq; int stop_irq; + struct work_struct crash_handler; + bool handover_issued; struct completion start_done; -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar @ 2022-09-23 7:03 ` Peng Fan 2022-12-28 16:20 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Peng Fan @ 2022-09-23 7:03 UTC (permalink / raw) To: Gokul krishna Krishnakumar, Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh > Subject: [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice > notifications before panic > > Subdevice notifications after a remoteproc has crashed are useful to any > clients that might want to preserve data pertaining to the driver after the > remoteproc crashed. Sending subdevice notifications before triggering a > kernel panic gives these drivers the time to do collect this information. > > Change-Id: Id6e55fb038b70f54ff5854d2adff72b74b6a9570 Change-Id should be removed. > Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> > --- > drivers/remoteproc/qcom_q6v5.c | 31 > +++++++++++++++++++++++++++++++ drivers/remoteproc/qcom_q6v5.h | > 2 ++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/remoteproc/qcom_q6v5.c > b/drivers/remoteproc/qcom_q6v5.c index 497acfb..89f5384 100644 > --- a/drivers/remoteproc/qcom_q6v5.c > +++ b/drivers/remoteproc/qcom_q6v5.c > @@ -15,6 +15,7 @@ > #include <linux/soc/qcom/smem.h> > #include <linux/soc/qcom/smem_state.h> > #include <linux/remoteproc.h> > +#include <linux/delay.h> Seq the head file order. > #include "qcom_common.h" > #include "qcom_q6v5.h" > > @@ -94,6 +95,29 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 > *q6v5) } EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); > > +static void qcom_q6v5_crash_handler_work(struct work_struct *work) { > + struct qcom_q6v5 *q6v5 = container_of(work, struct qcom_q6v5, > crash_handler); > + struct rproc *rproc = q6v5->rproc; > + struct rproc_subdev *subdev; > + > + mutex_lock(&rproc->lock); > + > + list_for_each_entry_reverse(subdev, &rproc->subdevs, node) { > + if (subdev->stop) > + subdev->stop(subdev, true); > + } > + > + mutex_unlock(&rproc->lock); > + > + /* > + * Temporary workaround until ramdump userspace application > calls > + * sync() and fclose() on attempting the dump. > + */ > + msleep(100); I have no knowledge on your system setup, but just guess this is not reliable. > + panic("Panicking, remoteproc %s crashed\n", q6v5->rproc->name); } It is remotecore crash, why use panic to crash the Linux side? > + > static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) { > struct qcom_q6v5 *q6v5 = data; > @@ -113,6 +137,9 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void > *data) > dev_err(q6v5->dev, "watchdog without message\n"); > > q6v5->running = false; > + if (q6v5->rproc->recovery_disabled) > + schedule_work(&q6v5->crash_handler); > + > rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG); > > return IRQ_HANDLED; > @@ -134,6 +161,9 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void > *data) > dev_err(q6v5->dev, "fatal error without message\n"); > > q6v5->running = false; > + if (q6v5->rproc->recovery_disabled) > + schedule_work(&q6v5->crash_handler); > + > rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR); > > return IRQ_HANDLED; > @@ -354,6 +384,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct > platform_device *pdev, > if (IS_ERR(q6v5->path)) > return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), > "failed to acquire interconnect path\n"); > + INIT_WORK(&q6v5->crash_handler, > qcom_q6v5_crash_handler_work); > > return 0; > } > diff --git a/drivers/remoteproc/qcom_q6v5.h > b/drivers/remoteproc/qcom_q6v5.h index 5a859c4..b1654be 100644 > --- a/drivers/remoteproc/qcom_q6v5.h > +++ b/drivers/remoteproc/qcom_q6v5.h > @@ -29,6 +29,8 @@ struct qcom_q6v5 { > int handover_irq; > int stop_irq; > > + struct work_struct crash_handler; > + > bool handover_issued; > > struct completion start_done; > -- > 2.7.4 Regards, Peng. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:03 ` Peng Fan @ 2022-12-28 16:20 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Bjorn Andersson @ 2022-12-28 16:20 UTC (permalink / raw) To: Gokul krishna Krishnakumar Cc: Andy Gross, Konrad Dybcio, Mathieu Poirier, linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh On Mon, Sep 19, 2022 at 09:00:38AM -0700, Gokul krishna Krishnakumar wrote: Looking back, I don't think I gave you proper feedback on this feature. Sorry about that Gokul. > Subdevice notifications after a remoteproc has crashed are useful to any > clients that might want to preserve data pertaining to the driver after the > remoteproc crashed. Sending subdevice notifications before triggering a > kernel panic gives these drivers the time to do collect this information. The elephant in the room here is the call to panic(), this deserves more attention in the commit messages. > > Change-Id: Id6e55fb038b70f54ff5854d2adff72b74b6a9570 Please remember to remove your Gerrit Change-Id when posting to the list. > Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> > --- > drivers/remoteproc/qcom_q6v5.c | 31 +++++++++++++++++++++++++++++++ > drivers/remoteproc/qcom_q6v5.h | 2 ++ > 2 files changed, 33 insertions(+) > > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c > index 497acfb..89f5384 100644 > --- a/drivers/remoteproc/qcom_q6v5.c > +++ b/drivers/remoteproc/qcom_q6v5.c > @@ -15,6 +15,7 @@ > #include <linux/soc/qcom/smem.h> > #include <linux/soc/qcom/smem_state.h> > #include <linux/remoteproc.h> > +#include <linux/delay.h> > #include "qcom_common.h" > #include "qcom_q6v5.h" > > @@ -94,6 +95,29 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5) > } > EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare); > > +static void qcom_q6v5_crash_handler_work(struct work_struct *work) > +{ > + struct qcom_q6v5 *q6v5 = container_of(work, struct qcom_q6v5, crash_handler); > + struct rproc *rproc = q6v5->rproc; > + struct rproc_subdev *subdev; > + > + mutex_lock(&rproc->lock); > + > + list_for_each_entry_reverse(subdev, &rproc->subdevs, node) { > + if (subdev->stop) > + subdev->stop(subdev, true); > + } > + > + mutex_unlock(&rproc->lock); > + > + /* > + * Temporary workaround until ramdump userspace application calls > + * sync() and fclose() on attempting the dump. > + */ I'm not able to see how this would happen, you only schedule_work() if rproc->recovery_disabled and the coredump generation will only happen if !rproc->recovery_disabled. Also, the reason I can see for invoking panic() here would be to gather a full system coredump to do post mortem analysis of all systems involved. So why would you capture a coredump as well? (This needs to be documented clearly...) > + msleep(100); > + panic("Panicking, remoteproc %s crashed\n", q6v5->rproc->name); As you might know, calling panic() is frowned upon. But I can see the benefit of being able to do full system post mortem analysis. I do however think that your patch indicates a shortcoming (to you) in the remoteproc_core's recovery logic. So please fix that shortcoming, rather than circumventing it in your driver. I.e. make it possible to get rproc_crash_handler_work() behave like you want it to, with a good motivation to why you want that. Regards, Bjorn > +} > + > static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) > { > struct qcom_q6v5 *q6v5 = data; > @@ -113,6 +137,9 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data) > dev_err(q6v5->dev, "watchdog without message\n"); > > q6v5->running = false; > + if (q6v5->rproc->recovery_disabled) > + schedule_work(&q6v5->crash_handler); > + > rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG); > > return IRQ_HANDLED; > @@ -134,6 +161,9 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data) > dev_err(q6v5->dev, "fatal error without message\n"); > > q6v5->running = false; > + if (q6v5->rproc->recovery_disabled) > + schedule_work(&q6v5->crash_handler); > + > rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR); > > return IRQ_HANDLED; > @@ -354,6 +384,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > if (IS_ERR(q6v5->path)) > return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path), > "failed to acquire interconnect path\n"); > + INIT_WORK(&q6v5->crash_handler, qcom_q6v5_crash_handler_work); > > return 0; > } > diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h > index 5a859c4..b1654be 100644 > --- a/drivers/remoteproc/qcom_q6v5.h > +++ b/drivers/remoteproc/qcom_q6v5.h > @@ -29,6 +29,8 @@ struct qcom_q6v5 { > int handover_irq; > int stop_irq; > > + struct work_struct crash_handler; > + > bool handover_issued; > > struct completion start_done; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar @ 2022-09-13 18:35 ` Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar ` (2 more replies) 2022-09-13 18:35 ` [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown Gokul krishna Krishnakumar ` (2 subsequent siblings) 4 siblings, 3 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-13 18:35 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar 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(-) 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar @ 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:05 ` Peng Fan 2022-12-28 16:22 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-19 16:00 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar 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(-) 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar @ 2022-09-23 7:05 ` Peng Fan 2022-12-28 16:22 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Peng Fan @ 2022-09-23 7:05 UTC (permalink / raw) To: Gokul krishna Krishnakumar, Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:05 ` Peng Fan @ 2022-12-28 16:22 ` Bjorn Andersson 2 siblings, 0 replies; 13+ messages in thread From: Bjorn Andersson @ 2022-12-28 16:22 UTC (permalink / raw) To: Gokul krishna Krishnakumar Cc: Andy Gross, Konrad Dybcio, Mathieu Poirier, linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar @ 2022-09-13 18:35 ` Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-19 16:00 ` [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar 2022-12-28 18:18 ` (subset) " Bjorn Andersson 4 siblings, 1 reply; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-13 18:35 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar Avoid setting smem bit in case of crash shutdown, as remote processor is not able to send the ack back. Change-Id: I33f19087627e5a7fe2c3bcce377b51b903574bc4 Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> --- drivers/remoteproc/qcom_q6v5.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 1b9e1e1..569427a 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -237,8 +237,10 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5, struct qcom_sysmon *sysmon) q6v5->running = false; - /* Don't perform SMP2P dance if sysmon already shut down the remote */ - if (qcom_sysmon_shutdown_acked(sysmon)) + /* Don't perform SMP2P dance if sysmon already shut + * down the remote or if it isn't running + */ + if (q6v5->rproc->state != RPROC_RUNNING || qcom_sysmon_shutdown_acked(sysmon)) return 0; qcom_smem_state_update_bits(q6v5->state, -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown 2022-09-13 18:35 ` [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown Gokul krishna Krishnakumar @ 2022-09-19 16:00 ` Gokul krishna Krishnakumar 0 siblings, 0 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-19 16:00 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar Avoid setting smem bit in case of crash shutdown, as remote processor is not able to send the ack back. Change-Id: I33f19087627e5a7fe2c3bcce377b51b903574bc4 Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com> --- drivers/remoteproc/qcom_q6v5.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c index 1b9e1e1..569427a 100644 --- a/drivers/remoteproc/qcom_q6v5.c +++ b/drivers/remoteproc/qcom_q6v5.c @@ -237,8 +237,10 @@ int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5, struct qcom_sysmon *sysmon) q6v5->running = false; - /* Don't perform SMP2P dance if sysmon already shut down the remote */ - if (qcom_sysmon_shutdown_acked(sysmon)) + /* Don't perform SMP2P dance if sysmon already shut + * down the remote or if it isn't running + */ + if (q6v5->rproc->state != RPROC_RUNNING || qcom_sysmon_shutdown_acked(sysmon)) return 0; qcom_smem_state_update_bits(q6v5->state, -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v1 0/3] Handle coprocessor crash 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar ` (2 preceding siblings ...) 2022-09-13 18:35 ` [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown Gokul krishna Krishnakumar @ 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-12-28 18:18 ` (subset) " Bjorn Andersson 4 siblings, 0 replies; 13+ messages in thread From: Gokul krishna Krishnakumar @ 2022-09-19 16:00 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Trilok Soni, Satya Durga Srinivasu Prabhala, Rajendra Nayak, Elliot Berman, Guru Das Srinagesh, Gokul krishna Krishnakumar Make the following changes in case of coprocessor crash: 1. Send subdevice notifications before panic 2. Do not report crash if SSR is disabled 3. Avoid setting smem bit in case of crash Gokul krishna Krishnakumar (3): remoteproc: qcom: q6v5: Send subdevice notifications before panic remoteproc: qcom: q6v5: Do not report crash if SSR is disabled remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown drivers/remoteproc/qcom_q6v5.c | 43 ++++++++++++++++++++++++++++++++++++++---- drivers/remoteproc/qcom_q6v5.h | 2 ++ 2 files changed, 41 insertions(+), 4 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: (subset) [PATCH v1 0/3] Handle coprocessor crash 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar ` (3 preceding siblings ...) 2022-09-19 16:00 ` [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar @ 2022-12-28 18:18 ` Bjorn Andersson 4 siblings, 0 replies; 13+ messages in thread From: Bjorn Andersson @ 2022-12-28 18:18 UTC (permalink / raw) To: quic_gokukris, mathieu.poirier, Bjorn Andersson, konrad.dybcio, agross Cc: quic_tsoni, quic_satyap, quic_eberman, quic_gurus, linux-arm-msm, quic_rjendra, linux-remoteproc, linux-kernel On Tue, 13 Sep 2022 11:35:41 -0700, Gokul krishna Krishnakumar wrote: > Make the following changes in case of coprocessor crash: > 1. Send subdevice notifications before panic > 2. Do not report crash if SSR is disabled > 3. Avoid setting smem bit in case of crash > > Gokul krishna Krishnakumar (3): > remoteproc: qcom: q6v5: Send subdevice notifications before panic > remoteproc: qcom: q6v5: Do not report crash if SSR is disabled > remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash > shutdown > > [...] Applied, thanks! [3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown commit: 3cc889eb83f59b5a6a869a685da11f79ffbb4e4d Best regards, -- Bjorn Andersson <andersson@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-28 18:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar 2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:03 ` Peng Fan 2022-12-28 16:20 ` Bjorn Andersson 2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-23 7:05 ` Peng Fan 2022-12-28 16:22 ` Bjorn Andersson 2022-09-13 18:35 ` [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown Gokul krishna Krishnakumar 2022-09-19 16:00 ` Gokul krishna Krishnakumar 2022-09-19 16:00 ` [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar 2022-12-28 18:18 ` (subset) " Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).