| Message ID | 20230127001141.407071-2-saravanak@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | fw_devlink improvements | expand |
On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote: > When a device X is bound successfully to a driver, if it has a child > firmware node Y that doesn't have a struct device created by then, we > delete fwnode links where the child firmware node Y is the supplier. We > did this to avoid blocking the consumers of the child firmware node Y > from deferring probe indefinitely. > > While that a step in the right direction, it's better to make the > consumers of the child firmware node Y to be consumers of the device X > because device X is probably implementing whatever functionality is > represented by child firmware node Y. By doing this, we capture the > device dependencies more accurately and ensure better > probe/suspend/resume ordering. ... > static unsigned int defer_sync_state_count = 1; > static DEFINE_MUTEX(fwnode_link_lock); > static bool fw_devlink_is_permissive(void); > +static void __fw_devlink_link_to_consumers(struct device *dev); > static bool fw_devlink_drv_reg_done; > static bool fw_devlink_best_effort; I'm wondering if may avoid adding more forward declarations... Perhaps it's a sign that devlink code should be split to its own module? ... > -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > +static int __fwnode_link_add(struct fwnode_handle *con, > + struct fwnode_handle *sup) I believe we tolerate a bit longer lines, so you may still have it on a single line. ... > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > +{ > + int ret = 0; Redundant assignment. > + mutex_lock(&fwnode_link_lock); > + ret = __fwnode_link_add(con, sup); > + mutex_unlock(&fwnode_link_lock); > return ret; > } ... > if (dev->fwnode && dev->fwnode->dev == dev) { You may have above something like fwnode = dev_fwnode(dev); if (fwnode && fwnode->dev == dev) { > struct fwnode_handle *child; > fwnode_links_purge_suppliers(dev->fwnode); > + mutex_lock(&fwnode_link_lock); > fwnode_for_each_available_child_node(dev->fwnode, child) > - fw_devlink_purge_absent_suppliers(child); > + __fw_devlink_pickup_dangling_consumers(child, > + dev->fwnode); __fw_devlink_pickup_dangling_consumers(child, fwnode); > + __fw_devlink_link_to_consumers(dev); > + mutex_unlock(&fwnode_link_lock); > }
On Fri, Jan 27, 2023 at 1:22 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote: > > When a device X is bound successfully to a driver, if it has a child > > firmware node Y that doesn't have a struct device created by then, we > > delete fwnode links where the child firmware node Y is the supplier. We > > did this to avoid blocking the consumers of the child firmware node Y > > from deferring probe indefinitely. > > > > While that a step in the right direction, it's better to make the > > consumers of the child firmware node Y to be consumers of the device X > > because device X is probably implementing whatever functionality is > > represented by child firmware node Y. By doing this, we capture the > > device dependencies more accurately and ensure better > > probe/suspend/resume ordering. > > ... > > > static unsigned int defer_sync_state_count = 1; > > static DEFINE_MUTEX(fwnode_link_lock); > > static bool fw_devlink_is_permissive(void); > > +static void __fw_devlink_link_to_consumers(struct device *dev); > > static bool fw_devlink_drv_reg_done; > > static bool fw_devlink_best_effort; > > I'm wondering if may avoid adding more forward declarations... > > Perhaps it's a sign that devlink code should be split to its own > module? I've thought about that before, but I'm not there yet. Maybe once my remaining refactors and TODOs are done, it'd be a good time to revisit this question. But I don't think it should be done for the reason of forward declaration as we'd just end up moving these into base.h and we can do that even today. > > ... > > > -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > +static int __fwnode_link_add(struct fwnode_handle *con, > > + struct fwnode_handle *sup) > > I believe we tolerate a bit longer lines, so you may still have it on a single > line. That'd make it >80 cols. I'm going to leave it as is. > > ... > > > +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > +{ > > > + int ret = 0; > > Redundant assignment. Thanks. Will fix in v3. > > > + mutex_lock(&fwnode_link_lock); > > + ret = __fwnode_link_add(con, sup); > > + mutex_unlock(&fwnode_link_lock); > > return ret; > > } > > ... > > > if (dev->fwnode && dev->fwnode->dev == dev) { > > You may have above something like > > > fwnode = dev_fwnode(dev); I'll leave it as-is for now. I see dev->fwnode vs dev_fwnode() don't always give the same results. I need to re-examine other places I use dev->fwnode in fw_devlink code before I start using that function. But in general it seems like a good idea. I'll add this to my TODOs. > if (fwnode && fwnode->dev == dev) { > > > struct fwnode_handle *child; > > fwnode_links_purge_suppliers(dev->fwnode); > > + mutex_lock(&fwnode_link_lock); > > fwnode_for_each_available_child_node(dev->fwnode, child) > > - fw_devlink_purge_absent_suppliers(child); > > + __fw_devlink_pickup_dangling_consumers(child, > > + dev->fwnode); > > __fw_devlink_pickup_dangling_consumers(child, fwnode); I like the dev->fwnode->dev == dev check. It makes it super clear that I'm checking "The device's fwnode points back to the device". If I just use fwnode->dev == dev, then one will have to go back and read what fwnode is set to, etc. Also, when reading all these function calls it's easier to see that I'm working on the dev's fwnode (where dev is the device that was just bound to a driver) instead of some other fwnode. So I find it more readable as is and the compiler would optimize it anyway. If you feel strongly about this, I can change to use fwnode instead of dev->fwnode. Thanks, Saravana
On Fri, Jan 27, 2023 at 11:33:28PM -0800, Saravana Kannan wrote: > On Fri, Jan 27, 2023 at 1:22 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Jan 26, 2023 at 04:11:28PM -0800, Saravana Kannan wrote: ... > > > static unsigned int defer_sync_state_count = 1; > > > static DEFINE_MUTEX(fwnode_link_lock); > > > static bool fw_devlink_is_permissive(void); > > > +static void __fw_devlink_link_to_consumers(struct device *dev); > > > static bool fw_devlink_drv_reg_done; > > > static bool fw_devlink_best_effort; > > > > I'm wondering if may avoid adding more forward declarations... > > > > Perhaps it's a sign that devlink code should be split to its own > > module? > > I've thought about that before, but I'm not there yet. Maybe once my > remaining refactors and TODOs are done, it'd be a good time to revisit > this question. > > But I don't think it should be done for the reason of forward > declaration as we'd just end up moving these into base.h and we can do > that even today. What I meant is that the stacking up forward declarations is a good sign that something has to be done sooner than later. ... > > > -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) > > > +static int __fwnode_link_add(struct fwnode_handle *con, > > > + struct fwnode_handle *sup) > > > > I believe we tolerate a bit longer lines, so you may still have it on a single > > line. > > That'd make it >80 cols. I'm going to leave it as is. Is it a problem? ... > > > if (dev->fwnode && dev->fwnode->dev == dev) { > > > > You may have above something like > > > > fwnode = dev_fwnode(dev); > > I'll leave it as-is for now. I see dev->fwnode vs dev_fwnode() don't > always give the same results. I need to re-examine other places I use > dev->fwnode in fw_devlink code before I start using that function. But > in general it seems like a good idea. I'll add this to my TODOs. Please do, the rationale is to actually move the fwnode to the proper layer, now we have the single linked list defined in struct fwnode_handle and dereferencing fwnode from struct device without helper adds a lot of headache in the future. So, I really would like to see that we stopped doing that. > > if (fwnode && fwnode->dev == dev) { > > > > > struct fwnode_handle *child; > > > fwnode_links_purge_suppliers(dev->fwnode); > > > + mutex_lock(&fwnode_link_lock); > > > fwnode_for_each_available_child_node(dev->fwnode, child) > > > - fw_devlink_purge_absent_suppliers(child); > > > + __fw_devlink_pickup_dangling_consumers(child, > > > + dev->fwnode); > > > > __fw_devlink_pickup_dangling_consumers(child, fwnode); > > I like the dev->fwnode->dev == dev check. It makes it super clear that > I'm checking "The device's fwnode points back to the device". If I > just use fwnode->dev == dev, then one will have to go back and read > what fwnode is set to, etc. Also, when reading all these function > calls it's easier to see that I'm working on the dev's fwnode (where > dev is the device that was just bound to a driver) instead of some > other fwnode. > > So I find it more readable as is and the compiler would optimize it > anyway. If you feel strongly about this, I can change to use fwnode > instead of dev->fwnode. Please, read above.
diff --git a/drivers/base/core.c b/drivers/base/core.c index a3e14143ec0c..b6d98cc82f26 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -54,11 +54,12 @@ static LIST_HEAD(deferred_sync); static unsigned int defer_sync_state_count = 1; static DEFINE_MUTEX(fwnode_link_lock); static bool fw_devlink_is_permissive(void); +static void __fw_devlink_link_to_consumers(struct device *dev); static bool fw_devlink_drv_reg_done; static bool fw_devlink_best_effort; /** - * fwnode_link_add - Create a link between two fwnode_handles. + * __fwnode_link_add - Create a link between two fwnode_handles. * @con: Consumer end of the link. * @sup: Supplier end of the link. * @@ -74,22 +75,18 @@ static bool fw_devlink_best_effort; * Attempts to create duplicate links between the same pair of fwnode handles * are ignored and there is no reference counting. */ -int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) +static int __fwnode_link_add(struct fwnode_handle *con, + struct fwnode_handle *sup) { struct fwnode_link *link; - int ret = 0; - - mutex_lock(&fwnode_link_lock); list_for_each_entry(link, &sup->consumers, s_hook) if (link->consumer == con) - goto out; + return 0; link = kzalloc(sizeof(*link), GFP_KERNEL); - if (!link) { - ret = -ENOMEM; - goto out; - } + if (!link) + return -ENOMEM; link->supplier = sup; INIT_LIST_HEAD(&link->s_hook); @@ -100,9 +97,17 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) list_add(&link->c_hook, &con->suppliers); pr_debug("%pfwP Linked as a fwnode consumer to %pfwP\n", con, sup); -out: - mutex_unlock(&fwnode_link_lock); + return 0; +} + +int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup) +{ + int ret = 0; + + mutex_lock(&fwnode_link_lock); + ret = __fwnode_link_add(con, sup); + mutex_unlock(&fwnode_link_lock); return ret; } @@ -181,6 +186,51 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fw_devlink_purge_absent_suppliers); +/** + * __fwnode_links_move_consumers - Move consumer from @from to @to fwnode_handle + * @from: move consumers away from this fwnode + * @to: move consumers to this fwnode + * + * Move all consumer links from @from fwnode to @to fwnode. + */ +static void __fwnode_links_move_consumers(struct fwnode_handle *from, + struct fwnode_handle *to) +{ + struct fwnode_link *link, *tmp; + + list_for_each_entry_safe(link, tmp, &from->consumers, s_hook) { + __fwnode_link_add(link->consumer, to); + __fwnode_link_del(link); + } +} + +/** + * __fw_devlink_pickup_dangling_consumers - Pick up dangling consumers + * @fwnode: fwnode from which to pick up dangling consumers + * @new_sup: fwnode of new supplier + * + * If the @fwnode has a corresponding struct device and the device supports + * probing (that is, added to a bus), then we want to let fw_devlink create + * MANAGED device links to this device, so leave @fwnode and its descendant's + * fwnode links alone. + * + * Otherwise, move its consumers to the new supplier @new_sup. + */ +static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode, + struct fwnode_handle *new_sup) +{ + struct fwnode_handle *child; + + if (fwnode->dev && fwnode->dev->bus) + return; + + fwnode->flags |= FWNODE_FLAG_NOT_DEVICE; + __fwnode_links_move_consumers(fwnode, new_sup); + + fwnode_for_each_available_child_node(fwnode, child) + __fw_devlink_pickup_dangling_consumers(child, new_sup); +} + #ifdef CONFIG_SRCU static DEFINE_MUTEX(device_links_lock); DEFINE_STATIC_SRCU(device_links_srcu); @@ -1267,16 +1317,23 @@ void device_links_driver_bound(struct device *dev) * them. So, fw_devlink no longer needs to create device links to any * of the device's suppliers. * - * Also, if a child firmware node of this bound device is not added as - * a device by now, assume it is never going to be added and make sure - * other devices don't defer probe indefinitely by waiting for such a - * child device. + * Also, if a child firmware node of this bound device is not added as a + * device by now, assume it is never going to be added. Make this bound + * device the fallback supplier to the dangling consumers of the child + * firmware node because this bound device is probably implementing the + * child firmware node functionality and we don't want the dangling + * consumers to defer probe indefinitely waiting for a device for the + * child firmware node. */ if (dev->fwnode && dev->fwnode->dev == dev) { struct fwnode_handle *child; fwnode_links_purge_suppliers(dev->fwnode); + mutex_lock(&fwnode_link_lock); fwnode_for_each_available_child_node(dev->fwnode, child) - fw_devlink_purge_absent_suppliers(child); + __fw_devlink_pickup_dangling_consumers(child, + dev->fwnode); + __fw_devlink_link_to_consumers(dev); + mutex_unlock(&fwnode_link_lock); } device_remove_file(dev, &dev_attr_waiting_for_supplier); @@ -1855,7 +1912,11 @@ static int fw_devlink_create_devlink(struct device *con, fwnode_is_ancestor_of(sup_handle, con->fwnode)) return -EINVAL; - sup_dev = get_dev_from_fwnode(sup_handle); + if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE) + sup_dev = fwnode_get_next_parent_dev(sup_handle); + else + sup_dev = get_dev_from_fwnode(sup_handle); + if (sup_dev) { /* * If it's one of those drivers that don't actually bind to
When a device X is bound successfully to a driver, if it has a child firmware node Y that doesn't have a struct device created by then, we delete fwnode links where the child firmware node Y is the supplier. We did this to avoid blocking the consumers of the child firmware node Y from deferring probe indefinitely. While that a step in the right direction, it's better to make the consumers of the child firmware node Y to be consumers of the device X because device X is probably implementing whatever functionality is represented by child firmware node Y. By doing this, we capture the device dependencies more accurately and ensure better probe/suspend/resume ordering. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/base/core.c | 97 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 18 deletions(-)