| Message ID | 20190904211126.47518-4-saravanak@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Solve postboot supplier cleanup and optimize probe ordering | expand |
Quoting Saravana Kannan (2019-09-04 14:11:22) > Add device links after the devices are created (but before they are > probed) by looking at common DT bindings like clocks and > interconnects. > > Automatically adding device links for functional dependencies at the > framework level provides the following benefits: > > - Optimizes device probe order and avoids the useless work of > attempting probes of devices that will not probe successfully > (because their suppliers aren't present or haven't probed yet). > > For example, in a commonly available mobile SoC, registering just > one consumer device's driver at an initcall level earlier than the > supplier device's driver causes 11 failed probe attempts before the > consumer device probes successfully. This was with a kernel with all > the drivers statically compiled in. This problem gets a lot worse if > all the drivers are loaded as modules without direct symbol > dependencies. > > - Supplier devices like clock providers, interconnect providers, etc > need to keep the resources they provide active and at a particular > state(s) during boot up even if their current set of consumers don't > request the resource to be active. This is because the rest of the > consumers might not have probed yet and turning off the resource > before all the consumers have probed could lead to a hang or > undesired user experience. > > Some frameworks (Eg: regulator) handle this today by turning off > "unused" resources at late_initcall_sync and hoping all the devices > have probed by then. This is not a valid assumption for systems with > loadable modules. Other frameworks (Eg: clock) just don't handle > this due to the lack of a clear signal for when they can turn off > resources. The clk framework disables unused clks at late_initcall_sync. What do you mean clk framework doesn't turn them off because of a clear signal? > This leads to downstream hacks to handle cases like this > that can easily be solved in the upstream kernel. > > By linking devices before they are probed, we give suppliers a clear > count of the number of dependent consumers. Once all of the > consumers are active, the suppliers can turn off the unused > resources without making assumptions about the number of consumers. > > By default we just add device-links to track "driver presence" (probe > succeeded) of the supplier device. If any other functionality provided > by device-links are needed, it is left to the consumer/supplier > devices to change the link when they probe. > Are there tabs above? Indentation looks off. > diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst > index d05d531b4ec9..6d421694d98e 100644 > --- a/Documentation/admin-guide/kernel-parameters.rst > +++ b/Documentation/admin-guide/kernel-parameters.rst > @@ -127,6 +127,7 @@ parameter is applicable:: > NET Appropriate network support is enabled. > NUMA NUMA support is enabled. > NFS Appropriate NFS support is enabled. > + OF Devicetree is enabled. > OSS OSS sound support is enabled. > PV_OPS A paravirtualized kernel is enabled. > PARIDE The ParIDE (parallel port IDE) subsystem is enabled. This could be split off and applied for dt_cpu_ftrs= in Documentation/admin-guide/kernel-parameters.txt > diff --git a/drivers/of/property.c b/drivers/of/property.c > index d7fa75e31f22..23b5ee5b0570 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -25,6 +25,7 @@ > #include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/string.h> > +#include <linux/moduleparam.h> > > #include "of_private.h" > > @@ -985,6 +986,245 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, > return of_device_get_match_data(dev); > } > > +static bool of_is_ancestor_of(struct device_node *test_ancestor, Maybe add kernel-doc indicating that this function returns with the of_node requiring an of_node_put() call on it, unless it returns false in which case it doesn't? > + struct device_node *child) > +{ > + of_node_get(child); > + while (child) { > + if (child == test_ancestor) { > + of_node_put(child); > + return false; > + } > + child = of_get_next_parent(child); > + } > + return true; > +} > + > +/** > + * of_link_to_phandle - Add device link to supplier from supplier phandle > + * @dev: consumer device > + * @sup_np: phandle to 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 > + * device link from @dev consumer device to the supplier device. This function > + * doesn't create device links for invalid scenarios such as trying to create a > + * link with a parent device as the consumer of its child device. In such > + * cases, it returns an error. Doesn't device links have problems with making cycles between providers and consumers? We have some scenarios where two clk providers are consumers of each other but it isn't a parent child relationship. They're peers on the SoC bus and there isn't a cycle in the clk tree but there is a cycle between the two device nodes and providers. I don't see the avoidance here but maybe I missed something? > + * > + * Returns: > + * - 0 if link successfully created to supplier > + * - -EAGAIN if linking to the supplier should be reattempted > + * - -EINVAL if the supplier link is invalid and should not be created > + * - -ENODEV if there is no device that corresponds to the supplier phandle > + */ > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np) > +{ > + struct device *sup_dev; > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER; Is it really a u32 instead of an unsigned int or unsigned long? > + int ret = 0; > + struct device_node *tmp_np = sup_np; > + > + of_node_get(sup_np); > + /* > + * Find the device node that contains the supplier phandle. It may be > + * @sup_np or it may be an ancestor of @sup_np. > + */ > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > + sup_np = of_get_next_parent(sup_np); I don't get this. This is assuming that drivers are only probed for device nodes that have a compatible string? What about drivers that make sub-devices for clk support that have drivers in drivers/clk/ that then attach at runtime later? This happens sometimes for MFDs that want to split the functionality across the driver tree to the respective subsystems. > + if (!sup_np) { > + dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np); > + return -ENODEV; > + } > + > + /* > + * Don't allow linking a device node as a consumer of one of its > + * 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); > + of_node_put(sup_np); > + return -EINVAL; > + } > + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); > + of_node_put(sup_np); > + if (!sup_dev) > + return -EAGAIN; > + if (!device_link_add(dev, sup_dev, dl_flags)) > + ret = -EAGAIN; > + put_device(sup_dev); > + return ret; > +} > + > +/** > + * parse_prop_cells - Property parsing function for suppliers > + * > + * @np: Pointer to device tree node containing a list > + * @prop_name: Name of property to be parsed. Expected to hold phandle values > + * @index: For properties holding a list of phandles, this is the index > + * into the list. > + * @list_name: Property name that is known to contain list of phandle(s) to > + * supplier(s) > + * @cells_name: property name that specifies phandles' arguments count > + * > + * This is a helper function to parse properties that have a known fixed name > + * and are a list of phandles and phandle arguments. > + * > + * Returns: > + * - phandle node pointer with refcount incremented. Caller must of_node_put() > + * on it when done. > + * - NULL if no phandle found at index > + */ > +static struct device_node *parse_prop_cells(struct device_node *np, > + const char *prop_name, int index, > + const char *list_name, > + const char *cells_name) > +{ > + struct of_phandle_args sup_args; > + > + if (strcmp(prop_name, list_name)) > + return NULL; > + > + if (of_parse_phandle_with_args(np, list_name, cells_name, index, > + &sup_args)) > + return NULL; > + > + return sup_args.np; > +} > + > +static struct device_node *parse_clocks(struct device_node *np, > + const char *prop_name, int index) > +{ > + return parse_prop_cells(np, prop_name, index, "clocks", "#clock-cells"); > +} Can this use of_parse_clkspec() instead? If it is exported out of the clk framework (which is weird to me for other reasons) then it should work to call that with the index passed in to this function. Ideally we don't have more than one place where we parse clock specifiers for a node. Another question is what happens for devices that are in DT but are using "clock-ranges"? As far as I know there are some DTS files that use that property to only send the clocks to some bus node that then lets devices find the "clocks" and "clock-names" properties from the bus node instead of from the node that corresponds to their device. > + > +static struct device_node *parse_interconnects(struct device_node *np, > + const char *prop_name, int index) > +{ > + return parse_prop_cells(np, prop_name, index, "interconnects", > + "#interconnect-cells"); > +} > + > +static int strcmp_suffix(const char *str, const char *suffix) And this doesn't go to lib/string.c why? > +{ > + unsigned int len, suffix_len; > + > + len = strlen(str); > + suffix_len = strlen(suffix); > + if (len <= suffix_len) > + return -1; > + return strcmp(str + len - suffix_len, suffix); > +} > + > +static struct device_node *parse_regulators(struct device_node *np, > + const char *prop_name, int index) > +{ > + if (index || strcmp_suffix(prop_name, "-supply")) > + return NULL; > + > + return of_parse_phandle(np, prop_name, 0); > +} > + > +/** > + * struct supplier_bindings - Property parsing functions for suppliers > + * > + * @parse_prop: function name > + * parse_prop() finds the node corresponding to a supplier phandle > + * @parse_prop.np: Pointer to device node holding supplier phandle property > + * @parse_prop.prop_name: Name of property holding a phandle value > + * @parse_prop.index: For properties holding a list of phandles, this is the > + * index into the list This is interesting kernel-doc. I've never seen it before. Does it work? > + * > + * Returns: > + * parse_prop() return values are > + * - phandle node pointer with refcount incremented. Caller must of_node_put() > + * on it when done. > + * - NULL if no phandle found at index > + */ > +struct supplier_bindings { > + struct device_node *(*parse_prop)(struct device_node *np, > + const char *prop_name, int index); Maybe this should be a typedef instead of a struct unless you plan to put more members in this struct? Or are arrays of function pointers impossible? > +}; > + > +static const struct supplier_bindings bindings[] = { This variable name is really bad. Please make it much more specific to this file instead of being called 'bindings' so that grepping for it and looking for it in kallsyms isn't difficult. > + { .parse_prop = parse_clocks, }, > + { .parse_prop = parse_interconnects, }, > + { .parse_prop = parse_regulators, }, > + {}, Nitpick: Don't put a comma after the sentinel so that it causes a compile error to follow it with another "valid" entry. > +}; > + > +/** > + * of_link_property - Create device links to suppliers listed in a property > + * @dev: Consumer device > + * @con_np: The consumer device tree node which contains the property > + * @prop_name: Name of property to be parsed > + * > + * This function checks if the property @prop_name that is present in the > + * @con_np device tree node is one of the known common device tree 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. > + * > + * Any failed attempt to create a device 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. > + */ > +static int of_link_property(struct device *dev, struct device_node *con_np, > + const char *prop_name) > +{ > + struct device_node *phandle; > + const struct supplier_bindings *s = bindings; > + unsigned int i = 0; > + bool matched = false; > + int ret = 0; > + > + /* 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) == -EAGAIN) > + ret = -EAGAIN; And don't break? > + of_node_put(phandle); > + } > + s++; > + } > + return ret; > +} > + > +static int __of_link_to_suppliers(struct device *dev, Why the double underscore? > + struct device_node *con_np) > +{ > + struct device_node *child; > + struct property *p; > + int ret = 0; > + > + for_each_property_of_node(con_np, p) > + if (of_link_property(dev, con_np, p->name)) > + ret = -EAGAIN; Same comment. > + > + return ret; > +}
On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote: > Quoting Saravana Kannan (2019-09-04 14:11:22) > > Add device links after the devices are created (but before they are > > probed) by looking at common DT bindings like clocks and > > interconnects. > > > > Automatically adding device links for functional dependencies at the > > framework level provides the following benefits: > > > > - Optimizes device probe order and avoids the useless work of > > attempting probes of devices that will not probe successfully > > (because their suppliers aren't present or haven't probed yet). > > > > For example, in a commonly available mobile SoC, registering just > > one consumer device's driver at an initcall level earlier than the > > supplier device's driver causes 11 failed probe attempts before the > > consumer device probes successfully. This was with a kernel with all > > the drivers statically compiled in. This problem gets a lot worse if > > all the drivers are loaded as modules without direct symbol > > dependencies. > > > > - Supplier devices like clock providers, interconnect providers, etc > > need to keep the resources they provide active and at a particular > > state(s) during boot up even if their current set of consumers don't > > request the resource to be active. This is because the rest of the > > consumers might not have probed yet and turning off the resource > > before all the consumers have probed could lead to a hang or > > undesired user experience. > > > > Some frameworks (Eg: regulator) handle this today by turning off > > "unused" resources at late_initcall_sync and hoping all the devices > > have probed by then. This is not a valid assumption for systems with > > loadable modules. Other frameworks (Eg: clock) just don't handle > > this due to the lack of a clear signal for when they can turn off > > resources. > > The clk framework disables unused clks at late_initcall_sync. What do > you mean clk framework doesn't turn them off because of a clear signal? There's a number of minor things you pointed out in this review. Saravana, can you send a follow-on patch for the minor code cleanups like formatting and the like that was found here? > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np) > > +{ > > + struct device *sup_dev; > > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER; > > Is it really a u32 instead of an unsigned int or unsigned long? > > > + int ret = 0; > > + struct device_node *tmp_np = sup_np; > > + > > + of_node_get(sup_np); > > + /* > > + * Find the device node that contains the supplier phandle. It may be > > + * @sup_np or it may be an ancestor of @sup_np. > > + */ > > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > > + sup_np = of_get_next_parent(sup_np); > > I don't get this. This is assuming that drivers are only probed for > device nodes that have a compatible string? What about drivers that make > sub-devices for clk support that have drivers in drivers/clk/ that then > attach at runtime later? This happens sometimes for MFDs that want to > split the functionality across the driver tree to the respective > subsystems. For that, the link would not be there, correct? > > +static int of_link_property(struct device *dev, struct device_node *con_np, > > + const char *prop_name) > > +{ > > + struct device_node *phandle; > > + const struct supplier_bindings *s = bindings; > > + unsigned int i = 0; > > + bool matched = false; > > + int ret = 0; > > + > > + /* 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) == -EAGAIN) > > + ret = -EAGAIN; > > And don't break? There was comments before about how this is not needed. Frank asked that the comment be removed. And now you point it out again :) Look at the comment a few lines up, we have to go through all of the suppliers. > > +static int __of_link_to_suppliers(struct device *dev, > > Why the double underscore? > > > + struct device_node *con_np) > > +{ > > + struct device_node *child; > > + struct property *p; > > + int ret = 0; > > + > > + for_each_property_of_node(con_np, p) > > + if (of_link_property(dev, con_np, p->name)) > > + ret = -EAGAIN; > > Same comment. Same response as above :) thanks, greg k-h
On Fri, Oct 4, 2019 at 8:37 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote: > > Quoting Saravana Kannan (2019-09-04 14:11:22) > > > Add device links after the devices are created (but before they are > > > probed) by looking at common DT bindings like clocks and > > > interconnects. > > > > > > Automatically adding device links for functional dependencies at the > > > framework level provides the following benefits: > > > > > > - Optimizes device probe order and avoids the useless work of > > > attempting probes of devices that will not probe successfully > > > (because their suppliers aren't present or haven't probed yet). > > > > > > For example, in a commonly available mobile SoC, registering just > > > one consumer device's driver at an initcall level earlier than the > > > supplier device's driver causes 11 failed probe attempts before the > > > consumer device probes successfully. This was with a kernel with all > > > the drivers statically compiled in. This problem gets a lot worse if > > > all the drivers are loaded as modules without direct symbol > > > dependencies. > > > > > > - Supplier devices like clock providers, interconnect providers, etc > > > need to keep the resources they provide active and at a particular > > > state(s) during boot up even if their current set of consumers don't > > > request the resource to be active. This is because the rest of the > > > consumers might not have probed yet and turning off the resource > > > before all the consumers have probed could lead to a hang or > > > undesired user experience. > > > > > > Some frameworks (Eg: regulator) handle this today by turning off > > > "unused" resources at late_initcall_sync and hoping all the devices > > > have probed by then. This is not a valid assumption for systems with > > > loadable modules. Other frameworks (Eg: clock) just don't handle > > > this due to the lack of a clear signal for when they can turn off > > > resources. > > > > The clk framework disables unused clks at late_initcall_sync. What do > > you mean clk framework doesn't turn them off because of a clear signal? > > There's a number of minor things you pointed out in this review. > > Saravana, can you send a follow-on patch for the minor code cleanups > like formatting and the like that was found here? Will do next week. Thanks, Saravana > > > > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np) > > > +{ > > > + struct device *sup_dev; > > > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER; > > > > Is it really a u32 instead of an unsigned int or unsigned long? > > > > > + int ret = 0; > > > + struct device_node *tmp_np = sup_np; > > > + > > > + of_node_get(sup_np); > > > + /* > > > + * Find the device node that contains the supplier phandle. It may be > > > + * @sup_np or it may be an ancestor of @sup_np. > > > + */ > > > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > > > + sup_np = of_get_next_parent(sup_np); > > > > I don't get this. This is assuming that drivers are only probed for > > device nodes that have a compatible string? What about drivers that make > > sub-devices for clk support that have drivers in drivers/clk/ that then > > attach at runtime later? This happens sometimes for MFDs that want to > > split the functionality across the driver tree to the respective > > subsystems. > > For that, the link would not be there, correct? > > > > +static int of_link_property(struct device *dev, struct device_node *con_np, > > > + const char *prop_name) > > > +{ > > > + struct device_node *phandle; > > > + const struct supplier_bindings *s = bindings; > > > + unsigned int i = 0; > > > + bool matched = false; > > > + int ret = 0; > > > + > > > + /* 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) == -EAGAIN) > > > + ret = -EAGAIN; > > > > And don't break? > > There was comments before about how this is not needed. Frank asked > that the comment be removed. And now you point it out again :) > > Look at the comment a few lines up, we have to go through all of the > suppliers. > > > > +static int __of_link_to_suppliers(struct device *dev, > > > > Why the double underscore? > > > > > + struct device_node *con_np) > > > +{ > > > + struct device_node *child; > > > + struct property *p; > > > + int ret = 0; > > > + > > > + for_each_property_of_node(con_np, p) > > > + if (of_link_property(dev, con_np, p->name)) > > > + ret = -EAGAIN; > > > > Same comment. > > Same response as above :) > > thanks, > > greg k-h
Quoting Greg Kroah-Hartman (2019-10-04 08:37:50) > On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote: > > Quoting Saravana Kannan (2019-09-04 14:11:22) > > > + int ret = 0; > > > + struct device_node *tmp_np = sup_np; > > > + > > > + of_node_get(sup_np); > > > + /* > > > + * Find the device node that contains the supplier phandle. It may be > > > + * @sup_np or it may be an ancestor of @sup_np. > > > + */ > > > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > > > + sup_np = of_get_next_parent(sup_np); > > > > I don't get this. This is assuming that drivers are only probed for > > device nodes that have a compatible string? What about drivers that make > > sub-devices for clk support that have drivers in drivers/clk/ that then > > attach at runtime later? This happens sometimes for MFDs that want to > > split the functionality across the driver tree to the respective > > subsystems. > > For that, the link would not be there, correct? The parent device (MFD) would have the links because that is the device node with the provider property like '#clock-cells'. The child clk device that's populated by the MFD would be the one actually providing the clk via a driver that may probe any time later, or never, depending on if the clk driver is configured as a module or not. I fail to see how this will work for these cases. Is this logic there to find the parent of a regulator phandle and match that to some driver? It looks like it. > > > > +static int of_link_property(struct device *dev, struct device_node *con_np, > > > + const char *prop_name) > > > +{ > > > + struct device_node *phandle; > > > + const struct supplier_bindings *s = bindings; > > > + unsigned int i = 0; > > > + bool matched = false; > > > + int ret = 0; > > > + > > > + /* 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) == -EAGAIN) > > > + ret = -EAGAIN; > > > > And don't break? > > There was comments before about how this is not needed. Frank asked > that the comment be removed. And now you point it out again :) > > Look at the comment a few lines up, we have to go through all of the > suppliers. > Ok. The comment tells me what is happening but it misses the essential part which is _why_ we must make links to each supplier and return -EAGAIN.
On Tue, Oct 8, 2019 at 7:53 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Greg Kroah-Hartman (2019-10-04 08:37:50) > > On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote: > > > Quoting Saravana Kannan (2019-09-04 14:11:22) > > > > + int ret = 0; > > > > + struct device_node *tmp_np = sup_np; > > > > + > > > > + of_node_get(sup_np); > > > > + /* > > > > + * Find the device node that contains the supplier phandle. It may be > > > > + * @sup_np or it may be an ancestor of @sup_np. > > > > + */ > > > > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > > > > + sup_np = of_get_next_parent(sup_np); > > > > > > I don't get this. This is assuming that drivers are only probed for > > > device nodes that have a compatible string? What about drivers that make > > > sub-devices for clk support that have drivers in drivers/clk/ that then > > > attach at runtime later? This happens sometimes for MFDs that want to > > > split the functionality across the driver tree to the respective > > > subsystems. > > > > For that, the link would not be there, correct? > > The parent device (MFD) would have the links because that is the device > node with the provider property like '#clock-cells'. The child clk > device that's populated by the MFD would be the one actually providing > the clk via a driver that may probe any time later, or never, depending > on if the clk driver is configured as a module or not. I fail to see how > this will work for these cases. > > Is this logic there to find the parent of a regulator phandle and match > that to some driver? It looks like it. In the case of an MFD creating "fake" children devices, the parent MFD device's driver is responsible for handling the sync state callback. It'll get the sync_state callback after all the child devices' consumers have probed. The MFD driver will need to do the sync state clean up for the children devices or pass it on to the child devices' drivers (whatever makes sense for that specific MFD) by whatever means those specific drivers talk to each other (direct calls, registering callbacks, etc). If they are real sub-devices, then they should really be captured in DT as child devices and then the child device's drivers will get the sync state callback directly. > > > > > > +static int of_link_property(struct device *dev, struct device_node *con_np, > > > > + const char *prop_name) > > > > +{ > > > > + struct device_node *phandle; > > > > + const struct supplier_bindings *s = bindings; > > > > + unsigned int i = 0; > > > > + bool matched = false; > > > > + int ret = 0; > > > > + > > > > + /* 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) == -EAGAIN) > > > > + ret = -EAGAIN; > > > > > > And don't break? > > > > There was comments before about how this is not needed. Frank asked > > that the comment be removed. And now you point it out again :) > > > > Look at the comment a few lines up, we have to go through all of the > > suppliers. > > > > Ok. The comment tells me what is happening but it misses the essential > part which is _why_ we must make links to each supplier and return > -EAGAIN. To be clear the -EAGAIN is only if any of the linking fails. The reason was already discussion in the email thread [1] but I agree it needs to be documented. I thought I had documented the _why_ in the documentation for fwnode.add_links(), but it's not there. I'll check to make sure I didn't capture it elsewhere and if not, I'll update fwnode.add_links documentation. To copy-paste the discussion from the earlier thread: "Actually, there is a point for this. Say Device-C depends on suppliers Device-S1 and Device-S2 and they are listed in DT in that order. Say, S1 gets populated after late_initcall_sync but S2 is probes way before that. If I don't continue past a "failed linking" to S1 and also link up to S2, then S2 will get a sync_state() callback before C is probed. So I have to go through all possible suppliers and [link] as many as possible." -Saravana [1] - https://lore.kernel.org/lkml/CAGETcx-hCrUvY5whZBihueqqCxmF3oDjFybjmoo3JUu87iiiEw@mail.gmail.com/
Quoting Saravana Kannan (2019-10-08 11:57:49) > On Tue, Oct 8, 2019 at 7:53 AM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Greg Kroah-Hartman (2019-10-04 08:37:50) > > > On Wed, Sep 11, 2019 at 03:29:25AM -0700, Stephen Boyd wrote: > > > > Quoting Saravana Kannan (2019-09-04 14:11:22) > > > > > + int ret = 0; > > > > > + struct device_node *tmp_np = sup_np; > > > > > + > > > > > + of_node_get(sup_np); > > > > > + /* > > > > > + * Find the device node that contains the supplier phandle. It may be > > > > > + * @sup_np or it may be an ancestor of @sup_np. > > > > > + */ > > > > > + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) > > > > > + sup_np = of_get_next_parent(sup_np); > > > > > > > > I don't get this. This is assuming that drivers are only probed for > > > > device nodes that have a compatible string? What about drivers that make > > > > sub-devices for clk support that have drivers in drivers/clk/ that then > > > > attach at runtime later? This happens sometimes for MFDs that want to > > > > split the functionality across the driver tree to the respective > > > > subsystems. > > > > > > For that, the link would not be there, correct? > > > > The parent device (MFD) would have the links because that is the device > > node with the provider property like '#clock-cells'. The child clk > > device that's populated by the MFD would be the one actually providing > > the clk via a driver that may probe any time later, or never, depending > > on if the clk driver is configured as a module or not. I fail to see how > > this will work for these cases. > > > > Is this logic there to find the parent of a regulator phandle and match > > that to some driver? It looks like it. > > In the case of an MFD creating "fake" children devices, the parent MFD > device's driver is responsible for handling the sync state callback. > It'll get the sync_state callback after all the child devices' > consumers have probed. The MFD driver will need to do the sync state > clean up for the children devices or pass it on to the child devices' > drivers (whatever makes sense for that specific MFD) by whatever means > those specific drivers talk to each other (direct calls, registering > callbacks, etc). > > If they are real sub-devices, then they should really be captured in > DT as child devices and then the child device's drivers will get the > sync state callback directly. It seems sort of hand-wavy at the moment. Is the plan to actually implement this for MFDs that are doing these things? It's really hard to understand this patch series without any actual users. From my perspective using driver probe as the signal that some resource like clks or regulators has been consumed and configured into the proper state is completely wrong. It makes a large assumption that driver probe is actually putting the device into some state that has taken over ownership of the device state by the time probe returns. That isn't always the case when you consider things like the display or GPU don't do much until their device is opened by userspace. It would be better to involve the various kernel frameworks in this decision by having those frameworks intercept the acquisition of the resources they provide and track consumers to the point where we can be certain all consumers have requested and configured the resources they need to operate properly without something go wrong. Maybe we need drivers to indicate this to frameworks somehow so that we don't turn the regulator off for the screen when the screen driver probes but the GPU driver hasn't started drawing anything there because userspace isn't running yet? I'm trying to take a step back and understand the bigger picture here. From what I can tell we're trying to answer the question "When have all the consumers of this resource put their constraints in place?" This is because we want to actively cleanup resources that have been left on or misconfigured by bootloader/firmware code but we can't be certain when to do that and if we should do that at all. Is that right?
diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst index d05d531b4ec9..6d421694d98e 100644 --- a/Documentation/admin-guide/kernel-parameters.rst +++ b/Documentation/admin-guide/kernel-parameters.rst @@ -127,6 +127,7 @@ parameter is applicable:: NET Appropriate network support is enabled. NUMA NUMA support is enabled. NFS Appropriate NFS support is enabled. + OF Devicetree is enabled. OSS OSS sound support is enabled. PV_OPS A paravirtualized kernel is enabled. PARIDE The ParIDE (parallel port IDE) subsystem is enabled. diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index af0a62af6fd8..e95f2a58acc5 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -3181,6 +3181,12 @@ This can be set from sysctl after boot. See Documentation/admin-guide/sysctl/vm.rst for details. + of_devlink [OF, KNL] Create device links between consumer and + supplier devices by scanning the devictree to infer the + consumer/supplier relationships. A consumer device + will not be probed until all the supplier devices have + probed successfully. + ohci1394_dma=early [HW] enable debugging via the ohci1394 driver. See Documentation/debugging-via-ohci1394.txt for more info. diff --git a/drivers/of/property.c b/drivers/of/property.c index d7fa75e31f22..23b5ee5b0570 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -25,6 +25,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/string.h> +#include <linux/moduleparam.h> #include "of_private.h" @@ -985,6 +986,245 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, return of_device_get_match_data(dev); } +static bool of_is_ancestor_of(struct device_node *test_ancestor, + struct device_node *child) +{ + of_node_get(child); + while (child) { + if (child == test_ancestor) { + of_node_put(child); + return false; + } + child = of_get_next_parent(child); + } + return true; +} + +/** + * of_link_to_phandle - Add device link to supplier from supplier phandle + * @dev: consumer device + * @sup_np: phandle to 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 + * device link from @dev consumer device to the supplier device. This function + * doesn't create device links for invalid scenarios such as trying to create a + * link with a parent device as the consumer of its child device. In such + * cases, it returns an error. + * + * Returns: + * - 0 if link successfully created to supplier + * - -EAGAIN if linking to the supplier should be reattempted + * - -EINVAL if the supplier link is invalid and should not be created + * - -ENODEV if there is no device that corresponds to the supplier phandle + */ +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np) +{ + struct device *sup_dev; + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER; + int ret = 0; + struct device_node *tmp_np = sup_np; + + of_node_get(sup_np); + /* + * Find the device node that contains the supplier phandle. It may be + * @sup_np or it may be an ancestor of @sup_np. + */ + while (sup_np && !of_find_property(sup_np, "compatible", NULL)) + sup_np = of_get_next_parent(sup_np); + if (!sup_np) { + dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np); + return -ENODEV; + } + + /* + * Don't allow linking a device node as a consumer of one of its + * 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); + of_node_put(sup_np); + return -EINVAL; + } + sup_dev = get_dev_from_fwnode(&sup_np->fwnode); + of_node_put(sup_np); + if (!sup_dev) + return -EAGAIN; + if (!device_link_add(dev, sup_dev, dl_flags)) + ret = -EAGAIN; + put_device(sup_dev); + return ret; +} + +/** + * parse_prop_cells - Property parsing function for suppliers + * + * @np: Pointer to device tree node containing a list + * @prop_name: Name of property to be parsed. Expected to hold phandle values + * @index: For properties holding a list of phandles, this is the index + * into the list. + * @list_name: Property name that is known to contain list of phandle(s) to + * supplier(s) + * @cells_name: property name that specifies phandles' arguments count + * + * This is a helper function to parse properties that have a known fixed name + * and are a list of phandles and phandle arguments. + * + * Returns: + * - phandle node pointer with refcount incremented. Caller must of_node_put() + * on it when done. + * - NULL if no phandle found at index + */ +static struct device_node *parse_prop_cells(struct device_node *np, + const char *prop_name, int index, + const char *list_name, + const char *cells_name) +{ + struct of_phandle_args sup_args; + + if (strcmp(prop_name, list_name)) + return NULL; + + if (of_parse_phandle_with_args(np, list_name, cells_name, index, + &sup_args)) + return NULL; + + return sup_args.np; +} + +static struct device_node *parse_clocks(struct device_node *np, + const char *prop_name, int index) +{ + return parse_prop_cells(np, prop_name, index, "clocks", "#clock-cells"); +} + +static struct device_node *parse_interconnects(struct device_node *np, + const char *prop_name, int index) +{ + return parse_prop_cells(np, prop_name, index, "interconnects", + "#interconnect-cells"); +} + +static int strcmp_suffix(const char *str, const char *suffix) +{ + unsigned int len, suffix_len; + + len = strlen(str); + suffix_len = strlen(suffix); + if (len <= suffix_len) + return -1; + return strcmp(str + len - suffix_len, suffix); +} + +static struct device_node *parse_regulators(struct device_node *np, + const char *prop_name, int index) +{ + if (index || strcmp_suffix(prop_name, "-supply")) + return NULL; + + return of_parse_phandle(np, prop_name, 0); +} + +/** + * struct supplier_bindings - Property parsing functions for suppliers + * + * @parse_prop: function name + * parse_prop() finds the node corresponding to a supplier phandle + * @parse_prop.np: Pointer to device node holding supplier phandle property + * @parse_prop.prop_name: Name of property holding a phandle value + * @parse_prop.index: For properties holding a list of phandles, this is the + * index into the list + * + * Returns: + * parse_prop() return values are + * - phandle node pointer with refcount incremented. Caller must of_node_put() + * on it when done. + * - NULL if no phandle found at index + */ +struct supplier_bindings { + struct device_node *(*parse_prop)(struct device_node *np, + const char *prop_name, int index); +}; + +static const struct supplier_bindings bindings[] = { + { .parse_prop = parse_clocks, }, + { .parse_prop = parse_interconnects, }, + { .parse_prop = parse_regulators, }, + {}, +}; + +/** + * of_link_property - Create device links to suppliers listed in a property + * @dev: Consumer device + * @con_np: The consumer device tree node which contains the property + * @prop_name: Name of property to be parsed + * + * This function checks if the property @prop_name that is present in the + * @con_np device tree node is one of the known common device tree 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. + * + * Any failed attempt to create a device 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. + */ +static int of_link_property(struct device *dev, struct device_node *con_np, + const char *prop_name) +{ + struct device_node *phandle; + const struct supplier_bindings *s = bindings; + unsigned int i = 0; + bool matched = false; + int ret = 0; + + /* 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) == -EAGAIN) + ret = -EAGAIN; + of_node_put(phandle); + } + s++; + } + return ret; +} + +static int __of_link_to_suppliers(struct device *dev, + struct device_node *con_np) +{ + struct device_node *child; + struct property *p; + int ret = 0; + + for_each_property_of_node(con_np, p) + if (of_link_property(dev, con_np, p->name)) + ret = -EAGAIN; + + return ret; +} + +static bool of_devlink; +core_param(of_devlink, of_devlink, bool, 0); + +static int of_fwnode_add_links(const struct fwnode_handle *fwnode, + struct device *dev) +{ + if (!of_devlink) + return 0; + + if (unlikely(!is_of_node(fwnode))) + return 0; + + return __of_link_to_suppliers(dev, to_of_node(fwnode)); +} + const struct fwnode_operations of_fwnode_ops = { .get = of_fwnode_get, .put = of_fwnode_put, @@ -1001,5 +1241,6 @@ const struct fwnode_operations of_fwnode_ops = { .graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint, .graph_get_port_parent = of_fwnode_graph_get_port_parent, .graph_parse_endpoint = of_fwnode_graph_parse_endpoint, + .add_links = of_fwnode_add_links, }; EXPORT_SYMBOL_GPL(of_fwnode_ops);
Add device links after the devices are created (but before they are probed) by looking at common DT bindings like clocks and interconnects. Automatically adding device links for functional dependencies at the framework level provides the following benefits: - Optimizes device probe order and avoids the useless work of attempting probes of devices that will not probe successfully (because their suppliers aren't present or haven't probed yet). For example, in a commonly available mobile SoC, registering just one consumer device's driver at an initcall level earlier than the supplier device's driver causes 11 failed probe attempts before the consumer device probes successfully. This was with a kernel with all the drivers statically compiled in. This problem gets a lot worse if all the drivers are loaded as modules without direct symbol dependencies. - Supplier devices like clock providers, interconnect providers, etc need to keep the resources they provide active and at a particular state(s) during boot up even if their current set of consumers don't request the resource to be active. This is because the rest of the consumers might not have probed yet and turning off the resource before all the consumers have probed could lead to a hang or undesired user experience. Some frameworks (Eg: regulator) handle this today by turning off "unused" resources at late_initcall_sync and hoping all the devices have probed by then. This is not a valid assumption for systems with loadable modules. Other frameworks (Eg: clock) just don't handle this due to the lack of a clear signal for when they can turn off resources. This leads to downstream hacks to handle cases like this that can easily be solved in the upstream kernel. By linking devices before they are probed, we give suppliers a clear count of the number of dependent consumers. Once all of the consumers are active, the suppliers can turn off the unused resources without making assumptions about the number of consumers. By default we just add device-links to track "driver presence" (probe succeeded) of the supplier device. If any other functionality provided by device-links are needed, it is left to the consumer/supplier devices to change the link when they probe. kbuild test robot reported clang error about missing const Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Saravana Kannan <saravanak@google.com> --- .../admin-guide/kernel-parameters.rst | 1 + .../admin-guide/kernel-parameters.txt | 6 + drivers/of/property.c | 241 ++++++++++++++++++ 3 files changed, 248 insertions(+)