[PATCH v2] remoteproc: core: Fetch the auto-boot firmware only once

[Yonghao Zhang <hyz3367@gmail.com>]

Auto-boot for an always-on remote processor registers an asynchronous
request_firmware_nowait() whose callback, rproc_auto_boot_callback(),
calls rproc_boot() first and only releases the fetched image at the
very end of the callback.  rproc_boot() does not take this image as an
argument: it fetches the very same image again with a synchronous
request_firmware(), so every auto-boot reads the firmware image twice
and allocates the buffer twice; on kernels with the sysfs fallback
enabled, the uevent round-trip is repeated as well.

The asynchronous request exists only so that rproc_add() does not
block on the filesystem read; the image it fetches is never used.
The core already defers rproc_boot() to a worker for detached
processors (attach_work).  Reuse that worker for offline processors
too and drop the asynchronous firmware request: rproc_boot() fetches
the image exactly once and dispatches between a firmware boot and
an attach based on the processor state.  The work is renamed to
boot_work to match its widened role.

commit 400e64df6b23 ("remoteproc: add framework for controlling remote
processors") noted back in 2011 that "we must wait until it completes
before we try to unregister the device".  rproc_del() now does exactly
that: it waits for the boot work with cancel_work_sync(), which also
closes the theoretical window in which a pending attach work could
outlive the rproc instance.

The changed fallback behaviour only matters for legacy
configurations.  udev dropped its userspace firmware loader back in
2014, as recorded in
Documentation/driver-api/firmware/fallback-mechanisms.rst, and the
kernel has documented since commit 02c399306826 ("firmware_loader:
enhance Kconfig documentation over FW_LOADER") that "Linux no longer
relies on or uses a fallback mechanism in userspace".  Auto-boot now
uses the same synchronous fallback semantics as every other explicit
boot source (sysfs, cdev).

Signed-off-by: Yonghao Zhang <hyz3367@gmail.com>
---
Changes in v2:
- Fix the order described in the changelog: the callback calls
  rproc_boot() first and releases the asynchronously fetched image at
  the end of the callback (Mathieu).
- Fix typo "proccessor".

 drivers/remoteproc/remoteproc_core.c | 65 ++++++++--------------------
 include/linux/remoteproc.h           |  4 +-
 2 files changed, 21 insertions(+), 48 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 263e12f022ea..f329dc478170 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1662,49 +1662,26 @@ static int rproc_attach(struct rproc *rproc)
 }
 
 /*
- * take a firmware and boot it up.
- *
- * Note: this function is called asynchronously upon registration of the
- * remote processor (so we must wait until it completes before we try
- * to unregister the device. one other option is just to use kref here,
- * that might be cleaner).
+ * Boot or attach the remote processor in the background, on behalf of
+ * rproc_trigger_auto_boot(): rproc_add() runs in probe context and must
+ * not block while the firmware image is read from storage.  rproc_boot()
+ * dispatches on the processor state, so this covers both a firmware boot
+ * and an attach to a processor started by another entity.
+ *
+ * Note: rproc_del() waits for this work to complete with
+ * cancel_work_sync(), so the rproc instance remains valid for the
+ * entire lifetime of this function.
  */
-static void rproc_auto_boot_callback(const struct firmware *fw, void *context)
+static void rproc_boot_work(struct work_struct *work)
 {
-	struct rproc *rproc = context;
+	struct rproc *rproc = container_of(work, struct rproc, boot_work);
 
 	rproc_boot(rproc);
-
-	release_firmware(fw);
 }
 
-static void rproc_attach_work(struct work_struct *work)
+static void rproc_trigger_auto_boot(struct rproc *rproc)
 {
-	struct rproc *rproc = container_of(work, struct rproc, attach_work);
-
-	rproc_boot(rproc);
-}
-
-static int rproc_trigger_auto_boot(struct rproc *rproc)
-{
-	int ret;
-
-	if (rproc->state == RPROC_DETACHED) {
-		schedule_work(&rproc->attach_work);
-		return 0;
-	}
-
-	/*
-	 * We're initiating an asynchronous firmware loading, so we can
-	 * be built-in kernel code, without hanging the boot process.
-	 */
-	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
-				      rproc->firmware, &rproc->dev, GFP_KERNEL,
-				      rproc, rproc_auto_boot_callback);
-	if (ret < 0)
-		dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
-
-	return ret;
+	schedule_work(&rproc->boot_work);
 }
 
 static int rproc_stop(struct rproc *rproc, bool crashed)
@@ -2348,11 +2325,8 @@ int rproc_add(struct rproc *rproc)
 	rproc_create_debug_dir(rproc);
 
 	/* if rproc is marked always-on, request it to boot */
-	if (rproc->auto_boot) {
-		ret = rproc_trigger_auto_boot(rproc);
-		if (ret < 0)
-			goto rproc_remove_dev;
-	}
+	if (rproc->auto_boot)
+		rproc_trigger_auto_boot(rproc);
 
 	/* expose to rproc_get_by_phandle users */
 	mutex_lock(&rproc_list_mutex);
@@ -2361,10 +2335,6 @@ int rproc_add(struct rproc *rproc)
 
 	return 0;
 
-rproc_remove_dev:
-	cancel_work_sync(&rproc->crash_handler);
-	rproc_delete_debug_dir(rproc);
-	device_del(dev);
 rproc_remove_cdev:
 	rproc_char_device_remove(rproc);
 	return ret;
@@ -2552,7 +2522,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->subdevs);
 	INIT_LIST_HEAD(&rproc->dump_segments);
 
-	INIT_WORK(&rproc->attach_work, rproc_attach_work);
+	INIT_WORK(&rproc->boot_work, rproc_boot_work);
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	spin_lock_init(&rproc->crash_handler_lock);
 
@@ -2630,6 +2600,9 @@ int rproc_del(struct rproc *rproc)
 	if (cancel_work_sync(&rproc->crash_handler))
 		pm_relax(rproc->dev.parent);
 
+	/* auto-boot may still be fetching firmware: wait for it here */
+	cancel_work_sync(&rproc->boot_work);
+
 	__rproc_shutdown(rproc, true);
 
 	rproc_delete_debug_dir(rproc);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index a44368737b39..d77e24539133 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -231,7 +231,7 @@ enum rproc_features {
  * @subdevs: list of subdevices, to following the running state
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
  * @index: index of this rproc device
- * @attach_work: workqueue for attaching rproc
+ * @boot_work: workqueue for booting rproc
  * @crash_handler: workqueue for handling a crash
  * @crash_handler_lock: serializes crash handler queueing and deletion
  * @deleting: remoteproc deletion has begun
@@ -277,7 +277,7 @@ struct rproc {
 	struct list_head subdevs;
 	struct idr notifyids;
 	int index;
-	struct work_struct attach_work;
+	struct work_struct boot_work;
 	struct work_struct crash_handler;
 	spinlock_t crash_handler_lock;
 	bool deleting;
-- 
2.34.1