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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> wrote:
>
> Hi All,
>
> This came up in review of
> https://lore.kernel.org/linux-iio/20210725172458.487343-1-jic23@xxxxxxxxxx/
> 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



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux