[PATCH 3/4] remoteproc: core: Roll a failed attach back with detach() when available

[Yonghao Zhang <hyz3367@gmail.com>]

When the subdevice registration that follows ops->attach() fails,
__rproc_attach() rolls back with an unconditional ops->stop() call.
That is wrong on three counts.

An attach-only implementation, one without stop() such as
commit 1168af40b1ad ("remoteproc: k3-r5: Add support for IPC-only
mode for all R5Fs"), dereferences NULL right there.  A processor that
is being attached to was started by another entity and is not ours to
power off: detach() is the matching undo of attach(), and stop()
should only be used as a last resort, when there is no detach() or
it fails.  This is reachable today: when the re-attach of an
RPROC_FEAT_ATTACH_ON_RECOVERY processor fails (imx_rproc and
xlnx_r5 use the feature), the unwind stops a processor which,
per the feature's own contract, "does not need help from Linux to
recover... Linux just needs to attach".  And the unwind leaves
the accounting inconsistent -- no resource table bookkeeping
is done, unlike on the rproc_stop() and __rproc_detach() paths,
and a processor powered off through the fallback keeps its
RPROC_DETACHED state, so the next rproc_boot() tries to attach to a
core that is no longer running.

Roll the attach back with detach() first, along with the same
rproc_reset_rsc_table_on_detach() bookkeeping __rproc_detach() does,
and fall back to stop() only when detach() is unavailable or failed.
A failed resource table reset does not abort the unwind: this is an
error path, and detaching from the remote processor, or powering it
off as the last resort, takes precedence over the bookkeeping.  A
successful fallback moves the processor to RPROC_OFFLINE so the next
boot reloads firmware instead of attaching to a dead core;
implementations with neither handler keep the processor running and
untouched, which is all an attach-only core needs.  The rollback is
factored into rproc_unwind_attach().

The unwind runs the same resource table resets as the detach and
stop paths, which free clean_table and leave a cached copy of the
installed table in rproc->cached_table.  Make rproc_attach()'s error
cleanup, which runs right after, null clean_table after freeing it
and release that copy along with table_ptr, or a failed attach
double-frees clean_table and leaks the copy.

Fixes: d848a4819d85 ("remoteproc: Introducing function rproc_attach()")
Signed-off-by: Yonghao Zhang <hyz3367@gmail.com>
---
 drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 19e0ea3e7240..c52212a1d180 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1348,6 +1348,59 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
+static int rproc_reset_rsc_table_on_detach(struct rproc *rproc);
+static int rproc_reset_rsc_table_on_stop(struct rproc *rproc);
+
+/*
+ * Undo an attach whose subdevice registration failed.  The remote
+ * processor was started by another entity and is not ours to power
+ * off, so roll back with detach() when available and fall back to
+ * stop() when there is no detach() or when it failed.  stop() is
+ * the only rollback that does not rely on the remote side.  A
+ * processor powered off through the fallback is marked RPROC_OFFLINE,
+ * so the next boot reloads firmware instead of attaching to a dead
+ * core; with neither handler there is nothing to roll back with and
+ * the processor is left running.
+ *
+ * The resource table resets are best-effort: when one fails, the
+ * unwind carries on with detach()/stop() anyway.  Unlike
+ * __rproc_detach() and rproc_stop(), which bail out before touching
+ * the processor when their reset fails, this is already an error
+ * path, and detaching the remote processor -- or, failing that,
+ * powering it off -- is the minimum it must still deliver.
+ */
+static void rproc_unwind_attach(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	if (rproc->ops->detach) {
+		ret = rproc_reset_rsc_table_on_detach(rproc);
+		if (ret)
+			dev_err(dev, "can't reset rsc table on detach: %d\n",
+				ret);
+
+		ret = rproc->ops->detach(rproc);
+		if (!ret)
+			return;
+
+		dev_err(dev, "can't detach from rproc %s: %d\n",
+			rproc->name, ret);
+	}
+
+	if (rproc->ops->stop) {
+		ret = rproc_reset_rsc_table_on_stop(rproc);
+		if (ret)
+			dev_err(dev, "can't reset rsc table on stop: %d\n",
+				ret);
+
+		if (rproc->ops->stop(rproc))
+			dev_err(dev, "can't stop rproc %s\n", rproc->name);
+		else
+			rproc->state = RPROC_OFFLINE;
+	}
+}
+
 static int __rproc_attach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
@@ -1373,7 +1426,7 @@ static int __rproc_attach(struct rproc *rproc)
 	if (ret) {
 		dev_err(dev, "failed to probe subdevices for %s: %d\n",
 			rproc->name, ret);
-		goto stop_rproc;
+		goto unwind_attach;
 	}
 
 	rproc->state = RPROC_ATTACHED;
@@ -1382,8 +1435,8 @@ static int __rproc_attach(struct rproc *rproc)
 
 	return 0;
 
-stop_rproc:
-	rproc->ops->stop(rproc);
+unwind_attach:
+	rproc_unwind_attach(rproc);
 unprepare_subdevices:
 	rproc_unprepare_subdevices(rproc);
 out:
@@ -1562,6 +1615,7 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
 	 * rproc_set_rsc_table().
 	 */
 	kfree(rproc->clean_table);
+	rproc->clean_table = NULL;
 
 	return 0;
 }
@@ -1597,6 +1651,7 @@ static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
 	 * won't be needed.  Allocated in rproc_set_rsc_table().
 	 */
 	kfree(rproc->clean_table);
+	rproc->clean_table = NULL;
 
 out:
 	/*
@@ -1675,6 +1730,10 @@ static int rproc_attach(struct rproc *rproc)
 	/* release HW resources if needed */
 	rproc_unprepare_device(rproc);
 	kfree(rproc->clean_table);
+	rproc->clean_table = NULL;
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+	rproc->table_ptr = NULL;
 disable_iommu:
 	rproc_disable_iommu(rproc);
 	return ret;
-- 
2.34.1