[PATCH v2 0/3] remoteproc: improve robustness for rproc_attach fail cases

[PATCH v2 0/3] remoteproc: improve robustness for rproc_attach fail cases

[Jingyi Wang]

Failure in rproc_attach() path can cause issues like NULL pointer
dereference when doing recovery and rproc_add() fail, improve the
robustness for rproc_attach fail cases.

Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
---
Changes in v2:
- commit msg update
- Introduce subdevs_started flag instead of check nullptr
- Add change to cancel crash_handler work
- Link to v1: https://lore.kernel.org/r/20260409-rproc-attach-issue-v1-0-088a1c348e7a@oss.qualcomm.com

---
Jingyi Wang (3):
      remoteproc: core: Attach rproc asynchronously in rproc_add() path via schedule_work()
      remoteproc: core: Check subdev start status in rproc_stop()
      remoteproc: core: cancel crash_handler work in rproc_add() error path

 drivers/remoteproc/remoteproc_core.c | 25 ++++++++++++++++---------
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 20 insertions(+), 9 deletions(-)
---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260518-rproc-attach-issue-5d26d820ab80

Best regards,
-- 
Jingyi Wang <jingyi.wang@oss.qualcomm.com>

[PATCH v2 1/3] remoteproc: core: Attach rproc asynchronously in rproc_add() path via schedule_work()

[Jingyi Wang]

Unlike the remoteproc firmware load path where rproc_add() call
rproc_auto_boot_callback() asynchronously and ignores the return value of
rproc_boot(), the attach path calls rproc_boot() synchronously and
propagates its return value back to rproc_add(). This means a failure
during rproc_attach() causes rproc_add() to fail and triggers resource
release, removing the remoteproc from sysfs and making it unavailable for
recovery or further boot attempts.

Align the remoteproc attach path with the firmware load path by
introducing attach_work and scheduling rproc_boot() asynchronously via
schedule_work(). This keeps the remoteproc registered and available in
sysfs even if the initial attach attempt fails, and avoids blocking
rproc_add() on the attach result.

Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c | 20 ++++++++++++--------
 include/linux/remoteproc.h           |  2 ++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index b087ed21858a..f02db1113fae 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1673,18 +1673,21 @@ static void rproc_auto_boot_callback(const struct firmware *fw, void *context)
 	release_firmware(fw);
 }
 
+static void rproc_attach_work(struct work_struct *work)
+{
+	struct rproc *rproc = container_of(work, struct rproc, attach_work);
+
+	rproc_boot(rproc);
+}
+
 static int rproc_trigger_auto_boot(struct rproc *rproc)
 {
 	int ret;
 
-	/*
-	 * Since the remote processor is in a detached state, it has already
-	 * been booted by another entity.  As such there is no point in waiting
-	 * for a firmware image to be loaded, we can simply initiate the process
-	 * of attaching to it immediately.
-	 */
-	if (rproc->state == RPROC_DETACHED)
-		return rproc_boot(rproc);
+	if (rproc->state == RPROC_DETACHED) {
+		schedule_work(&rproc->attach_work);
+		return 0;
+	}
 
 	/*
 	 * We're initiating an asynchronous firmware loading, so we can
@@ -2512,6 +2515,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->dump_segments);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
+	INIT_WORK(&rproc->attach_work, rproc_attach_work);
 
 	rproc->state = RPROC_OFFLINE;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b4795698d8c2..580d324a1e8f 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -526,6 +526,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
  * @crash_handler: workqueue for handling a crash
  * @crash_cnt: crash counter
  * @recovery_disabled: flag that state if recovery was disabled
@@ -568,6 +569,7 @@ struct rproc {
 	struct list_head subdevs;
 	struct idr notifyids;
 	int index;
+	struct work_struct attach_work;
 	struct work_struct crash_handler;
 	unsigned int crash_cnt;
 	bool recovery_disabled;

-- 
2.34.1

[PATCH v2 2/3] remoteproc: core: Check subdev start status in rproc_stop()

[Jingyi Wang]

For rproc that doing attach, rproc_start_subdevices() is called only when
attach successfully. If rproc_report_crash() is called in the attach
function, rproc_boot_recovery()->rproc_stop()->rproc_stop_subdevices()->
glink_subdev_stop() could be called and cause NULL pointer dereference:

 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000300
 Mem abort info:
 ...
 pc : qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem]
 lr : glink_subdev_stop+0x1c/0x30 [qcom_common]
 ...
 Call trace:
  qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem] (P)
  glink_subdev_stop+0x1c/0x30 [qcom_common]
  rproc_stop+0x58/0x17c
  rproc_trigger_recovery+0xb0/0x150
  rproc_crash_handler_work+0xa4/0xc4
  process_scheduled_works+0x18c/0x2d8
  worker_thread+0x144/0x280
  kthread+0x124/0x138
  ret_from_fork+0x10/0x20
 Code: a9be7bfd 910003fd a90153f3 aa0003f3 (b9430000)
 ---[ end trace 0000000000000000 ]---

Introduce "subdevs_started" flag to indicate rproc_start_subdevices() has
been called successfully. Ensure subdevices are only stopped if they have
been started.

Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c | 4 +++-
 include/linux/remoteproc.h           | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f02db1113fae..6e23cb11e515 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1308,6 +1308,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 		goto stop_rproc;
 	}
 
+	rproc->subdevs_started = true;
 	rproc->state = RPROC_RUNNING;
 
 	dev_info(dev, "remote processor %s is now up\n", rproc->name);
@@ -1712,7 +1713,8 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 		return -EINVAL;
 
 	/* Stop any subdevices for the remote processor */
-	rproc_stop_subdevices(rproc, crashed);
+	if (rproc->subdevs_started)
+		rproc_stop_subdevices(rproc, crashed);
 
 	/* the installed resource table is no longer accessible */
 	ret = rproc_reset_rsc_table_on_stop(rproc);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 580d324a1e8f..bc6adbd23827 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -539,6 +539,7 @@ enum rproc_features {
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
  * @sysfs_read_only: flag to make remoteproc sysfs files read only
+ * @subdevs_started: flag to indicate if subdevs have started
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @elf_class: firmware ELF class
@@ -581,6 +582,7 @@ struct rproc {
 	bool has_iommu;
 	bool auto_boot;
 	bool sysfs_read_only;
+	bool subdevs_started;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;

-- 
2.34.1

Re: [PATCH v2 2/3] remoteproc: core: Check subdev start status in rproc_stop()

[Stephan Gerhold]

On Tue, May 19, 2026 at 12:20:03AM -0700, Jingyi Wang wrote:
> For rproc that doing attach, rproc_start_subdevices() is called only when
> attach successfully. If rproc_report_crash() is called in the attach
> function, rproc_boot_recovery()->rproc_stop()->rproc_stop_subdevices()->
> glink_subdev_stop() could be called and cause NULL pointer dereference:
> 
>  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000300
>  Mem abort info:
>  ...
>  pc : qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem]
>  lr : glink_subdev_stop+0x1c/0x30 [qcom_common]
>  ...
>  Call trace:
>   qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem] (P)
>   glink_subdev_stop+0x1c/0x30 [qcom_common]
>   rproc_stop+0x58/0x17c
>   rproc_trigger_recovery+0xb0/0x150
>   rproc_crash_handler_work+0xa4/0xc4
>   process_scheduled_works+0x18c/0x2d8
>   worker_thread+0x144/0x280
>   kthread+0x124/0x138
>   ret_from_fork+0x10/0x20
>  Code: a9be7bfd 910003fd a90153f3 aa0003f3 (b9430000)
>  ---[ end trace 0000000000000000 ]---
> 
> Introduce "subdevs_started" flag to indicate rproc_start_subdevices() has
> been called successfully. Ensure subdevices are only stopped if they have
> been started.
> 
> Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 4 +++-
>  include/linux/remoteproc.h           | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f02db1113fae..6e23cb11e515 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1308,6 +1308,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  		goto stop_rproc;
>  	}
>  
> +	rproc->subdevs_started = true;
>  	rproc->state = RPROC_RUNNING;
>  
>  	dev_info(dev, "remote processor %s is now up\n", rproc->name);
> @@ -1712,7 +1713,8 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  		return -EINVAL;
>  
>  	/* Stop any subdevices for the remote processor */
> -	rproc_stop_subdevices(rproc, crashed);
> +	if (rproc->subdevs_started)
> +		rproc_stop_subdevices(rproc, crashed);

