Re: [PATCH 4/5] remoteproc: k3-r5: Use devm_ioremap_wc() helper

[Andrew Davis <afd@ti.com>]

On 12/4/24 5:11 AM, Beleswar Padhi wrote:
> Use a device lifecycle managed ioremap helper function. This helps
> prevent mistakes like unmapping out of order in cleanup functions and
> forgetting to unmap on all error paths.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 +++++-------------------
>   1 file changed, 8 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 2966cb210403..1a7681502f62 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -1004,17 +1004,13 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   	/* use remaining reserved memory regions for static carveouts */
>   	for (i = 0; i < num_rmems; i++) {
>   		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> -		if (!rmem_np) {
> -			ret = -EINVAL;
> -			goto unmap_rmem;
> -		}
> +		if (!rmem_np)
> +			return -EINVAL;
>   
>   		rmem = of_reserved_mem_lookup(rmem_np);
>   		of_node_put(rmem_np);
> -		if (!rmem) {
> -			ret = -EINVAL;
> -			goto unmap_rmem;
> -		}
> +		if (!rmem)
> +			return -EINVAL;
>   
>   		kproc->rmem[i].bus_addr = rmem->base;
>   		/*
> @@ -1029,12 +1025,11 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   		 */
>   		kproc->rmem[i].dev_addr = (u32)rmem->base;
>   		kproc->rmem[i].size = rmem->size;
> -		kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
> +		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
>   		if (!kproc->rmem[i].cpu_addr) {
>   			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
>   				i + 1, &rmem->base, &rmem->size);
> -			ret = -ENOMEM;
> -			goto unmap_rmem;
> +			return -ENOMEM;
>   		}
>   
>   		dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx va %pK da 0x%x\n",
> @@ -1045,19 +1040,6 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   	kproc->num_rmems = num_rmems;
>   
>   	return 0;
> -
> -unmap_rmem:
> -	for (i--; i >= 0; i--)
> -		iounmap(kproc->rmem[i].cpu_addr);
> -	return ret;
> -}
> -
> -static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
> -{
> -	int i;
> -
> -	for (i = 0; i < kproc->num_rmems; i++)
> -		iounmap(kproc->rmem[i].cpu_addr);
>   }
>   
>   /*
> @@ -1285,10 +1267,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   		}
>   
>   		ret = rproc_add(rproc);
> -		if (ret) {
> -			dev_err(dev, "rproc_add failed, ret = %d\n", ret);
> -			goto err_add;
> -		}
> +		if (ret)
> +			dev_err_probe(dev, ret, "rproc_add failed\n");

Did you mean to return the result of dev_err_probe() here? Without that
now anything between here and where err_add used to be will get run.
Might be more clear that this is correct by still doing a "goto out;"

Andrew

>   
>   		/* create only one rproc in lockstep, single-cpu or
>   		 * single core mode
> @@ -1333,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   
>   err_powerup:
>   	rproc_del(rproc);
> -err_add:
> -	k3_r5_reserved_mem_exit(kproc);
>   out:
>   	/* undo core0 upon any failures on core1 in split-mode */
>   	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
> @@ -1379,8 +1357,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>   		mbox_free_channel(kproc->mbox);
>   
>   		rproc_del(rproc);
> -
> -		k3_r5_reserved_mem_exit(kproc);
>   	}
>   }
>