| Message ID | 20230320221826.2728078-2-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | remoteproc: Call of_node_put() on iteration error | expand |
Hi Mathieu, On 3/20/23 23:18, Mathieu Poirier wrote: > Function of_phandle_iterator_next() calls of_node_put() on the last > device_node it iterated over, but when the loop exits prematurely it has > to be called explicitly> > Fixes: 13140de09cc2 ("remoteproc: stm32: add an ST stm32_rproc driver") > Cc: stable@vger.kernel.org > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/stm32_rproc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > index 7d782ed9e589..23c1690b8d73 100644 > --- a/drivers/remoteproc/stm32_rproc.c > +++ b/drivers/remoteproc/stm32_rproc.c > @@ -223,11 +223,13 @@ static int stm32_rproc_prepare(struct rproc *rproc) > while (of_phandle_iterator_next(&it) == 0) { > rmem = of_reserved_mem_lookup(it.node); > if (!rmem) { > + of_node_put(it.node); > dev_err(dev, "unable to acquire memory-region\n"); > return -EINVAL; > } > > if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) { > + of_node_put(it.node); > dev_err(dev, "memory region not valid %pa\n", > &rmem->base); > return -EINVAL; > @@ -254,8 +256,10 @@ static int stm32_rproc_prepare(struct rproc *rproc) > it.node->name); > } > > - if (!mem) > + if (!mem) { > + of_node_put(it.node); > return -ENOMEM; > + } Good catch! Looking in code I don't see that we call of_node_put() when we release the carveouts. Please tell me if I'm wrong but look to me that we should also call of_node_put() in mem->release() op, in drivers. This one remains valid. reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> Thanks, Arnaud > > rproc_add_carveout(rproc, mem); > index++;
On Tue, Mar 21, 2023 at 10:00:03AM +0100, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 3/20/23 23:18, Mathieu Poirier wrote: > > Function of_phandle_iterator_next() calls of_node_put() on the last > > device_node it iterated over, but when the loop exits prematurely it has > > to be called explicitly> > > Fixes: 13140de09cc2 ("remoteproc: stm32: add an ST stm32_rproc driver") > > Cc: stable@vger.kernel.org > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/stm32_rproc.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c > > index 7d782ed9e589..23c1690b8d73 100644 > > --- a/drivers/remoteproc/stm32_rproc.c > > +++ b/drivers/remoteproc/stm32_rproc.c > > @@ -223,11 +223,13 @@ static int stm32_rproc_prepare(struct rproc *rproc) > > while (of_phandle_iterator_next(&it) == 0) { > > rmem = of_reserved_mem_lookup(it.node); > > if (!rmem) { > > + of_node_put(it.node); > > dev_err(dev, "unable to acquire memory-region\n"); > > return -EINVAL; > > } > > > > if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) { > > + of_node_put(it.node); > > dev_err(dev, "memory region not valid %pa\n", > > &rmem->base); > > return -EINVAL; > > @@ -254,8 +256,10 @@ static int stm32_rproc_prepare(struct rproc *rproc) > > it.node->name); > > } > > > > - if (!mem) > > + if (!mem) { > > + of_node_put(it.node); > > return -ENOMEM; > > + } > > Good catch! > > Looking in code I don't see that we call of_node_put() when we release the > carveouts. > Please tell me if I'm wrong but look to me that we should also call of_node_put() > in mem->release() op, in drivers. > Are you referring to entry->release(), which for stm32 is stm32_rproc_mem_release(), in rproc_resource_cleanup()? If so then no, it is not needed since of_phandle_iterator_next() calls of_node_put() on the previous device_node with each iteration. Otherwise I fail to understand the question and will ask you to clarify. > This one remains valid. > reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > Ok > Thanks, > Arnaud > > > > > > rproc_add_carveout(rproc, mem); > > index++;
On 3/21/23 22:32, Mathieu Poirier wrote: > On Tue, Mar 21, 2023 at 10:00:03AM +0100, Arnaud POULIQUEN wrote: >> Hi Mathieu, >> >> On 3/20/23 23:18, Mathieu Poirier wrote: >>> Function of_phandle_iterator_next() calls of_node_put() on the last >>> device_node it iterated over, but when the loop exits prematurely it has >>> to be called explicitly> >>> Fixes: 13140de09cc2 ("remoteproc: stm32: add an ST stm32_rproc driver") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>> --- >>> drivers/remoteproc/stm32_rproc.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c >>> index 7d782ed9e589..23c1690b8d73 100644 >>> --- a/drivers/remoteproc/stm32_rproc.c >>> +++ b/drivers/remoteproc/stm32_rproc.c >>> @@ -223,11 +223,13 @@ static int stm32_rproc_prepare(struct rproc *rproc) >>> while (of_phandle_iterator_next(&it) == 0) { >>> rmem = of_reserved_mem_lookup(it.node); >>> if (!rmem) { >>> + of_node_put(it.node); >>> dev_err(dev, "unable to acquire memory-region\n"); >>> return -EINVAL; >>> } >>> >>> if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) { >>> + of_node_put(it.node); >>> dev_err(dev, "memory region not valid %pa\n", >>> &rmem->base); >>> return -EINVAL; >>> @@ -254,8 +256,10 @@ static int stm32_rproc_prepare(struct rproc *rproc) >>> it.node->name); >>> } >>> >>> - if (!mem) >>> + if (!mem) { >>> + of_node_put(it.node); >>> return -ENOMEM; >>> + } >> >> Good catch! >> >> Looking in code I don't see that we call of_node_put() when we release the >> carveouts. >> Please tell me if I'm wrong but look to me that we should also call of_node_put() >> in mem->release() op, in drivers. >> > > Are you referring to entry->release(), which for stm32 is > stm32_rproc_mem_release(), in rproc_resource_cleanup()? > > If so then no, it is not needed since of_phandle_iterator_next() calls > of_node_put() on the previous device_node with each iteration. > > Otherwise I fail to understand the question and will ask you to clarify. My apologize, I misread the of_phandle_iterator_next function. you can forget my comment. Regards, Arnaud > >> This one remains valid. >> reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> > > Ok > >> Thanks, >> Arnaud >> >> >>> >>> rproc_add_carveout(rproc, mem); >>> index++;
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c index 7d782ed9e589..23c1690b8d73 100644 --- a/drivers/remoteproc/stm32_rproc.c +++ b/drivers/remoteproc/stm32_rproc.c @@ -223,11 +223,13 @@ static int stm32_rproc_prepare(struct rproc *rproc) while (of_phandle_iterator_next(&it) == 0) { rmem = of_reserved_mem_lookup(it.node); if (!rmem) { + of_node_put(it.node); dev_err(dev, "unable to acquire memory-region\n"); return -EINVAL; } if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) { + of_node_put(it.node); dev_err(dev, "memory region not valid %pa\n", &rmem->base); return -EINVAL; @@ -254,8 +256,10 @@ static int stm32_rproc_prepare(struct rproc *rproc) it.node->name); } - if (!mem) + if (!mem) { + of_node_put(it.node); return -ENOMEM; + } rproc_add_carveout(rproc, mem); index++;
Function of_phandle_iterator_next() calls of_node_put() on the last device_node it iterated over, but when the loop exits prematurely it has to be called explicitly. Fixes: 13140de09cc2 ("remoteproc: stm32: add an ST stm32_rproc driver") Cc: stable@vger.kernel.org Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/stm32_rproc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)