RFC: Should we have a device_for_each_available_child_node()?

RFC: Should we have a device_for_each_available_child_node()?

[Jonathan Cameron]

Hi All,

This came up in review of
https://lore.kernel.org/linux-iio/20210725172458.487343-1-jic23@kernel.org/
which is a series converting a dt only driver over to generic properties.
I'm sending a separate email to raise the profile of the question rather
higher than it was buried in a driver review.

The original code used for_each_available_child_of_node(np, child) 
and the patch converted it to device_for_each_child_node().

Andy raised the question of whether it should have been
device_for_each_available_child_node() but that doesn't exist currently.

Things get more interesting when you look at the implementation of
device_for_each_child_node() which uses device_get_next_child_node()
which in turn calls fwnode_get_next_child_node() which calls
the get_next_child_node() op and for of that is
of_fwnode_get_next_child_node() which uses of_get_next_available_child()
rather than of_get_next_child().

So I think under the hood device_for_each_child_node() on of_ is going to
end up checking the node is available anyway.

So this all seemed a little odd given there were obvious calls to use
if we wanted to separate the two cases for device tree and they weren't
the ones used.  However, if we conclude that there is a bug here and
the two cases should be handled separately then it will be really hard
to be sure no driver is relying on this behaviour.

So, ultimately the question is:  Should I add a
device_for_each_available_child_node()?  It will be something like:

struct fwnode_handle *device_get_next_child_node(struct device *dev,
						 struct fwnode_handle *child)
{
	const struct fwnode_handle *fwnode = dev_fwnode(dev);
	struct fwnode_handle *next;

	/* Try to find a child in primary fwnode */
	next = fwnode_get_next_available_child_node(fwnode, child);
	if (next)
		return next;

	/* When no more children in primary, continue with secondary */
	if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
		next = fwnode_get_next_available_child_node(fwnode->secondary, child);

	return next;
}

#define device_for_each_child_node(dev, child)				\
	for (child = device_get_next_available_child_node(dev, NULL); child;	\
	     child = device_get_next_avaialble_child_node(dev, child))

As far as I can tell it doesn't make any difference for my particular bit
of refactoring in the sense of I won't break anything that currently
works by using device_for_each_child_node() but it may cause issues with
other firmware by enumerating disabled child nodes.

Jonathan

Re: RFC: Should we have a device_for_each_available_child_node()?

[Andy Shevchenko]

I think we need Rob here (or anybody with DT API knowledge) to explain
this subtle detail you found, i.e. checking node for availability in
of_fwnode_get_next_child_node(). This raises another question why do
we have for_each_available_child_of_node() in the first place if it's
equivalent (is it?) to for_each_child_of_node()/

On Sun, Dec 5, 2021 at 8:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> Hi All,
>
> This came up in review of
> https://lore.kernel.org/linux-iio/20210725172458.487343-1-jic23@kernel.org/
> which is a series converting a dt only driver over to generic properties.
> I'm sending a separate email to raise the profile of the question rather
> higher than it was buried in a driver review.
>
> The original code used for_each_available_child_of_node(np, child)
> and the patch converted it to device_for_each_child_node().
>
> Andy raised the question of whether it should have been
> device_for_each_available_child_node() but that doesn't exist currently.
>
> Things get more interesting when you look at the implementation of
> device_for_each_child_node() which uses device_get_next_child_node()
> which in turn calls fwnode_get_next_child_node() which calls
> the get_next_child_node() op and for of that is
> of_fwnode_get_next_child_node() which uses of_get_next_available_child()
> rather than of_get_next_child().
>
> So I think under the hood device_for_each_child_node() on of_ is going to
> end up checking the node is available anyway.
>
> So this all seemed a little odd given there were obvious calls to use
> if we wanted to separate the two cases for device tree and they weren't
> the ones used.  However, if we conclude that there is a bug here and
> the two cases should be handled separately then it will be really hard
> to be sure no driver is relying on this behaviour.
>
> So, ultimately the question is:  Should I add a
> device_for_each_available_child_node()?  It will be something like:
>
> struct fwnode_handle *device_get_next_child_node(struct device *dev,
>                                                  struct fwnode_handle *child)
> {
>         const struct fwnode_handle *fwnode = dev_fwnode(dev);
>         struct fwnode_handle *next;
>
>         /* Try to find a child in primary fwnode */
>         next = fwnode_get_next_available_child_node(fwnode, child);
>         if (next)
>                 return next;
>
>         /* When no more children in primary, continue with secondary */
>         if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
>                 next = fwnode_get_next_available_child_node(fwnode->secondary, child);
>
>         return next;
> }
>
> #define device_for_each_child_node(dev, child)                          \
>         for (child = device_get_next_available_child_node(dev, NULL); child;    \
>              child = device_get_next_avaialble_child_node(dev, child))
>
> As far as I can tell it doesn't make any difference for my particular bit
> of refactoring in the sense of I won't break anything that currently
> works by using device_for_each_child_node() but it may cause issues with
> other firmware by enumerating disabled child nodes.
>
> Jonathan
>
>
>
>
>


