Message-ID: <a4c139c1-c9d1-3e5a-f47f-cd790b42da1f@gmail.com>
Date:   Mon, 19 Aug 2019 21:25:44 -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/19/19 5:00 PM, Saravana Kannan wrote:
> On Sun, Aug 18, 2019 at 8:38 PM Frank Rowand <frowand.list@...il.com> wrote:
>>
>> 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.
> 
> Thanks for the context.
> 
>> 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.
> 
> I'll take this as a compliment :)

Please do!


> 
>> 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.
> 
> Are you distinguishing between API level vs implementation level based
> on the function being "static"/not exported? I believe the earlier

No, being static helps say a function is not API, but an function that is
not static may be intended to be used in a limited and constrained manner.
I distinguished based on the usage of the function.


> version of this series had this function as an exported API. So maybe
> that's why I had it as "Mark device".
> 
>> 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
>>
> 
> Ok
> 
>> I did not ask why this feature is tied to _only_ the platform bus, but will now.
> 
> Because devicetree and platform bus the only ones I'm familiar with.
> If other busses want to add this, I'd be happy to help with code
> and/or direction/review. But I won't pretend to know anything about
> ACPI.

Sorry, you don't get to ignore other buses because you are not familiar
with them.

I am not aware of any reason to exclude devices that on other buses and your
answer below does not provide a valid technical reason why the new feature is
correct when it excludes all other buses.

> 
>> 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?
> 
> Yes, it's hypothetically possible. But I haven't seen such a
> relationship being defined in DT. Nor somewhere else where this might
> be captured. So, how common/realistic is it?

It is entirely legal.  I have no idea how common it is but that is not a valid
reason to exclude other buses from the feature.

> 
>>>
>>>>  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.
> 
> I'll take in all the ones I feel are reasonable or don't feel strongly
> about. We can revisit the rest later.
> 
>>>
>>>>     * 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

Reading what you say below, and looking at the code again, what I say
in that sentence is backwards.  It is not adding the missing supplier
device links, it is instead adding existing supplier device inks.


>> the name that I suggested.  We can keep thinking.
> 
> No, this function _IS_ about adding links to suppliers. These

You are mis-reading what I wrote.  I said the function "is not adding
the missing suppliers".  You are converting that to "is not adding
links to the missing suppliers".

My suggested name was hinting "add_supplier_links", which is what you
say it does below.  The name you suggest is hinting "add_missing_suppliers".
Do you see the difference?


> consumers were "saved" as "not yet having the supplier" earlier by
> device_link_wait_for_supplier(). This function doesn't do that. This
> function is just trying to see if those missing suppliers are present
> now and if so adding a link to them from the "saved" consumers. I
> think device_link_add_missing_suppliers() is actually a pretty good
> name. Let me know what you think now.
> 
>>
>>
>>>
>>> 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.
> 
> No, it doesn't do that. I can't keep a list of things that aren't
> allocated yet :). In the whole patch series, we only keep a list of things
> (consumers) that are waiting on other things (missing suppliers).

OK, as I noted above, I stated that backwards.  It is adding links for
existing suppliers, not for the missing suppliers.

> 
>>>
>>> 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.
> 
> It doesn't. How would you add a supplier device to a list if the
> device itself isn't there? :)

Again, that should be existing suppliers, as you noted.  But the point stands
that the function is potentially adding links.


> 
>>  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?
> 
> No, that's a list of consumers that needs_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.
> 
> Thanks for the pointer about fwnode and fwnode_operations. I wasn't
> aware of those. I see where you are going with this. I see a couple of
> problems with this approach though:
> 
> 1. How you interpret the properties of a fwnode is specific to the fw
> type. The clocks DT property isn't going to have the same definition
> in ACPI or some other firmware. Heck, I don't know if ACPI even has a
> clocks like property. So have one function to parse all the FW types
> doesn't make a lot of sense.

The functions in fwnode_operations are specific to the proper firmware.
So there is a set of functions in a struct fwnode_operations for
devicetree that only know about devicetree.  And there is a different
variable of type fwnode_operations that is initialized with ACPI
specific functions.

> 
> 2. If this common code is implemented as part of driver/base/, then at
> a minimum, I'll have to check if a fwnode is a DT node before I start
> interpreting the properties of a device's fwnode. But that means I'll
> have to include linux/of.h to use is_of_node(). I don't like having
> driver/base code depend on OF or platform or ACPI headers.

You just use the function in the device's fwnode_operations (I think,
I would have to go look at the precise way the code works because it
has been quite a while since I've looked at it).

> 
> 3. The supplier info doesn't always need to come from a firmware. So I
> don't want to limit it to that?

If you can find another source of topology info, then I would expect
that another set of fwnode_operations functions would be created
for the info source.


> 
> Also, I don't necessarily see this as conflating firmware (DT, ACPI,
> etc) with the bus (platform bus, ACPI bus, PCI bus). Whoever creates
> the device seems like the entity best suited to figure out the
> suppliers of the device (apart from the driver, obviously). So the bus
> deciding the suppliers doesn't seem wrong to me.

Patch 3 assigns the devicetree add_links function to the platform bus.
It seems incorrect to me for of_platform_default_populate_init() to be
changing a field in platform_bus_type.


   of_platform_default_populate_init()
           ...
           platform_bus_type.add_links = of_link_to_suppliers;


> 
> In this specific case, I'm trying to address DT for now and leaving
> ACPI to whoever else wants to add device links based on ACPI data.
> Most OF/DT based devices end up in platform bus. So I'm just handling
> this in platform bus. If some other person wants this to work for ACPI
> bus or PCI bus, they are welcome to implement add_links() for those
> busses? I'm nowhere close to an expert on those.

Devicetree is not limited to the platform bus.


> 
>>>
>>>>
>>>>
>>>>> + *
>>>>> + *           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.
>>
> 
> Thanks for the review.
> 
> -Saravana
> 

Powered by blists - more mailing lists