| Message ID | 20201104232356.4038506-16-saravanak@google.com |
|---|---|
| State | Accepted |
| Commit | 8a06d1ea061739dd2e60aff3d64a58892e4031cf |
| Headers | show |
| Series | [v1,01/18] Revert "driver core: Avoid deferred probe due to fw_devlink_pause/resume()" | expand |
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@google.com> > --- > 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? > 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? > 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. > 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. 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? thanks, greg k-h
On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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@google.com> > > --- > > 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
On Thu, Nov 5, 2020 at 3:25 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> 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@google.com> > > > --- > > > 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. *facepalm* for my earlier response. You'll notice that I've already deleted the "dev" input param to this function. That's why I can't use it here :) -Saravana
On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> 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@google.com> > > > --- > > > 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? That's fine, but why get rid of dev? The driver core works on these things, and we want errors/messages/warnings to spit out what device is causing those issues. It is fine to drag around a struct device pointer just for messages, that's to be expected, and is good. > > 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. If it can't happen, no need to check for it :) thanks, greg k-h
On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote: > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> 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@google.com> > > > > --- > > > > 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? > > That's fine, but why get rid of dev? The driver core works on these > things, and we want errors/messages/warnings to spit out what device is > causing those issues. It is fine to drag around a struct device pointer > just for messages, that's to be expected, and is good. In general I agree. If the fwnode being parsed is related to the dev, it's nice to have the dev name in the logs. But in this instance I feel it's analogous to printing the PID that's parsing the fwnode -- kinda irrelevant. The device in question can appear very random and it'll just cause more confusion than be of help if it shows up in the logs. For example it can be something like: <gpio device name>: linking <wifi fwnode> to <iommu fwnode> If the device was actually that of the wifi fwnode of the iommu fwnode, I agree it'll be useful to carry around the dev even if it's just for logs. Hope that makes sense. > > > 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. > > If it can't happen, no need to check for it :) I don't have a strong opinion, so I can delete it if you insist. -Saravana
On Thu, Nov 05, 2020 at 11:41:20PM -0800, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 11:22 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Nov 05, 2020 at 03:25:56PM -0800, Saravana Kannan wrote: > > > On Thu, Nov 5, 2020 at 1:41 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> 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@google.com> > > > > > --- > > > > > 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? > > > > That's fine, but why get rid of dev? The driver core works on these > > things, and we want errors/messages/warnings to spit out what device is > > causing those issues. It is fine to drag around a struct device pointer > > just for messages, that's to be expected, and is good. > > In general I agree. If the fwnode being parsed is related to the dev, > it's nice to have the dev name in the logs. > > But in this instance I feel it's analogous to printing the PID that's > parsing the fwnode -- kinda irrelevant. The device in question can > appear very random and it'll just cause more confusion than be of help > if it shows up in the logs. > > For example it can be something like: > <gpio device name>: linking <wifi fwnode> to <iommu fwnode> > > If the device was actually that of the wifi fwnode of the iommu > fwnode, I agree it'll be useful to carry around the dev even if it's > just for logs. > > Hope that makes sense. Not really, as the device here should be the one that is doing the linking, so why doesn't that matter? It shouldn't be confusing as this is what the DT asks to have happen, so reflecting that in the log if an error, or debugging, wants it should be helpful, not confusing. thanks, greg k-h
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); 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); 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); 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; -static int of_fwnode_add_links(const struct fwnode_handle *fwnode, - struct device *dev) -{ - if (unlikely(!is_of_node(fwnode))) - return 0; + for_each_property_of_node(con_np, p) + of_link_property(con_np, p->name); - return of_link_to_suppliers(dev, to_of_node(fwnode)); + return 0; } const struct fwnode_operations of_fwnode_ops = {
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@google.com> --- drivers/of/property.c | 150 ++++++++++++------------------------------ 1 file changed, 41 insertions(+), 109 deletions(-)