On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> > > }
> > >
> > > /* Iterate through each output port to discover topology */
> > > - while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > + for_each_endpoint_of_node(parent, ep) {
> > > /*
> > > * Legacy binding mixes input/output ports under the
> > > * same parent. So, skip the input ports if we are dealing
> >
> > I think there's a bug below. The loop contains
> >
> > ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > if (ret)
> > return ret;
> >
> > which leaks the reference to ep. This is not introduced by this patch,
>
> Someone should create for_each_endpoint_of_node_scoped().
>
> #define for_each_endpoint_of_node_scoped(parent, child) \
> for (struct device_node *child __free(device_node) = \
> of_graph_get_next_endpoint(parent, NULL); child != NULL; \
> child = of_graph_get_next_endpoint(parent, child))
I was thinking about that too :-) I wondered if we should then bother
taking and releasing references, given that references to the children
can't be leaked out of the loop. My reasoning was that the parent
device_node is guaranteed to be valid throughout the loop, so borrowing
references to children instead of creating new ones within the loop
should be fine. This assumes that endpoints and ports can't vanish while
the parent is there. Thinking further about it, it may not be a safe
assumption for the future. As we anyway use functions internally that
create new references, we can as well handle them correctly.
Using this new macro, the loop body would need to call of_node_get() if
it wants to get a reference out of the loop. That's the right thing to
do, and I think it would be less error-prone than having to drop
references when exiting from the loop as we do today. It would still be
nice if we could have an API that allows catching this missing
of_node_get() automatically, but I don't see a simple way to do so at
the moment.
--
Regards,
Laurent Pinchart
