| Message ID | 20201104232356.4038506-18-saravanak@google.com |
|---|---|
| State | New |
| 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:54PM -0800, Saravana Kannan wrote: > Add helper functions __fw_devlink_link_to_consumers() and > __fw_devlink_link_to_suppliers() that convert fwnode links to device > links. > > __fw_devlink_link_to_consumers() is for creating: > - Device links between a newly added device and all its consumer devices > that have been added to driver core. > - Proxy SYNC_STATE_ONLY device links between the newly added device and > the parent devices of all its consumers that have not been added to > driver core yet. > > __fw_devlink_link_to_suppliers() is for creating: > - Device links between a newly added device and all its supplier devices > - Proxy SYNC_STATE_ONLY device links between the newly added device and > all the supplier devices of its child device nodes. > > Signed-off-by: Saravana Kannan <saravanak@google.com> Did you just add build warnings with these static functions that no one calls? thanks, greg k-h
On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > Add helper functions __fw_devlink_link_to_consumers() and > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > links. > > > > __fw_devlink_link_to_consumers() is for creating: > > - Device links between a newly added device and all its consumer devices > > that have been added to driver core. > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > the parent devices of all its consumers that have not been added to > > driver core yet. > > > > __fw_devlink_link_to_suppliers() is for creating: > > - Device links between a newly added device and all its supplier devices > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > all the supplier devices of its child device nodes. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Did you just add build warnings with these static functions that no one > calls? The next patch in this series uses it. I'm just splitting it up into a separate patch so that it's digestible and I can provide more details in the commit text. Couple of options: 1. Drop the static in this patch and add it back when it's used in patch 18/18. 2. Drop the commit text and squash this with 18/18 if you think the function documentation is clear enough and it won't make patch 18/18 too hard to review. Please let me know which one you'd prefer or if you have other options. I don't have a strong opinion on how the patches are split up as long as it's easy for the reviewers and future folks who look at git log to understand stuff. -Saravana
On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > > Add helper functions __fw_devlink_link_to_consumers() and > > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > > links. > > > > > > __fw_devlink_link_to_consumers() is for creating: > > > - Device links between a newly added device and all its consumer devices > > > that have been added to driver core. > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > the parent devices of all its consumers that have not been added to > > > driver core yet. > > > > > > __fw_devlink_link_to_suppliers() is for creating: > > > - Device links between a newly added device and all its supplier devices > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > all the supplier devices of its child device nodes. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Did you just add build warnings with these static functions that no one > > calls? > > The next patch in this series uses it. I'm just splitting it up into a > separate patch so that it's digestible and I can provide more details > in the commit text. But you can not add build warnings, you know this :) > Couple of options: > 1. Drop the static in this patch and add it back when it's used in patch 18/18. > 2. Drop the commit text and squash this with 18/18 if you think the > function documentation is clear enough and it won't make patch 18/18 > too hard to review. It is hard to review new functions when you do not see them being used, otherwise you have to flip back and forth between patches, which is difficult. Add the functions, and use them, in the same patch. Otherwise we have no idea _HOW_ you are using them, or even if you end up using them at all. thanks, greg k-h
On Thu, Nov 5, 2020 at 11:23 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 05, 2020 at 03:32:05PM -0800, Saravana Kannan wrote: > > On Thu, Nov 5, 2020 at 1:43 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Nov 04, 2020 at 03:23:54PM -0800, Saravana Kannan wrote: > > > > Add helper functions __fw_devlink_link_to_consumers() and > > > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > > > links. > > > > > > > > __fw_devlink_link_to_consumers() is for creating: > > > > - Device links between a newly added device and all its consumer devices > > > > that have been added to driver core. > > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > > the parent devices of all its consumers that have not been added to > > > > driver core yet. > > > > > > > > __fw_devlink_link_to_suppliers() is for creating: > > > > - Device links between a newly added device and all its supplier devices > > > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > > > all the supplier devices of its child device nodes. > > > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > > Did you just add build warnings with these static functions that no one > > > calls? > > > > The next patch in this series uses it. I'm just splitting it up into a > > separate patch so that it's digestible and I can provide more details > > in the commit text. > > But you can not add build warnings, you know this :) I know I can't break the build because that'll screw git bisect. But I thought warning that's fixed later in the series might be okay if it helps readability. I know now :) > > > Couple of options: > > 1. Drop the static in this patch and add it back when it's used in patch 18/18. > > 2. Drop the commit text and squash this with 18/18 if you think the > > function documentation is clear enough and it won't make patch 18/18 > > too hard to review. > > It is hard to review new functions when you do not see them being used, > otherwise you have to flip back and forth between patches, which is > difficult. > > Add the functions, and use them, in the same patch. Otherwise we have > no idea _HOW_ you are using them, or even if you end up using them at > all. Sounds good. I'll squash them. -Saravana
On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > Add helper functions __fw_devlink_link_to_consumers() and > __fw_devlink_link_to_suppliers() that convert fwnode links to device > links. > > __fw_devlink_link_to_consumers() is for creating: > - Device links between a newly added device and all its consumer devices > that have been added to driver core. > - Proxy SYNC_STATE_ONLY device links between the newly added device and > the parent devices of all its consumers that have not been added to > driver core yet. > > __fw_devlink_link_to_suppliers() is for creating: > - Device links between a newly added device and all its supplier devices > - Proxy SYNC_STATE_ONLY device links between the newly added device and > all the supplier devices of its child device nodes. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 228 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index d51dd564add1..0c87ff949d81 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > return dev; > } > > +/** > + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode Have you considered renaming "fw_devlink" to "fwnode_link"? That would be much less confusing IMO and the name of this function would be clearer. > + * @con - Consumer device for the device link > + * @sup - fwnode handle of supplier > + * > + * This function will try to create a device link between the consumer and the > + * supplier devices. "... between consumer device @con and the supplier device represented by @sup" (and see below). > + * > + * The supplier has to be provided as a fwnode because incorrect cycles in > + * fwnode links can sometimes cause the supplier device to never be created. > + * This function detects such cases and returns an error if a device link being > + * created in invalid. "... returns an error if it cannot create a device link from the consumer to a missing supplier." > + * > + * Returns, > + * 0 on successfully creating a device link > + * -EINVAL if the device link being attempted is invalid "if the device link cannot be created as expected" > + * -EAGAIN if the device link needs to be attempted again in the future "if the device link cannot be created right now, but it may be possible to do that in the future." > + */ > +static int fw_devlink_create_devlink(struct device *con, > + struct fwnode_handle *sup, u32 flags) I'd call the second arg sup_handle to indicate that it is not a struct device. > +{ > + struct device *sup_dev, *sup_par_dev; > + int ret = 0; > + > + sup_dev = get_dev_from_fwnode(sup); > + /* > + * If we can't find the supplier device from its fwnode, it might be > + * due to a cyclic dependcy between fwnodes. Some of these cycles can dependency > + * be broken by applying logic. Check for these types of cycles and > + * break them so that devices in the cycle probe properly. > + */ I would do if (sup_dev) { if (!device_link_add(con, sup_dev, flags)) ret = -EINVAL; put_device(sup_dev); return ret; } here and the cycle detection (error code path) below, possibly using the same dev pointer. > + 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 (flags & DL_FLAG_SYNC_STATE_ONLY) > + return -EAGAIN; > + > + sup_par_dev = fwnode_get_next_parent_dev(sup); > + > + /* > + * If the supplier's parent is dependent on the consumer, then > + * the consumer-supplier dependency is a false dependency. So, > + * treat it as an invalid link. > + */ > + if (sup_par_dev && device_is_dependent(con, sup_par_dev)) { > + dev_dbg(con, "Not linking to %pfwP - False link\n", > + sup); > + ret = -EINVAL; > + } else { > + /* > + * Can't check for cycles or no cycles. So let's try > + * again later. > + */ > + ret = -EAGAIN; > + } > + > + put_device(sup_par_dev); > + return ret; > + } > + > + /* > + * If we get this far and fail, this is due to cycles in device links. > + * Just give up on this link and treat it as invalid. > + */ > + if (!device_link_add(con, sup_dev, flags)) > + ret = -EINVAL; > + put_device(sup_dev); > + > + return ret; > +} > + > +/** > + * __fw_devlink_link_to_consumers - Create device links to consumers of a device > + * @dev - Device that needs to be linked to its consumers > + * > + * This function looks at all the consumer fwnodes of @dev and creates device > + * links between the consumer device and @dev (supplier). > + * > + * If the consumer device has not been added yet, then this function creates a > + * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device > + * of the consumer fwnode. This is necessary to make sure @dev doesn't get a > + * sync_state() callback before the real consumer device gets to be added and > + * then probed. > + * > + * Once device links are created from the real consumer to @dev (supplier), the > + * fwnode links are deleted. > + */ > +static void __fw_devlink_link_to_consumers(struct device *dev) > +{ > + struct fwnode_handle *fwnode = dev->fwnode; > + struct fwnode_link *link, *tmp; > + > + list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) { > + u32 dl_flags = fw_devlink_get_flags(); > + struct device *con_dev; > + bool own_link = true; > + int ret; > + > + con_dev = get_dev_from_fwnode(link->consumer); > + /* > + * If consumer device is not available yet, make a "proxy" > + * SYNC_STATE_ONLY link from the consumer's parent device to > + * the supplier device. This is necessary to make sure the > + * supplier doesn't get a sync_state() callback before the real > + * consumer can create a device link to the supplier. > + * > + * This proxy link step is needed to handle the case where the > + * consumer's parent device is added before the supplier. > + */ > + if (!con_dev) { > + con_dev = fwnode_get_next_parent_dev(link->consumer); > + /* > + * However, if the consumer's parent device is also the > + * parent of the supplier, don't create a > + * consumer-supplier link from the parent to its child > + * device. Such a dependency is impossible. > + */ > + if (con_dev && > + fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) { > + put_device(con_dev); > + con_dev = NULL; > + } else { > + own_link = false; > + dl_flags = DL_FLAG_SYNC_STATE_ONLY; > + } > + } > + > + if (!con_dev) > + continue; > + > + ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags); > + put_device(con_dev); > + if (!own_link || ret == -EAGAIN) > + continue; > + > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); > + } > +} > + > +/** > + * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device > + * @dev - The consumer device that needs to be linked to its suppliers > + * @fwnode - Root of the fwnode tree that is used to create device links > + * > + * This function looks at all the supplier fwnodes of fwnode tree rooted at > + * @fwnode and creates device links between @dev (consumer) and all the > + * supplier devices of the entire fwnode tree at @fwnode. See > + * fw_devlink_create_devlink() for more details. > + * > + * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev > + * and the real suppliers of @dev. Once these device links are created, the > + * fwnode links are deleted. When such device links are successfully created, > + * this function is called recursively on those supplier devices. This is > + * needed to detect and break some invalid cycles in fwnode links. > + * > + * In addition, it also looks at all the suppliers of the entire fwnode tree > + * because some of the child devices of @dev that have not been added yet > + * (because @dev hasn't probed) might already have their suppliers added to > + * driver core. So, this function creates SYNC_STATE_ONLY device links between > + * @dev (consumer) and these suppliers to make sure they don't execute their > + * sync_state() callbacks before these child devices have a chance to create > + * their device links. The fwnode links that correspond to the child devices > + * aren't delete because they are needed later to create the device links > + * between the real consumer and supplier devices. > + */ > +static void __fw_devlink_link_to_suppliers(struct device *dev, > + struct fwnode_handle *fwnode) > +{ > + bool own_link = (dev->fwnode == fwnode); > + struct fwnode_link *link, *tmp; > + struct fwnode_handle *child = NULL; > + u32 dl_flags; > + > + if (own_link) > + dl_flags = fw_devlink_get_flags(); > + else > + dl_flags = DL_FLAG_SYNC_STATE_ONLY; > + > + list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) { > + int ret; > + struct device *sup_dev; > + struct fwnode_handle *sup = link->supplier; > + > + ret = fw_devlink_create_devlink(dev, sup, dl_flags); > + if (!own_link || ret == -EAGAIN) > + continue; > + > + list_del(&link->s_hook); > + list_del(&link->c_hook); > + kfree(link); > + > + /* If no device link was created, nothing more to do. */ > + if (ret) > + continue; > + > + /* > + * If a device link was successfully created to a supplier, we > + * now need to try and link the supplier to all its suppliers. > + * > + * This is needed to detect and delete false dependencies in > + * fwnode links that haven't been converted to a device link > + * yet. See comments in fw_devlink_create_devlink() for more > + * details on the false dependency. > + * > + * Without deleting these false dependencies, some devices will > + * never probe because they'll keep waiting for their false > + * dependency fwnode links to be converted to device links. > + */ > + sup_dev = get_dev_from_fwnode(sup); > + __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode); > + put_device(sup_dev); > + } > + > + /* > + * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of > + * all the descendants. This proxy link step is needed to handle the > + * case where the supplier is added before the consumer's parent device > + * (@dev). > + */ > + while ((child = fwnode_get_next_available_child_node(fwnode, child))) > + __fw_devlink_link_to_suppliers(dev, child); > +} > + > static void fw_devlink_link_device(struct device *dev) > { > int fw_ret; > -- > 2.29.1.341.ge80a0c044ae-goog >
On Mon, Nov 16, 2020 at 8:57 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > Add helper functions __fw_devlink_link_to_consumers() and > > __fw_devlink_link_to_suppliers() that convert fwnode links to device > > links. > > > > __fw_devlink_link_to_consumers() is for creating: > > - Device links between a newly added device and all its consumer devices > > that have been added to driver core. > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > the parent devices of all its consumers that have not been added to > > driver core yet. > > > > __fw_devlink_link_to_suppliers() is for creating: > > - Device links between a newly added device and all its supplier devices > > - Proxy SYNC_STATE_ONLY device links between the newly added device and > > all the supplier devices of its child device nodes. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 228 insertions(+) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index d51dd564add1..0c87ff949d81 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) > > return dev; > > } > > > > +/** > > + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode > > Have you considered renaming "fw_devlink" to "fwnode_link"? I avoided using fwnode_link prefix because it's not related to the fwnode link APIs. This is a fw_devlink feature related code and hence the fw_devlink prefix. I could just call it fw_devlink_create() or fw_devlink_create_links() or fwnode_to_dev_link() if that's less confusing. Any preferences? > That would be much less confusing IMO and the name of this function > would be clearer. > > > + * @con - Consumer device for the device link > > + * @sup - fwnode handle of supplier > > + * > > + * This function will try to create a device link between the consumer and the > > + * supplier devices. > > "... between consumer device @con and the supplier device represented > by @sup" (and see below). > > > + * > > + * The supplier has to be provided as a fwnode because incorrect cycles in > > + * fwnode links can sometimes cause the supplier device to never be created. > > + * This function detects such cases and returns an error if a device link being > > + * created in invalid. > > "... returns an error if it cannot create a device link from the > consumer to a missing supplier." > > > + * > > + * Returns, > > + * 0 on successfully creating a device link > > + * -EINVAL if the device link being attempted is invalid > > "if the device link cannot be created as expected" > > > + * -EAGAIN if the device link needs to be attempted again in the future > > "if the device link cannot be created right now, but it may be > possible to do that in the future." Ack to all the documentation comments above. > > > + */ > > +static int fw_devlink_create_devlink(struct device *con, > > + struct fwnode_handle *sup, u32 flags) > > I'd call the second arg sup_handle to indicate that it is not a struct device. Ack > > > +{ > > + struct device *sup_dev, *sup_par_dev; > > + int ret = 0; > > + > > + sup_dev = get_dev_from_fwnode(sup); > > + /* > > + * If we can't find the supplier device from its fwnode, it might be > > + * due to a cyclic dependcy between fwnodes. Some of these cycles can > > dependency > Ack > > + * be broken by applying logic. Check for these types of cycles and > > + * break them so that devices in the cycle probe properly. > > + */ > > I would do > > if (sup_dev) { > if (!device_link_add(con, sup_dev, flags)) > ret = -EINVAL; > > put_device(sup_dev); > return ret; > } > > here and the cycle detection (error code path) below, possibly using > the same dev pointer. Nice suggestion, thanks! -Saravana
diff --git a/drivers/base/core.c b/drivers/base/core.c index d51dd564add1..0c87ff949d81 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1585,6 +1585,234 @@ static struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode) return dev; } +/** + * fw_devlink_create_devlink - Create a device link from a consumer to fwnode + * @con - Consumer device for the device link + * @sup - fwnode handle of supplier + * + * This function will try to create a device link between the consumer and the + * supplier devices. + * + * The supplier has to be provided as a fwnode because incorrect cycles in + * fwnode links can sometimes cause the supplier device to never be created. + * This function detects such cases and returns an error if a device link being + * created in invalid. + * + * Returns, + * 0 on successfully creating a device link + * -EINVAL if the device link being attempted is invalid + * -EAGAIN if the device link needs to be attempted again in the future + */ +static int fw_devlink_create_devlink(struct device *con, + struct fwnode_handle *sup, u32 flags) +{ + struct device *sup_dev, *sup_par_dev; + int ret = 0; + + sup_dev = get_dev_from_fwnode(sup); + /* + * If we can't find the supplier device from its fwnode, it might be + * due to a cyclic dependcy between fwnodes. Some of these cycles can + * be broken by applying logic. Check for these types of cycles and + * break them so that devices in the cycle probe properly. + */ + 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 (flags & DL_FLAG_SYNC_STATE_ONLY) + return -EAGAIN; + + sup_par_dev = fwnode_get_next_parent_dev(sup); + + /* + * If the supplier's parent is dependent on the consumer, then + * the consumer-supplier dependency is a false dependency. So, + * treat it as an invalid link. + */ + if (sup_par_dev && device_is_dependent(con, sup_par_dev)) { + dev_dbg(con, "Not linking to %pfwP - False link\n", + sup); + ret = -EINVAL; + } else { + /* + * Can't check for cycles or no cycles. So let's try + * again later. + */ + ret = -EAGAIN; + } + + put_device(sup_par_dev); + return ret; + } + + /* + * If we get this far and fail, this is due to cycles in device links. + * Just give up on this link and treat it as invalid. + */ + if (!device_link_add(con, sup_dev, flags)) + ret = -EINVAL; + put_device(sup_dev); + + return ret; +} + +/** + * __fw_devlink_link_to_consumers - Create device links to consumers of a device + * @dev - Device that needs to be linked to its consumers + * + * This function looks at all the consumer fwnodes of @dev and creates device + * links between the consumer device and @dev (supplier). + * + * If the consumer device has not been added yet, then this function creates a + * SYNC_STATE_ONLY link between @dev (supplier) and the closest ancestor device + * of the consumer fwnode. This is necessary to make sure @dev doesn't get a + * sync_state() callback before the real consumer device gets to be added and + * then probed. + * + * Once device links are created from the real consumer to @dev (supplier), the + * fwnode links are deleted. + */ +static void __fw_devlink_link_to_consumers(struct device *dev) +{ + struct fwnode_handle *fwnode = dev->fwnode; + struct fwnode_link *link, *tmp; + + list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) { + u32 dl_flags = fw_devlink_get_flags(); + struct device *con_dev; + bool own_link = true; + int ret; + + con_dev = get_dev_from_fwnode(link->consumer); + /* + * If consumer device is not available yet, make a "proxy" + * SYNC_STATE_ONLY link from the consumer's parent device to + * the supplier device. This is necessary to make sure the + * supplier doesn't get a sync_state() callback before the real + * consumer can create a device link to the supplier. + * + * This proxy link step is needed to handle the case where the + * consumer's parent device is added before the supplier. + */ + if (!con_dev) { + con_dev = fwnode_get_next_parent_dev(link->consumer); + /* + * However, if the consumer's parent device is also the + * parent of the supplier, don't create a + * consumer-supplier link from the parent to its child + * device. Such a dependency is impossible. + */ + if (con_dev && + fwnode_is_ancestor_of(con_dev->fwnode, fwnode)) { + put_device(con_dev); + con_dev = NULL; + } else { + own_link = false; + dl_flags = DL_FLAG_SYNC_STATE_ONLY; + } + } + + if (!con_dev) + continue; + + ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags); + put_device(con_dev); + if (!own_link || ret == -EAGAIN) + continue; + + list_del(&link->s_hook); + list_del(&link->c_hook); + kfree(link); + } +} + +/** + * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device + * @dev - The consumer device that needs to be linked to its suppliers + * @fwnode - Root of the fwnode tree that is used to create device links + * + * This function looks at all the supplier fwnodes of fwnode tree rooted at + * @fwnode and creates device links between @dev (consumer) and all the + * supplier devices of the entire fwnode tree at @fwnode. See + * fw_devlink_create_devlink() for more details. + * + * The function creates normal (non-SYNC_STATE_ONLY) device links between @dev + * and the real suppliers of @dev. Once these device links are created, the + * fwnode links are deleted. When such device links are successfully created, + * this function is called recursively on those supplier devices. This is + * needed to detect and break some invalid cycles in fwnode links. + * + * In addition, it also looks at all the suppliers of the entire fwnode tree + * because some of the child devices of @dev that have not been added yet + * (because @dev hasn't probed) might already have their suppliers added to + * driver core. So, this function creates SYNC_STATE_ONLY device links between + * @dev (consumer) and these suppliers to make sure they don't execute their + * sync_state() callbacks before these child devices have a chance to create + * their device links. The fwnode links that correspond to the child devices + * aren't delete because they are needed later to create the device links + * between the real consumer and supplier devices. + */ +static void __fw_devlink_link_to_suppliers(struct device *dev, + struct fwnode_handle *fwnode) +{ + bool own_link = (dev->fwnode == fwnode); + struct fwnode_link *link, *tmp; + struct fwnode_handle *child = NULL; + u32 dl_flags; + + if (own_link) + dl_flags = fw_devlink_get_flags(); + else + dl_flags = DL_FLAG_SYNC_STATE_ONLY; + + list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) { + int ret; + struct device *sup_dev; + struct fwnode_handle *sup = link->supplier; + + ret = fw_devlink_create_devlink(dev, sup, dl_flags); + if (!own_link || ret == -EAGAIN) + continue; + + list_del(&link->s_hook); + list_del(&link->c_hook); + kfree(link); + + /* If no device link was created, nothing more to do. */ + if (ret) + continue; + + /* + * If a device link was successfully created to a supplier, we + * now need to try and link the supplier to all its suppliers. + * + * This is needed to detect and delete false dependencies in + * fwnode links that haven't been converted to a device link + * yet. See comments in fw_devlink_create_devlink() for more + * details on the false dependency. + * + * Without deleting these false dependencies, some devices will + * never probe because they'll keep waiting for their false + * dependency fwnode links to be converted to device links. + */ + sup_dev = get_dev_from_fwnode(sup); + __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode); + put_device(sup_dev); + } + + /* + * Make "proxy" SYNC_STATE_ONLY device links to represent the needs of + * all the descendants. This proxy link step is needed to handle the + * case where the supplier is added before the consumer's parent device + * (@dev). + */ + while ((child = fwnode_get_next_available_child_node(fwnode, child))) + __fw_devlink_link_to_suppliers(dev, child); +} + static void fw_devlink_link_device(struct device *dev) { int fw_ret;
Add helper functions __fw_devlink_link_to_consumers() and __fw_devlink_link_to_suppliers() that convert fwnode links to device links. __fw_devlink_link_to_consumers() is for creating: - Device links between a newly added device and all its consumer devices that have been added to driver core. - Proxy SYNC_STATE_ONLY device links between the newly added device and the parent devices of all its consumers that have not been added to driver core yet. __fw_devlink_link_to_suppliers() is for creating: - Device links between a newly added device and all its supplier devices - Proxy SYNC_STATE_ONLY device links between the newly added device and all the supplier devices of its child device nodes. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/base/core.c | 228 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+)