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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <85dc5e05-5d54-4d0f-a7e2-24134a5392f5@ti.com>
Date: Tue, 17 Dec 2024 10:00:22 -0600
From: Andrew Davis <afd@...com>
To: Beleswar Padhi <b-padhi@...com>, <andersson@...nel.org>,
        <mathieu.poirier@...aro.org>
CC: <hnagalla@...com>, <u-kumar1@...com>, <jan.kiszka@...mens.com>,
        <christophe.jaillet@...adoo.fr>, <linux-remoteproc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

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@...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);