Re: [PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

[Andrew Davis <afd@ti.com>]

On 12/4/24 5:11 AM, Beleswar Padhi wrote:
> Use a device lifecycle managed action to release tsp ti_sci_proc handle.
> This helps prevent mistakes like releasing out of order in cleanup
> functions and forgetting to release on error paths.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 0753a5c35c7e..2966cb210403 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -1513,6 +1513,13 @@ static int k3_r5_core_of_get_sram_memories(struct platform_device *pdev,
>   	return 0;
>   }
>   
> +static void k3_r5_release_tsp(void *data)
> +{
> +	struct ti_sci_proc *tsp = data;
> +
> +	ti_sci_proc_release(tsp);
> +}
> +
>   static int k3_r5_core_of_init(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
>   		goto err;
>   	}
>   
> +	ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, core->tsp);
> +	if (ret)
> +		goto err;
> +
>   	platform_set_drvdata(pdev, core);
>   	devres_close_group(dev, k3_r5_core_of_init);
>   
> @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
>    */
>   static void k3_r5_core_of_exit(struct platform_device *pdev)
>   {
> -	struct k3_r5_core *core = platform_get_drvdata(pdev);
>   	struct device *dev = &pdev->dev;
> -	int ret;
> -
> -	ret = ti_sci_proc_release(core->tsp);
> -	if (ret)
> -		dev_err(dev, "failed to release proc, ret = %d\n", ret);
>   

One thing to remember is devm unrolling happens after remove(). So
here you are changing the order things happen. ti_sci_proc_release()
now will get called after the below functions. This most likely
isn't wrong, but to make review easier it helps to start from the
last called function in remove() and work backwards so nothing
is reordered.

Andrew

>   	platform_set_drvdata(pdev, NULL);
>   	devres_release_group(dev, k3_r5_core_of_init);