[PATCH 4/4] remoteproc: core: Clean up after a failed recovery

[Yonghao Zhang]

When rproc_boot_recovery() fails to bring a crashed processor back
(the firmware request fails, or rproc_start() fails), it returns
with the processor stopped but two kinds of state still held.

The resources of the boot the recovery was trying to restore are
never released: rproc_stop() does not clean them up, and unlike
rproc_attach_recovery(), which releases everything when its
re-attach fails, boot_recovery just returned.  Stale carveout
entries then fail every later firmware boot at "already associated
to resource table", until one of those failing boots happens to run
the cleanup of rproc_fw_boot().

The power references fare worse: nothing can release them anymore.
Take a processor with two outstanding rproc_boot() references whose
recovery stops it and then fails to restart it -- RPROC_OFFLINE,
with the count still at two.  rproc_shutdown() drops exactly one
reference per call, and only once past its state gate; the gate
admits RPROC_RUNNING, RPROC_ATTACHED and RPROC_CRASHED, so an
offline processor never passes and no amount of shutdown() calls
releases anything.  With the count still above zero, rproc_boot()
then short-circuits on atomic_inc_return(&rproc->power) > 1 and
returns success without doing anything: the users of a dead
processor are told it is running.  The reference count and the
state machine are misaligned for good.

Release the resources on the failure paths of rproc_boot_recovery(),
the same way rproc_shutdown() does, and void the power count in
rproc_trigger_recovery() when the recovery failed without leaving
the processor crashed, offline or detached.  The service the count
was tracking is gone, so every outstanding reference is dead, which
decrementing instead would leave the survivors stranded exactly as
above.  A processor that is still crashed keeps its references, as
rproc_shutdown() can still drain them in that state.

Fixes: ba194232edc0 ("remoteproc: Support attach recovery after rproc crash")
Signed-off-by: Yonghao Zhang <hyz3367@xxxxxxxxx>
---
 drivers/remoteproc/remoteproc_core.c | 34 +++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c52212a1d180..b138680b1905 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1900,7 +1900,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
 	ret = request_firmware(&firmware_p, rproc->firmware, dev);
 	if (ret < 0) {
 		dev_err(dev, "request_firmware failed: %d\n", ret);
-		return ret;
+		goto clean_up_resources;
 	}
 
 	/* boot the remote processor up again */
@@ -1908,6 +1908,24 @@ static int rproc_boot_recovery(struct rproc *rproc)
 
 	release_firmware(firmware_p);
 
+	if (ret < 0)
+		goto clean_up_resources;
+
+	return 0;
+
+clean_up_resources:
+	/*
+	 * rproc_stop() has already switched the remote processor off, but
+	 * unlike rproc_shutdown() nothing releases the resources of the
+	 * boot this recovery was trying to restore.
+	 */
+	rproc_resource_cleanup(rproc);
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+	rproc->table_ptr = NULL;
+	/* release HW resources if needed */
+	rproc_unprepare_device(rproc);
+	rproc_disable_iommu(rproc);
 	return ret;
 }
 
@@ -1948,6 +1966,20 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	else
 		ret = rproc_boot_recovery(rproc);
 
+	/*
+	 * A failed recovery leaves the remote processor in a state from which
+	 * rproc_shutdown() refuses to release the outstanding power references
+	 * (RPROC_OFFLINE or RPROC_DETACHED), so every rproc_boot() would
+	 * free-ride on them and silently do nothing. The service those
+	 * references were tracking is gone: void them all.  Failures that
+	 * leave the processor crashed keep the references, as rproc_shutdown()
+	 * can still drain them in that state.
+	 */
+	if (ret && rproc->state != RPROC_CRASHED) {
+		dev_err(dev, "failed to recover %s: %d\n", rproc->name, ret);
+		atomic_set(&rproc->power, 0);
+	}
+
 unlock_mutex:
 	mutex_unlock(&rproc->lock);
 	return ret;
-- 
2.34.1