On Mon, Nov 16, 2020 at 8:25 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > > > This function is a wrapper around fwnode_operations.add_links(). > > > > This function parses each node in a fwnode tree and create fwnode links > > for each of those nodes. The information for creating the fwnode links > > (the supplier and consumer fwnode) is obtained by parsing the properties > > in each of the fwnodes. > > > > This function also ensures that no fwnode is parsed more than once by > > marking the fwnodes as parsed. > > > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx> > > --- > > drivers/base/core.c | 19 +++++++++++++++++++ > > include/linux/fwnode.h | 3 +++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 4a0907574646..ee28d8c7ee85 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -1543,6 +1543,25 @@ static bool fw_devlink_is_permissive(void) > > return fw_devlink_flags == DL_FLAG_SYNC_STATE_ONLY; > > } > > > > +static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode) > > +{ > > + if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED) > > + return; > > Why is the flag needed? > > Duplicate links won't be created anyway and it doesn't cause the tree > walk to be terminated. To avoid parsing a fwnode more than once. The cumulative impact of the repeated parsing is actually quite high. And I intentionally didn't do this check at the tree walk level because DT overlay can add/remove/change individual fwnodes and I want to reparse those when they are added while avoiding parsing other nodes that have already been parsed and not changed by DT overlay. > > > + > > + fwnode_call_int_op(fwnode, add_links, NULL); > > + fwnode->flags |= FWNODE_FLAG_LINKS_ADDED; > > +} > > + > > +static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode) > > +{ > > + struct fwnode_handle *child = NULL; > > + > > + fw_devlink_parse_fwnode(fwnode); > > + > > + while ((child = fwnode_get_next_available_child_node(fwnode, child))) > > I'd prefer > > for (child = NULL; child; child = > fwnode_get_next_available_child_node(fwnode, child)) I was about to change to this and then realized it won't work. It would have to be for (child = fwnode_get_next_available_child_node(fwnode, NULL)); child; child = fwnode_get_next_available_child_node(fwnode, child)) Is that really better? The while() seems a lot more readable to me. I don't have a strong opinion, so I'll go with whatever you say after reading this. > > > + fw_devlink_parse_fwtree(child); > > +} > > + > > static void fw_devlink_link_device(struct device *dev) > > { > > int fw_ret; > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > > index ec02e1e939cc..9aaf9e4f3994 100644 > > --- a/include/linux/fwnode.h > > +++ b/include/linux/fwnode.h > > @@ -15,12 +15,15 @@ > > struct fwnode_operations; > > struct device; > > > > Description here, please. Ack > > > +#define FWNODE_FLAG_LINKS_ADDED BIT(0) > > + > > struct fwnode_handle { > > struct fwnode_handle *secondary; > > const struct fwnode_operations *ops; > > struct device *dev; > > struct list_head suppliers; > > struct list_head consumers; > > + u32 flags; > > That's a bit wasteful. Maybe u8 would suffice for the time being? Ack. -Saravana