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

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

[Yonghao Zhang]

Auto-boot for an always-on remote processor registers an asynchronous
request_firmware_nowait() whose callback discards the fetched image
and calls rproc_boot().  rproc_boot() then 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 proccessor 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>
---
 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

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

[Mathieu Poirier]

Good day,

On Wed, Sep 23, 2026 at 10:58:55AM +0800, Yonghao Zhang wrote:
> Auto-boot for an always-on remote processor registers an asynchronous
> request_firmware_nowait() whose callback discards the fetched image

The callback function is rproc_auto_boot_callback() but I don't see where the
image fetched by request_firmware_nowait() is discarded.

Otherwise I agree with this patch.

Thanks,
Mathieu

> and calls rproc_boot().  rproc_boot() then 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 proccessor 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>
> ---
>  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
> 

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

[Yonghao Zhang]

Hi Mathieu,

Thanks for the review.

On Mon, Sep 28, 2026 at 11:34:14AM -0600, Mathieu Poirier wrote:
> Good day,
> 
> On Wed, Sep 23, 2026 at 10:58:55AM +0800, Yonghao Zhang wrote:
> > Auto-boot for an always-on remote processor registers an asynchronous
> > request_firmware_nowait() whose callback discards the fetched image
> 
> The callback function is rproc_auto_boot_callback() but I don't see where the
> image fetched by request_firmware_nowait() is discarded.

It is the release_firmware(fw) called at the end of this callback.
The image fetched by request_firmware_nowait() is never consumed:
rproc_boot() doesn't take it as an argument and fetches it's own copy
with a synchronous request_firmware(). It is that second copy which
rproc_fw_boot() parses and which is release at the end of rproc_boot().
The asynchronous copy is only ever touched by the final
release_firmware(fw) -- fetched, never read, freed.

"Discarded" in the commit message refers to exactly that. I can send
a v2 that names this release_firmware() explicitly in the first
paragraph if you want the message to be verifiable at a glance.

Thanks,
Yonghao

> 
> Otherwise I agree with this patch.
> 
> Thanks,
> Mathieu
> 
> > and calls rproc_boot().  rproc_boot() then 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 proccessor 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>
> > ---
> >  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
> > 

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

[Mathieu Poirier]

On Mon, 28 Sept 2026 at 22:43, Yonghao Zhang <hyz3367@gmail.com> wrote:
>
> Hi Mathieu,
>
> Thanks for the review.
>
> On Mon, Sep 28, 2026 at 11:34:14AM -0600, Mathieu Poirier wrote:
> > Good day,
> >
> > On Wed, Sep 23, 2026 at 10:58:55AM +0800, Yonghao Zhang wrote:
> > > Auto-boot for an always-on remote processor registers an asynchronous
> > > request_firmware_nowait() whose callback discards the fetched image
> >
> > The callback function is rproc_auto_boot_callback() but I don't see where the
> > image fetched by request_firmware_nowait() is discarded.
>
> It is the release_firmware(fw) called at the end of this callback.

Right, this is happening at the _end_ of the callback, but the
changelog says the firmware is discarded and then rproc_boot() called.
This is not accurate and needs to be fixed.

> The image fetched by request_firmware_nowait() is never consumed:
> rproc_boot() doesn't take it as an argument and fetches it's own copy
> with a synchronous request_firmware(). It is that second copy which
> rproc_fw_boot() parses and which is release at the end of rproc_boot().
> The asynchronous copy is only ever touched by the final
> release_firmware(fw) -- fetched, never read, freed.
>
> "Discarded" in the commit message refers to exactly that. I can send
> a v2 that names this release_firmware() explicitly in the first
> paragraph if you want the message to be verifiable at a glance.
>
> Thanks,
> Yonghao
>
> >
> > Otherwise I agree with this patch.
> >
> > Thanks,
> > Mathieu
> >
> > > and calls rproc_boot().  rproc_boot() then 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 proccessor 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>
> > > ---
> > >  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
> > >