Re: [PATCH 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] remoteproc: amd_mbv_bram: returning error from stop() leaks resources during unbind
--

commit a451b1daadf4b577c725f4374022e1394373deee
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

Partial reconfiguration of the programmable logic restores the AXI GPIO
that drives the soft-core reset to its hardware default direction, but
gpiolib keeps its cached output direction. gpiod_set_value_cansleep()
then only updates the data register, so the processor never sees a
reset transition: remoteproc reports it as running while the newly
loaded firmware never starts.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index 6cf33c7b7e641..da621f62f7df5 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> @@ -110,8 +127,8 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  	struct amd_bram_rproc *priv = rproc->priv;
>  	int ret;
>  
> -	/* Assert reset before disabling the processor clock. */
> -	ret = gpiod_set_value_cansleep(priv->reset, 1);
> +	/* Restore the output direction after DFX and assert reset. */
> +	ret = gpiod_direction_output(priv->reset, 1);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Disable clock after asserting reset */
>  	clk_disable_unprepare(priv->clk);
>  
>  	return 0;
>  }

[Severity: High]
Does this error path bypass local clock teardown and cause the core remoteproc
to abort its cleanup?

If gpiod_direction_output() fails in amd_bram_rproc_stop() during device unbind
or module removal, it returns an error. This propagates to __rproc_shutdown():

drivers/remoteproc/remoteproc_core.c:__rproc_shutdown() {
	...
	ret = rproc_stop(rproc, crashed);
	if (ret) {
		atomic_inc(&rproc->power);
		goto out;
	}

	/* clean up all acquired resources */
	rproc_resource_cleanup(rproc);
	...
}

This causes __rproc_shutdown() to abort early and skip rproc_resource_cleanup().

Since rproc_del() proceeds unconditionally:

drivers/remoteproc/remoteproc_core.c:rproc_del() {
	...
	__rproc_shutdown(rproc, true);

	rproc_delete_debug_dir(rproc);
	...
}

the error from __rproc_shutdown() is ignored.

Can this result in the rproc object and driver memory being freed by devm
while leaving the clock enabled, core remoteproc resources (carveouts,
IOMMU mappings, trace files) leaked, and any local mailboxes/works uncancelled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014213426.2957480-1-ben.levinsky@amd.com?part=2