Message-ID: <522e8375-5070-f579-6509-3e44fe66768e@gmail.com>
Date:   Sun, 18 Aug 2019 20:38:46 -0700
From:   Frank Rowand <frowand.list@...il.com>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        David Collins <collinsd@...eaurora.org>,
        Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [PATCH v7 1/7] driver core: Add support for linking devices
 during device addition

On 8/15/19 6:50 PM, Saravana Kannan wrote:
> On Wed, Aug 7, 2019 at 7:04 PM Frank Rowand <frowand.list@...il.com> 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@...gle.com>
>>>
>>> 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@...gle.com>
>>> ---
>>>  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
> 

As a meta-comment, I found this series very hard to understand in the context
of reading the new code for the first time.  When I read the code again in
six months or a year or two years it will not be in near term memory and it
will be as if I am reading it for the first time.  A lot of my suggestions
for changes of names are in that context -- the current names may be fine
when one has recently read the code, but not so much when trying to read
the whole thing again with a blank mind.

The code also inherits a good deal of complexity because it does not stand
alone in a nice discrete chunk, but instead delicately weaves into a more
complex body of code.

When I was trying to understand the code, I wrote a lot of additional
comments within my reply email to provide myself context, information
about various things, and questions that I needed to answer (or if I
could not answer to then ask you).  Then I ended up being able to remove
many of those notes before sending the reply.


> 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.

I agree with the intent of trying to describe the semantics of a function,
especially at the API level where other systems (or drivers) would be using
the function.  But for this case the function is at the implementation level
and describing explicitly what it is doing makes this much more readable for
me.

I also find "Mark device" to be vague and not descriptive of what the
intent is.

> 
>>
>>
>>> + * @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.

My comment is left over from when I was taking notes, trying to understand the
code.

At the moment, only devicetree is used as a source of the dependency information.
The comment would better be re-phrased as:

        * Scan all consumer devices in the firmware description of the hardware topology

I did not ask why this feature is tied to _only_ the platform bus, but will now.

I do not know of any reason that a consumer / supplier relationship can not be
between devices on different bus types.  Do you know of such a reason?


> 
>>  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.

Note that my version of these comments was written while I was reading the code,
and did not have any big picture understanding yet.  This will likely also be
the mind set of most everyone who reads this code in the future, once it is
woven into the kernel.

If you don't like the change, I can revisit it in a later version of the
patch set.


> 
>>     * 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.

Naming new fields or variables related to device links looks pretty
challenging to me, because of the desire to be part of device links
and not a wart pasted on the side.  So I share the pain in trying
to find good names.

> 
> Maybe device_link_add_missing_suppliers()?

My first reaction was "yes, that sounds good".  But then I stopped and
tried to read the name out of context.  The name is not adding the
missing suppliers, it is saving the information that a supplier is
not yet available (eg, is "missing").  I struggled in coming up with
the name that I suggested.  We can keep thinking.


> 
> I don't think we need "links" repeated twice in the function name.

Yeah, I didn't like that either.


> With this suggestion, what side effect is hidden in your opinion? That
> the fully linked consumer is removed from the "waiting for suppliers"
> list?

The side effect is that the function does not merely do a check.  It also
adds missing suppliers to a list.


> 
> Maybe device_link_try_removing_from_wfs()?

I like that, other than the fact that it still does not provide a clue
that the function is potentially adding suppliers to a list.  I think
part of the challenge is that the function does two things: (1) a check,
and (2) potentially adding missing suppliers to a list.  Maybe a simple
one line comment at the call site, something like:

   /* adds missing suppliers to 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.

Yes, brain mis-fire on my part.  I'll have to go back and look at the
list related code again.


> 
>>
>>> +     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.

If just as good and shorter, then better.

Also the original says "it is not ready to probe".  That is not correct.  It
is ready to probe, it is just that the probe attempt will return -EPROBE_DEFER.
Nit picky on my part, but tiny things like that mean I have to think harder.
I have to think "why is it not ready to probe?".  Maybe my version should have
instead been something like:

        * Device waiting for supplier to become available will return
        * -EPROBE_DEFER if probed.  Avoid the unneeded processing.

> 
>>> +     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.

"the other devices (consumers) have been waiting for this device (supplier)".
Isn't that a 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.

The original comment is vague.  It does not explain why consumer links must be
created before the probe.  I had to go off and read other code to determine
why that is true.

And again, brevity is better if otherwise 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.

I didn't remove it.  I rephrased it with a little bit more explanation as
"If some suppliers are not yet available, this function will be
called again when the suppliers become available." (below).


> 
>>  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().

This is conflating bus with the source of the firmware description of the
hardware topology.  For drivers that use various APIs to access firmware
description of topology that may be either devicetree or ACPI the access
is done via fwnode_operations, based on struct device.fwnode (if I recall
properly).

I failed to completely address why add_links() is not needed.  The answer
is that there should be a single function called for all buses.  Then
the proper firmware data source would be accessed via a struct fwnode_operations.

I think I left this out because I had not yet asked why this feature is
tied only to the platform bus.  Which I asked earlier in this reply.

> 
>>
>>
>>> + *
>>> + *           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.

Is creating links to consumers of this device implemented in this patch
series?  If so, I overlooked it and will have to consider how that
fits in to the design.


> 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.

Aha, I got confused about that while trying to keep everything straight.

Somehow I managed to conflate needs_suppliers with the links between
consumers and suppliers that are create via device_link_add().

So original comment is fine.

It is getting late, so I'll continue with patches 2 and 3 tomorrow.

-Frank

> 
> Thanks for your review.
> 
> 
> 
> -Saravana
> 

Powered by blists - more mailing lists