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

[Beleswar Prasad Padhi <b-padhi@ti.com>]

On 17/12/24 21:33, Andrew Davis wrote:
> 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


Thanks for catching this. Should return here. Will fix in v2.

Thanks,
Beleswar

> 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);
>>       }
>>   }