-- 
With Best Regards,
Andy Shevchenko

Re: RFC: Should we have a device_for_each_available_child_node()?

[Rob Herring]

On Sun, Dec 5, 2021 at 1:48 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> I think we need Rob here (or anybody with DT API knowledge) to explain
> this subtle detail you found, i.e. checking node for availability in
> of_fwnode_get_next_child_node(). This raises another question why do
> we have for_each_available_child_of_node() in the first place if it's
> equivalent (is it?) to for_each_child_of_node()/

It's not equivalent, but in an ideal world they would have been. Most
of the time, one should be using for_each_available_child_of_node() as
that treats disabled nodes as if they were non-existent.
Unfortunately, there are some cases where walking the disabled nodes
is desirable/needed. On !Arm, disabled CPU nodes are ones that are
offline for example.

Ideally, we would have had for_each_child_of_node() and
for_each_yes_I_really_want_disabled_child_of_node() instead.

> On Sun, Dec 5, 2021 at 8:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > Hi All,
> >
> > This came up in review of
> > https://lore.kernel.org/linux-iio/20210725172458.487343-1-jic23@kernel.org/
> > which is a series converting a dt only driver over to generic properties.
> > I'm sending a separate email to raise the profile of the question rather
> > higher than it was buried in a driver review.
> >
> > The original code used for_each_available_child_of_node(np, child)
> > and the patch converted it to device_for_each_child_node().
> >
> > Andy raised the question of whether it should have been
> > device_for_each_available_child_node() but that doesn't exist currently.
> >
> > Things get more interesting when you look at the implementation of
> > device_for_each_child_node() which uses device_get_next_child_node()
> > which in turn calls fwnode_get_next_child_node() which calls
> > the get_next_child_node() op and for of that is
> > of_fwnode_get_next_child_node() which uses of_get_next_available_child()
> > rather than of_get_next_child().

That may have been based on my feedback so that fwnode has the 'right'
interface...

> > So I think under the hood device_for_each_child_node() on of_ is going to
> > end up checking the node is available anyway.
> >
> > So this all seemed a little odd given there were obvious calls to use
> > if we wanted to separate the two cases for device tree and they weren't
> > the ones used.  However, if we conclude that there is a bug here and
> > the two cases should be handled separately then it will be really hard
> > to be sure no driver is relying on this behaviour.
> >
> > So, ultimately the question is:  Should I add a
> > device_for_each_available_child_node()?  It will be something like:
> >
> > struct fwnode_handle *device_get_next_child_node(struct device *dev,
> >                                                  struct fwnode_handle *child)
> > {
> >         const struct fwnode_handle *fwnode = dev_fwnode(dev);
> >         struct fwnode_handle *next;
> >
> >         /* Try to find a child in primary fwnode */
> >         next = fwnode_get_next_available_child_node(fwnode, child);
> >         if (next)
> >                 return next;
> >
> >         /* When no more children in primary, continue with secondary */
> >         if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> >                 next = fwnode_get_next_available_child_node(fwnode->secondary, child);
> >
> >         return next;
> > }
> >
> > #define device_for_each_child_node(dev, child)                          \
> >         for (child = device_get_next_available_child_node(dev, NULL); child;    \
> >              child = device_get_next_avaialble_child_node(dev, child))
> >
> > As far as I can tell it doesn't make any difference for my particular bit
> > of refactoring in the sense of I won't break anything that currently
> > works by using device_for_each_child_node() but it may cause issues with
> > other firmware by enumerating disabled child nodes.
> >
> > Jonathan
> >
> >
> >
> >
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko