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@xxxxxxxxxxxxxxxx>
> ---
>  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