Thanks, this approach looks better. But where do we clear this flag?
Shouldn't we do that here?

In addition, I think we also need to set subdevs_started = true if
attach succeeds. And clear it when detaching a remoteproc. I think you
just need to update all the callers of rproc_stop_subdevices() and
rproc_start_subdevices(). Or, which is probably simpler, just make the
check directly inside these two functions to cover all users.

Thanks,
Stephan

Re: [PATCH v2 2/3] remoteproc: core: Check subdev start status in rproc_stop()

[Jingyi Wang]

On 5/22/2026 8:14 PM, Stephan Gerhold wrote:
> On Tue, May 19, 2026 at 12:20:03AM -0700, Jingyi Wang wrote:
>> For rproc that doing attach, rproc_start_subdevices() is called only when
>> attach successfully. If rproc_report_crash() is called in the attach
>> function, rproc_boot_recovery()->rproc_stop()->rproc_stop_subdevices()->
>> glink_subdev_stop() could be called and cause NULL pointer dereference:
>>
>>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000300
>>   Mem abort info:
>>   ...
>>   pc : qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem]
>>   lr : glink_subdev_stop+0x1c/0x30 [qcom_common]
>>   ...
>>   Call trace:
>>    qcom_glink_smem_unregister+0x14/0x48 [qcom_glink_smem] (P)
>>    glink_subdev_stop+0x1c/0x30 [qcom_common]
>>    rproc_stop+0x58/0x17c
>>    rproc_trigger_recovery+0xb0/0x150
>>    rproc_crash_handler_work+0xa4/0xc4
>>    process_scheduled_works+0x18c/0x2d8
>>    worker_thread+0x144/0x280
>>    kthread+0x124/0x138
>>    ret_from_fork+0x10/0x20
>>   Code: a9be7bfd 910003fd a90153f3 aa0003f3 (b9430000)
>>   ---[ end trace 0000000000000000 ]---
>>
>> Introduce "subdevs_started" flag to indicate rproc_start_subdevices() has
>> been called successfully. Ensure subdevices are only stopped if they have
>> been started.
>>
>> Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
>> ---
>>   drivers/remoteproc/remoteproc_core.c | 4 +++-
>>   include/linux/remoteproc.h           | 2 ++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index f02db1113fae..6e23cb11e515 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1308,6 +1308,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>   		goto stop_rproc;
>>   	}
>>   
>> +	rproc->subdevs_started = true;
>>   	rproc->state = RPROC_RUNNING;
>>   
>>   	dev_info(dev, "remote processor %s is now up\n", rproc->name);
>> @@ -1712,7 +1713,8 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>>   		return -EINVAL;
>>   
>>   	/* Stop any subdevices for the remote processor */
>> -	rproc_stop_subdevices(rproc, crashed);
>> +	if (rproc->subdevs_started)
>> +		rproc_stop_subdevices(rproc, crashed);
> 
> Thanks, this approach looks better. But where do we clear this flag?
> Shouldn't we do that here?
> 
> In addition, I think we also need to set subdevs_started = true if
> attach succeeds. And clear it when detaching a remoteproc. I think you
> just need to update all the callers of rproc_stop_subdevices() and
> rproc_start_subdevices(). Or, which is probably simpler, just make the
> check directly inside these two functions to cover all users.
> 
> Thanks,
> Stephan

Thx, I think it be better to modify the flag inside these two functions.
Will fix in next version.

Thanks,
Jingyi

[PATCH v2 3/3] remoteproc: core: cancel crash_handler work in rproc_add() error path

[Jingyi Wang]

Ensure crash_handler work is cancelled before tearing down rproc resources
to avoid accessing freed memory.

Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
---
 drivers/remoteproc/remoteproc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6e23cb11e515..abaa3786731b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2327,6 +2327,7 @@ 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:

-- 
2.34.1