On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Nov 04, 2020 at 03:23:52PM -0800, Saravana Kannan wrote:
> > The semantics of add_links() has changed from creating device link
> > between devices to creating fwnode links between fwnodes. So, update the
> > implementation of add_links() to match the new semantics.
> >
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > ---
> > drivers/of/property.c | 150 ++++++++++++------------------------------
> > 1 file changed, 41 insertions(+), 109 deletions(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..86303803f1b3 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1038,33 +1038,9 @@ static bool of_is_ancestor_of(struct device_node *test_ancestor,
> > }
> >
> > /**
> > - * of_get_next_parent_dev - Add device link to supplier from supplier phandle
> > - * @np: device tree node
> > - *
> > - * Given a device tree node (@np), this function finds its closest ancestor
> > - * device tree node that has a corresponding struct device.
> > - *
> > - * The caller of this function is expected to call put_device() on the returned
> > - * device when they are done.
> > - */
> > -static struct device *of_get_next_parent_dev(struct device_node *np)
> > -{
> > - struct device *dev = NULL;
> > -
> > - of_node_get(np);
> > - do {
> > - np = of_get_next_parent(np);
> > - if (np)
> > - dev = get_dev_from_fwnode(&np->fwnode);
> > - } while (np && !dev);
> > - of_node_put(np);
> > - return dev;
> > -}
> > -
> > -/**
> > - * of_link_to_phandle - Add device link to supplier from supplier phandle
> > - * @dev: consumer device
> > - * @sup_np: phandle to supplier device tree node
> > + * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
> > + * @con_np: consumer device tree node
> > + * @sup_np: supplier device tree node
> > *
> > * Given a phandle to a supplier device tree node (@sup_np), this function
> > * finds the device that owns the supplier device tree node and creates a
> > @@ -1074,16 +1050,14 @@ static struct device *of_get_next_parent_dev(struct device_node *np)
> > * cases, it returns an error.
> > *
> > * Returns:
> > - * - 0 if link successfully created to supplier
> > - * - -EAGAIN if linking to the supplier should be reattempted
> > + * - 0 if fwnode link successfully created to supplier
> > * - -EINVAL if the supplier link is invalid and should not be created
> > - * - -ENODEV if there is no device that corresponds to the supplier phandle
> > + * - -ENODEV if struct device will never be create for supplier
> > */
> > -static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > - u32 dl_flags)
> > +static int of_link_to_phandle(struct device_node *con_np,
> > + struct device_node *sup_np)
> > {
> > - struct device *sup_dev, *sup_par_dev;
> > - int ret = 0;
> > + struct device *sup_dev;
> > struct device_node *tmp_np = sup_np;
> >
> > of_node_get(sup_np);
> > @@ -1106,7 +1080,8 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > }
> >
> > if (!sup_np) {
> > - dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
> > + pr_debug("Not linking %pOFP to %pOFP - No device\n",
> > + con_np, tmp_np);
>
> Who is calling this function without a valid dev pointer?
Sorry, I plan to delete the "dev" parameter as it's not really used
anywhere. I'm trying to do that without causing build time errors and
making the series into digestible small patches.
I can do the deletion of the parameter as a Patch 19/19. Will that work?
> > return -ENODEV;
> > }
> >
> > @@ -1115,53 +1090,30 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
> > * descendant nodes. By definition, a child node can't be a functional
> > * dependency for the parent node.
> > */
> > - if (of_is_ancestor_of(dev->of_node, sup_np)) {
> > - dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
> > + if (of_is_ancestor_of(con_np, sup_np)) {
> > + pr_debug("Not linking %pOFP to %pOFP - is descendant\n",
> > + con_np, sup_np);
>
> Why not dev_dbg() here? dev should be valid, correct?
Responded above.
> > of_node_put(sup_np);
> > return -EINVAL;
> > }
> > +
> > + /*
> > + * Don't create links to "early devices" that won't have struct devices
> > + * created for them.
> > + */
> > sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
> > if (!sup_dev && of_node_check_flag(sup_np, OF_POPULATED)) {
> > - /* Early device without struct device. */
> > - dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
> > - sup_np);
> > + pr_debug("Not linking %pOFP to %pOFP - No struct device\n",
> > + con_np, sup_np);
>
> How is dev not valid here? sup_dev might not be, but dev should be.
Answered above.
>
>
> > of_node_put(sup_np);
> > return -ENODEV;
> > - } else if (!sup_dev) {
> > - /*
> > - * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
> > - * cycles. So cycle detection isn't necessary and shouldn't be
> > - * done.
> > - */
> > - if (dl_flags & DL_FLAG_SYNC_STATE_ONLY) {
> > - of_node_put(sup_np);
> > - return -EAGAIN;
> > - }
> > -
> > - sup_par_dev = of_get_next_parent_dev(sup_np);
> > -
> > - if (sup_par_dev && device_is_dependent(dev, sup_par_dev)) {
> > - /* Cyclic dependency detected, don't try to link */
> > - dev_dbg(dev, "Not linking to %pOFP - cycle detected\n",
> > - sup_np);
> > - ret = -EINVAL;
> > - } else {
> > - /*
> > - * Can't check for cycles or no cycles. So let's try
> > - * again later.
> > - */
> > - ret = -EAGAIN;
> > - }
> > -
> > - of_node_put(sup_np);
> > - put_device(sup_par_dev);
> > - return ret;
> > }
> > - of_node_put(sup_np);
> > - if (!device_link_add(dev, sup_dev, dl_flags))
> > - ret = -EINVAL;
> > put_device(sup_dev);
> > - return ret;
> > +
> > + fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > + of_node_put(sup_np);
> > +
> > + return 0;
> > }
> >
> > /**
> > @@ -1361,37 +1313,29 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> > * that list phandles to suppliers. If @prop_name isn't one, this function
> > * doesn't do anything.
> > *
> > - * If @prop_name is one, this function attempts to create device links from the
> > - * consumer device @dev to all the devices of the suppliers listed in
> > - * @prop_name.
> > + * If @prop_name is one, this function attempts to create fwnode links from the
> > + * consumer device tree node @con_np to all the suppliers device tree nodes
> > + * listed in @prop_name.
> > *
> > - * Any failed attempt to create a device link will NOT result in an immediate
> > + * Any failed attempt to create a fwnode link will NOT result in an immediate
> > * return. of_link_property() must create links to all the available supplier
> > - * devices even when attempts to create a link to one or more suppliers fail.
> > + * device tree nodes even when attempts to create a link to one or more
> > + * suppliers fail.
> > */
> > -static int of_link_property(struct device *dev, struct device_node *con_np,
> > - const char *prop_name)
> > +static int of_link_property(struct device_node *con_np, const char *prop_name)
> > {
> > struct device_node *phandle;
> > const struct supplier_bindings *s = of_supplier_bindings;
> > unsigned int i = 0;
> > bool matched = false;
> > int ret = 0;
> > - u32 dl_flags;
> > -
> > - if (dev->of_node == con_np)
> > - dl_flags = fw_devlink_get_flags();
> > - else
> > - dl_flags = DL_FLAG_SYNC_STATE_ONLY;
> >
> > /* Do not stop at first failed link, link all available suppliers. */
> > while (!matched && s->parse_prop) {
> > while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> > matched = true;
> > i++;
> > - if (of_link_to_phandle(dev, phandle, dl_flags)
> > - == -EAGAIN)
> > - ret = -EAGAIN;
> > + of_link_to_phandle(con_np, phandle);
> > of_node_put(phandle);
> > }
> > s++;
> > @@ -1399,31 +1343,19 @@ static int of_link_property(struct device *dev, struct device_node *con_np,
> > return ret;
> > }
> >
> > -static int of_link_to_suppliers(struct device *dev,
> > - struct device_node *con_np)
> > +static int of_fwnode_add_links(struct fwnode_handle *fwnode,
> > + struct device *dev)
> > {
> > - struct device_node *child;
> > struct property *p;
> > - int ret = 0;
> > + struct device_node *con_np = to_of_node(fwnode);
> >
> > - for_each_property_of_node(con_np, p)
> > - if (of_link_property(dev, con_np, p->name))
> > - ret = -ENODEV;
> > -
> > - for_each_available_child_of_node(con_np, child)
> > - if (of_link_to_suppliers(dev, child) && !ret)
> > - ret = -EAGAIN;
> > -
> > - return ret;
> > -}
> > + if (unlikely(!con_np))
> > + return -EINVAL;
>
> Can you prove that unlikely() results in faster code? If not, don't use
> it.
Ok will delete it.
> And the only way it can be NULL is if fwnode is NULL, and as you control
> the callers to it, how can that be the case?
fwnode represents a generic firmware node. The to_of_node() returns
NULL if fwnode is not a DT node. So con_np can be NULL if that
happens. That's why we need a NULL check here. With the current code,
that can never happen, bit I think it doesn't hurt to check in case
there's a buggy caller. I don't have a strong opinion - so I can do it
whichever way.
-Saravana
