> 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.
>
> 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
> + * @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
* Scan all consumer devices in the devicetree. 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).
* 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()
> +{
> + 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?
> + 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
*/
> + 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().
>
> 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 (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.
*/
> + 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. 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()
> + *
> + * 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
> 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.
> * @status: Driver status information.
> */
> struct dev_links_info {
> struct list_head suppliers;
> struct list_head consumers;
> + struct list_head needs_suppliers;
> enum dl_dev_state status;
> };
>
> --
> 2.22.0.709.g102302147b-goog
>
>
