On Wed, Aug 7, 2019 at 7:04 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>
> > Date: Tue, 23 Jul 2019 17:10:54 -0700
> > Subject: [PATCH v7 1/7] driver core: Add support for linking devices during
> > device addition
> > From: Saravana Kannan <saravanak@xxxxxxxxxx>
> >
> > When devices are added, the bus might want to create device links to track
> > functional dependencies between supplier and consumer devices. This
> > tracking of supplier-consumer relationship allows optimizing device probe
> > order and tracking whether all consumers of a supplier are active. The
> > add_links bus callback is added to support this.
>
> Change above to:
>
> When devices are added, the bus may create device links to track which
> suppliers a consumer device depends upon. This
> tracking of supplier-consumer relationship may be used to defer probing
> the driver of a consumer device before the driver(s) for its supplier device(s)
> are probed. It may also be used by a supplier driver to determine if
> all of its consumers have been successfully probed.
> The add_links bus callback is added to create the supplier device links
>
> >
> > However, when consumer devices are added, they might not have a supplier
> > device to link to despite needing mandatory resources/functionality from
> > one or more suppliers. A waiting_for_suppliers list is created to track
> > such consumers and retry linking them when new devices get added.
>
> Change above to:
>
> If a supplier device has not yet been created when the consumer device attempts
> to link it, the consumer device is added to the wait_for_suppliers list.
> When supplier devices are created, the supplier device link will be added to
> the relevant consumer devices on the wait_for_suppliers list.
>
I'll take these commit text suggestions if we decide to revert the
entire series at the end of this review.
> >
> > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> > ---
> > drivers/base/core.c | 83 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/device.h | 14 +++++++
> > 2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index da84a73f2ba6..1b4eb221968f 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
> > #endif
> >
> > /* Device links support. */
> > +static LIST_HEAD(wait_for_suppliers);
> > +static DEFINE_MUTEX(wfs_lock);
> >
> > #ifdef CONFIG_SRCU
> > static DEFINE_MUTEX(device_links_lock);
> > @@ -401,6 +403,51 @@ struct device_link *device_link_add(struct device *consumer,
> > }
> > EXPORT_SYMBOL_GPL(device_link_add);
> >
> > +/**
>
> > + * device_link_wait_for_supplier - Mark device as waiting for supplier
>
> * device_link_wait_for_supplier - Add device to wait_for_suppliers list
I intentionally chose "Mark device..." because that's a better
description of the semantics of the function instead of trying to
describe the implementation. Whether I'm using a linked list or some
other data structure should not be the one line documentation of a
function. Unless the function is explicitly about operating on that
specific data structure.
>
>
> > + * @consumer: Consumer device
> > + *
> > + * Marks the consumer device as waiting for suppliers to become available. The
> > + * consumer device will never be probed until it's unmarked as waiting for
> > + * suppliers. The caller is responsible for adding the link to the supplier
> > + * once the supplier device is present.
> > + *
> > + * This function is NOT meant to be called from the probe function of the
> > + * consumer but rather from code that creates/adds the consumer device.
> > + */
> > +static void device_link_wait_for_supplier(struct device *consumer)
> > +{
> > + mutex_lock(&wfs_lock);
> > + list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
> > + mutex_unlock(&wfs_lock);
> > +}
> > +
> > +/**
>
>
> > + * device_link_check_waiting_consumers - Try to remove from supplier wait list
> > + *
> > + * Loops through all consumers waiting on suppliers and tries to add all their
> > + * supplier links. If that succeeds, the consumer device is unmarked as waiting
> > + * for suppliers. Otherwise, they are left marked as waiting on suppliers,
> > + *
> > + * The add_links bus callback is expected to return 0 if it has found and added
> > + * all the supplier links for the consumer device. It should return an error if
> > + * it isn't able to do so.
> > + *
> > + * The caller of device_link_wait_for_supplier() is expected to call this once
> > + * it's aware of potential suppliers becoming available.
>
> Change above comment to:
>
> * device_link_add_supplier_links - add links from consumer devices to
> * supplier devices, leaving any consumer
> * with inactive suppliers on the
> * wait_for_suppliers list
I didn't know that the first one line comment could span multiple
lines. Good to know.
> * Scan all consumer devices in the devicetree.
This function doesn't have anything to do with devicetree. I've
intentionally kept all OF related parts out of the driver/core because
I hope that other busses can start using this feature too. So I can't
take this bit.
> For any supplier device that
> * is not already linked to the consumer device, add the supplier to the
> * consumer device's device links.
> *
> * If all of a consumer device's suppliers are available then the consumer
> * is removed from the wait_for_suppliers list (if previously on the list).
> * Otherwise the consumer is added to the wait_for_suppliers list (if not
> * already on the list).
Honestly, I don't think this is any better than what I already have.
> * The add_links bus callback must return 0 if it has found and added all
> * the supplier links for the consumer device. It must return an error if
> * it is not able to do so.
> *
> * The caller of device_link_wait_for_supplier() is expected to call this once
> * it is aware of potential suppliers becoming available.
>
>
>
> > + */
> > +static void device_link_check_waiting_consumers(void)
>
> Function name is misleading and hides side effects.
>
> I have not come up with a name that does not hide side effects, but a better
> name would be:
>
> device_link_add_supplier_links()
I kinda agree that it could afford a better name. The current name is
too similar to device_links_check_suppliers() and I never liked that.
Maybe device_link_add_missing_suppliers()?
I don't think we need "links" repeated twice in the function name.
With this suggestion, what side effect is hidden in your opinion? That
the fully linked consumer is removed from the "waiting for suppliers"
list?
Maybe device_link_try_removing_from_wfs()?
I'll wait for us to agree on a better name here before I change this.
> > +{
> > + struct device *dev, *tmp;
> > +
> > + mutex_lock(&wfs_lock);
> > + list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
> > + links.needs_suppliers)
> > + if (!dev->bus->add_links(dev))
> > + list_del_init(&dev->links.needs_suppliers);
>
> Empties dev->links.needs_suppliers, but does not remove dev from
> wait_for_suppliers list. Where does that happen?
I'll chalk this up to you having a long day or forgetting your coffee
:) list_del_init() does both of those things because needs_suppliers
is the node and wait_for_suppliers is the list.
>
> > + mutex_unlock(&wfs_lock);
> > +}
> > +
> > static void device_link_free(struct device_link *link)
> > {
> > while (refcount_dec_not_one(&link->rpm_active))
> > @@ -535,6 +582,19 @@ int device_links_check_suppliers(struct device *dev)
> > struct device_link *link;
> > int ret = 0;
> >
> > + /*
> > + * If a device is waiting for one or more suppliers (in
> > + * wait_for_suppliers list), it is not ready to probe yet. So just
> > + * return -EPROBE_DEFER without having to check the links with existing
> > + * suppliers.
> > + */
>
> Change comment to:
>
> /*
> * Device waiting for supplier to become available is not allowed
> * to probe
> */
Po-tay-to. Po-tah-to? I think my comment is just as good.
> > + mutex_lock(&wfs_lock);
> > + if (!list_empty(&dev->links.needs_suppliers)) {
> > + mutex_unlock(&wfs_lock);
> > + return -EPROBE_DEFER;
> > + }
> > + mutex_unlock(&wfs_lock);
> > +
> > device_links_write_lock();
>
> Update Documentation/driver-api/device_link.rst to reflect the
> check of &dev->links.needs_suppliers in device_links_check_suppliers().
Thanks! Will do.
>
> >
> > list_for_each_entry(link, &dev->links.suppliers, c_node) {
> > @@ -812,6 +872,10 @@ static void device_links_purge(struct device *dev)
> > {
> > struct device_link *link, *ln;
> >
> > + mutex_lock(&wfs_lock);
> > + list_del(&dev->links.needs_suppliers);
> > + mutex_unlock(&wfs_lock);
> > +
> > /*
> > * Delete all of the remaining links from this device to any other
> > * devices (either consumers or suppliers).
> > @@ -1673,6 +1737,7 @@ void device_initialize(struct device *dev)
> > #endif
> > INIT_LIST_HEAD(&dev->links.consumers);
> > INIT_LIST_HEAD(&dev->links.suppliers);
> > + INIT_LIST_HEAD(&dev->links.needs_suppliers);
> > dev->links.status = DL_DEV_NO_DRIVER;
> > }
> > EXPORT_SYMBOL_GPL(device_initialize);
> > @@ -2108,6 +2173,24 @@ int device_add(struct device *dev)
> > BUS_NOTIFY_ADD_DEVICE, dev);
> >
> > kobject_uevent(&dev->kobj, KOBJ_ADD);
>
> > +
> > + /*
> > + * Check if any of the other devices (consumers) have been waiting for
> > + * this device (supplier) to be added so that they can create a device
> > + * link to it.
> > + *
> > + * This needs to happen after device_pm_add() because device_link_add()
> > + * requires the supplier be registered before it's called.
> > + *
> > + * But this also needs to happe before bus_probe_device() to make sure
> > + * waiting consumers can link to it before the driver is bound to the
> > + * device and the driver sync_state callback is called for this device.
> > + */
>
> /*
> * Add links to dev from any dependent consumer that has dev on it's
> * list of needed suppliers
There is no list of needed suppliers.
> (links.needs_suppliers). Device_pm_add()
> * must have previously registered dev to allow the links to be added.
> *
> * The consumer links must be created before dev is probed because the
> * sync_state callback for dev will use the consumer links.
> */
I think what I wrote is just as clear.
>
> > + device_link_check_waiting_consumers();
> > +
> > + if (dev->bus && dev->bus->add_links && dev->bus->add_links(dev))
> > + device_link_wait_for_supplier(dev);
> > +
> > bus_probe_device(dev);
> > if (parent)
> > klist_add_tail(&dev->p->knode_parent,
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index c330b75c6c57..5d70babb7462 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -78,6 +78,17 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> > * -EPROBE_DEFER it will queue the device for deferred probing.
> > * @uevent: Called when a device is added, removed, or a few other things
> > * that generate uevents to add the environment variables.
>
> > + * @add_links: Called, perhaps multiple times per device, after a device is
> > + * added to this bus. The function is expected to create device
> > + * links to all the suppliers of the input device that are
> > + * available at the time this function is called. As in, the
> > + * function should NOT stop at the first failed device link if
> > + * other unlinked supplier devices are present in the system.
>
> * @add_links: Called after a device is added to this bus.
Why are you removing the "perhaps multiple times" part? that's true
and that's how some of the other ops are documented.
> The function is
> * expected to create device links to all the suppliers of the
> * device that are available at the time this function is called.
> * The function must NOT stop at the first failed device link if
> * other unlinked supplier devices are present in the system.
> * If some suppliers are not yet available, this function will be
> * called again when the suppliers become available.
>
> but add_links() not needed, so moving this comment to of_link_to_suppliers()
Sorry, I'm not sure I understand. Can you please explain what you are
trying to say? of_link_to_suppliers() is just one implementation of
add_links(). The comment above is try for any bus trying to implement
add_links().
>
>
> > + *
> > + * Return 0 if device links have been successfully created to all
> > + * the suppliers of this device. Return an error if some of the
> > + * suppliers are not yet available and this function needs to be
> > + * reattempted in the future.
>
> *
> * Return 0 if device links have been successfully created to all
> * the suppliers of this device. Return an error if some of the
> * suppliers are not yet available.
>
>
> > * @probe: Called when a new device or driver add to this bus, and callback
> > * the specific driver's probe to initial the matched device.
> > * @remove: Called when a device removed from this bus.
> > @@ -122,6 +133,7 @@ struct bus_type {
> >
> > int (*match)(struct device *dev, struct device_driver *drv);
> > int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
>
>
> > + int (*add_links)(struct device *dev);
>
> ^^^^^^^^^ add_supplier ???
> ^^^^^^^^^ add_suppliers ???
>
> ^^^^^^^^^ link_suppliers ???
>
> ^^^^^^^^^ add_supplier_dependency ???
> ^^^^^^^^^ add_supplier_dependencies ???
> add_links() not needed
add_links() was an intentional decision. There's no requirement that
the bus should only create links from this device to its suppliers. If
the bus also knows the consumers of this device (dev), then it
can/should add those too. So, it shouldn't have "suppliers" in the
name.
> > int (*probe)(struct device *dev);
> > int (*remove)(struct device *dev);
> > void (*shutdown)(struct device *dev);
>
>
>
>
> > @@ -893,11 +905,13 @@ enum dl_dev_state {
> > * struct dev_links_info - Device data related to device links.
> > * @suppliers: List of links to supplier devices.
> > * @consumers: List of links to consumer devices.
>
> > + * @needs_suppliers: Hook to global list of devices waiting for suppliers.
>
> * @needs_suppliers: List of devices deferring probe until supplier drivers
> * are successfully probed.
It's "need suppliers". As in, this is a device that "needs suppliers".
So, no, this is not a list. This is a node in a list. And all "nodes
in a list" are documented as "Hook" in rest of places in this file. So
I think the documentation is correct as is.
Thanks for your review.
-Saravana
