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

[Beleswar Prasad Padhi]

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