linux-kernel - Re: RFC: Should we have a device_for_each_available_child

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 7 Dec 2021 15:12:33 -0600
From:   Rob Herring <robh+dt@...nel.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Jonathan Cameron <jonathan.cameron@...wei.com>
Subject: Re: RFC: Should we have a device_for_each_available_child_node()?

On Sun, Dec 5, 2021 at 1:48 PM Andy Shevchenko
<andy.shevchenko@...il.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@...nel.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