[PATCH] remoteproc: qcom: pas: Request stop before tearing down GLINK and SMD

[Jure Rebernik]

A crash of the ADSP that the host reports, for example through the
remoteproc debugfs crash file, can reset the whole SoC on QCS8550
instead of restarting the ADSP. The reset was most frequent shortly
after the ADSP had started: on two QCS8550-based boards, crashing it
about 4.5 seconds after it was started reset the SoC in 19 of 42
tries. The serial console showed that the system stopped right after
the recovery started. Nothing more was logged, and the SoC was reset 6
to 7 seconds later.

Since commit 0ea50486978f ("remoteproc: qcom: q6v5: Request shutdown if
crash is triggered host-side"), qcom_q6v5_request_stop() also sets the
SMP2P stop bit for a crash that the host reports, so that the firmware
takes part in its shutdown. The stop is requested from qcom_pas_stop(),
which rproc_stop() calls only after all subdevices have stopped. By
then the GLINK and SMD edges to the DSP have been unregistered and
their client devices, such as FastRPC, torn down. On QCS8550 the ADSP
firmware treats the stop bit as a fatal error and runs its error
handler after these links have gone away.

Add a subdevice whose .stop callback requests the stop, and add it
before sysmon. Subdevices stop in the reverse order of addition, so the
callback runs after the SSR BEFORE_SHUTDOWN notification and
sysmon_stop(), and before SMD and GLINK are torn down. The firmware
then handles the stop request while its GLINK and SMD edges are still
registered. qcom_q6v5_request_stop() still skips the request when
sysmon got a shutdown ack, or when the firmware reported the crash
itself and the fatal or watchdog interrupt has cleared the running
flag. It also clears the running flag, so the call in qcom_pas_stop()
now only sends a request when the subdevices were not started.

With this change, none of 104 host-reported crashes on the same boards
reset the SoC: 40 ADSP crashes at 4.5 seconds, 20 at 4 to 15 seconds,
20 while the ADSP was streaming IMU sensor data and 24 CDSP crashes.
The ADSP or CDSP restarted each time.

The problem was found, and this change was written and tested, with
the help of an AI coding assistant (Claude Code, by Anthropic). I
reviewed the change and the test results.

Fixes: 0ea50486978f ("remoteproc: qcom: q6v5: Request shutdown if crash is triggered host-side")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jure Rebernik <jure.rebernik@xxxxxxxxxxx>
---

Notes:
    Tested on Luxonis OAK4-S boards (QCS8550), ADSP and CDSP, with a kernel
    based on v7.2.6, which has 0ea50486978f as stable commit 0dc9e289bbb5.
    The crashes were triggered through the debugfs crash file. To see when
    the system stopped, a test script wrote a kernel log line to the serial
    console 20 times a second. A crash that the firmware reports could not
    be injected.
    
    Graceful stops through sysfs also reset these boards occasionally, with
    and without this patch (2 of 300 stops with it, 2 of about 290 without
    it). That problem is unrelated to this patch, and I am looking into it
    separately. This patch does not change that case: the firmware
    acknowledged the sysmon shutdown request, so no stop request was sent.
    
    qcom_q6v5_adsp and qcom_q6v5_mss also call qcom_q6v5_request_stop()
    from their .stop callbacks, after the subdevices have stopped, and
    qcom_q6v5_wcss does when requires_force_stop is set. I have no hardware
    for them and left them unchanged. If the approach is acceptable, they
    could get the same change.
    
    Built for arm64 (defconfig, drivers/remoteproc/, W=1) on rproc-next
    672126dce63c and on v7.4-rc1, without warnings. Not done: sparse and
    runtime tests with lockdep and the other debug options.
    
    Tool use: I asked Claude Code to make our QCS8550 image stable, to
    stress it on our test boards and to find why a simulated ADSP crash
    reset the board. It ran the tests, found the ordering problem and wrote
    a first fix that skipped the stop request for a crashed DSP. I asked
    for the stop request to be sent before the teardown instead, which is
    this patch. It wrote the code and this changelog and ran the tests
    above with the debugfs crash file, the serial console log and a test
    script on the boards. I reviewed the code and the results.

 drivers/remoteproc/qcom_q6v5_pas.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 57a111a0d..ddbffe45d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -138,6 +138,7 @@ struct qcom_pas {
 	struct qcom_rproc_glink glink_subdev;
 	struct qcom_rproc_subdev smd_subdev;
 	struct qcom_rproc_pdm pdm_subdev;
+	struct rproc_subdev stop_subdev;
 	struct qcom_rproc_ssr ssr_subdev;
 	struct qcom_sysmon *sysmon;
 	struct platform_device *bam_dmux;
@@ -429,6 +430,14 @@ static void qcom_pas_handover(struct qcom_q6v5 *q6v5)
 	qcom_pas_pds_disable(pas, pas->proxy_pds, pas->proxy_pd_count);
 }
 
+static void qcom_pas_stop_subdev(struct rproc_subdev *subdev, bool crashed)
+{
+	struct qcom_pas *pas = container_of(subdev, struct qcom_pas, stop_subdev);
+
+	if (qcom_q6v5_request_stop(&pas->q6v5, pas->sysmon) == -ETIMEDOUT)
+		dev_err(pas->dev, "timed out on wait\n");
+}
+
 static int qcom_pas_stop(struct rproc *rproc)
 {
 	struct qcom_pas *pas = rproc->priv;
@@ -1012,6 +1021,15 @@ static int qcom_pas_probe(struct platform_device *pdev)
 	qcom_add_glink_subdev(rproc, &pas->glink_subdev, desc->ssr_name);
 	qcom_add_smd_subdev(rproc, &pas->smd_subdev);
 	qcom_add_pdm_subdev(rproc, &pas->pdm_subdev);
+
+	/*
+	 * Subdevices stop in the reverse order of addition. Request the stop
+	 * after sysmon has asked for a graceful shutdown and before smd and
+	 * glink are torn down.
+	 */
+	pas->stop_subdev.stop = qcom_pas_stop_subdev;
+	rproc_add_subdev(rproc, &pas->stop_subdev);
+
 	pas->sysmon = qcom_add_sysmon_subdev(rproc, desc->sysmon_name, desc->ssctl_id);
 	if (IS_ERR(pas->sysmon)) {
 		ret = PTR_ERR(pas->sysmon);
@@ -1049,6 +1067,7 @@ static int qcom_pas_probe(struct platform_device *pdev)
 	qcom_remove_ssr_subdev(rproc, &pas->ssr_subdev);
 	qcom_remove_sysmon_subdev(pas->sysmon);
 deinit_remove_pdm_smd_glink:
+	rproc_remove_subdev(rproc, &pas->stop_subdev);
 	qcom_remove_pdm_subdev(rproc, &pas->pdm_subdev);
 	qcom_remove_smd_subdev(rproc, &pas->smd_subdev);
 	qcom_remove_glink_subdev(rproc, &pas->glink_subdev);
@@ -1081,6 +1100,7 @@ static void qcom_pas_remove(struct platform_device *pdev)
 	qcom_remove_sysmon_subdev(pas->sysmon);
 	qcom_remove_smd_subdev(pas->rproc, &pas->smd_subdev);
 	qcom_remove_pdm_subdev(pas->rproc, &pas->pdm_subdev);
+	rproc_remove_subdev(pas->rproc, &pas->stop_subdev);
 	qcom_remove_ssr_subdev(pas->rproc, &pas->ssr_subdev);
 	qcom_pas_pds_detach(pas, pas->proxy_pds, pas->proxy_pd_count);
 	device_init_wakeup(pas->dev, false);

base-commit: 672126dce63c94e7897867e4c0e681806c49cf02
-- 
2.56.0