Re: [PATCH 0/2] Device links documentation

Re: [PATCH 0/2] Device links documentation

[Mauro Carvalho Chehab]

Hi Lukas,

Em Sun, 4 Dec 2016 13:10:04 +0100
Lukas Wunner <lukas@wunner.de> escreveu:

> Here's my suggestion for initial documentation on the device links
> feature that's queued for 4.10 on Greg KH's driver-core-next branch.
> Please let me know if you have any additions or corrections.
> 
> To read this in rendered form:
> http://wunner.de/kernel-doc/core-api/device_link.html
> 
> Patch [2/2] could go in via driver-core-next, but patch [1/2] would
> need to go in via docs-next (because the core-api directory doesn't
> exist yet in driver-core-next).
>
> @Jonathan Corbet:  Is core-api the right place to put this? An
> alternative would be Documentation/driver-api, but unlike core-api
> it contains less prose text but mostly just bare API docs gleaned
> from kernel-doc.

Just my 2 cents here, but, IMHO, this belongs to driver-api.
Yeah, it contains less prose, with is, IMHO, something that should
be improved ;)

Regards,
Mauro

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Mauro Carvalho Chehab]

Em Sun, 4 Dec 2016 13:10:04 +0100
Lukas Wunner <lukas@wunner.de> escreveu:

> Document device links as introduced in v4.10 with commits:
>     4bdb35506b89 ("driver core: Add a wrapper around
>                    __device_release_driver()")
>     9ed9895370ae ("driver core: Functional dependencies tracking
>                    support")
>     8c73b4288496 ("PM / sleep: Make async suspend/resume of devices use
>                    device links")
>     21d5c57b3726 ("PM / runtime: Use device links")
>     baa8809f6097 ("PM / runtime: Optimize the use of device links")
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Silvio Fricke <silvio.fricke@gmail.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Kukjin Kim <kgene@kernel.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-samsung-soc@vger.kernel.org
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  Documentation/core-api/device_link.rst | 279 +++++++++++++++++++++++++++++++++
>  Documentation/core-api/index.rst       |   8 +
>  2 files changed, 287 insertions(+)
>  create mode 100644 Documentation/core-api/device_link.rst
> 
> diff --git a/Documentation/core-api/device_link.rst b/Documentation/core-api/device_link.rst
> new file mode 100644
> index 0000000..5f57134
> --- /dev/null
> +++ b/Documentation/core-api/device_link.rst
> @@ -0,0 +1,279 @@
> +============
> +Device links
> +============
> +
> +By default, the driver core only enforces dependencies between devices
> +that are borne out of a parent/child relationship within the device
> +hierarchy: When suspending, resuming or shutting down the system, devices
> +are ordered based on this relationship, i.e. children are always suspended
> +before their parent, and the parent is always resumed before its children.
> +
> +Sometimes there is a need to represent device dependencies beyond the
> +mere parent/child relationship, e.g. between siblings, and have the
> +driver core automatically take care of them.
> +
> +Secondly, the driver core by default does not enforce any driver presence
> +dependencies, i.e. that one device must be bound to a driver before
> +another one can probe or function correctly.
> +
> +Often these two dependency types come together, so a device depends on
> +another one both with regards to driver presence *and* with regards to
> +suspend/resume and shutdown ordering.
> +
> +Device links allow representation of such dependencies in the driver core.
> +
> +In its standard form, a device link combines *both* dependency types:
> +It guarantees correct suspend/resume and shutdown ordering between a
> +"supplier" device and its "consumer" devices, and it guarantees driver
> +presence on the supplier.  The consumer devices are not probed before the
> +supplier is bound to a driver, and they're unbound before the supplier
> +is unbound.
> +
> +When driver presence on the supplier is irrelevant and only correct
> +suspend/resume and shutdown ordering is needed, the device link may
> +simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> +enforcing driver presence on the supplier is optional.
> +
> +Another optional feature is runtime PM integration:  By setting the
> +``DL_FLAG_PM_RUNTIME`` flag on addition of the device link, the PM core
> +is instructed to runtime resume the supplier and keep it active
> +whenever and for as long as the consumer is runtime resumed.
> +
> +Usage
> +=====

You should be using, instead:

Usage
-----

(and the same '-' symbol for all sections of this chapter)

The way you did, in thesis, ReST should be putting all tags at the
same level as the first one, as they're all using '='. 

The other ReST markups look OK on my eyes.

Regards,
Mauro

> +
> +The earliest point in time when device links can be added is after
> +:c:func:`device_add()` has been called for the supplier and
> +:c:func:`device_initialize()` has been called for the consumer.
> +
> +It is legal to add them later, but care must be taken that the system
> +remains in a consistent state:  E.g. a device link cannot be added in
> +the midst of a suspend/resume transition, so either commencement of
> +such a transition needs to be prevented with :c:func:`lock_system_sleep()`,
> +or the device link needs to be added from a function which is guaranteed
> +not to run in parallel to a suspend/resume transition, such as from a
> +device ``->probe`` callback or a boot-time PCI quirk.
> +
> +Another example for an inconsistent state would be a device link that
> +represents a driver presence dependency, yet is added from the consumer's
> +``->probe`` callback while the supplier hasn't probed yet:  Had the driver
> +core known about the device link earlier, it wouldn't have probed the
> +consumer in the first place.  The onus is thus on the consumer to check
> +presence of the supplier after adding the link, and defer probing on
> +non-presence.
> +
> +If a device link is added in the ``->probe`` callback of the supplier or
> +consumer driver, it is typically deleted in its ``->remove`` callback for
> +symmetry.  That way, if the driver is compiled as a module, the device
> +link is added on module load and orderly deleted on unload.  The same
> +restrictions that apply to device link addition (e.g. exclusion of a
> +parallel suspend/resume transition) apply equally to deletion.
> +
> +Several flags may be specified on device link addition, two of which
> +have already been mentioned above:  ``DL_FLAG_STATELESS`` to express that no
> +driver presence dependency is needed (but only correct suspend/resume and
> +shutdown ordering) and ``DL_FLAG_PM_RUNTIME`` to express that runtime PM
> +integration is desired.
> +
> +Two other flags are specifically targeted at use cases where the device
> +link is added from the consumer's ``->probe`` callback:  ``DL_FLAG_RPM_ACTIVE``
> +can be specified to runtime resume the supplier upon addition of the
> +device link.  ``DL_FLAG_AUTOREMOVE`` causes the device link to be automatically
> +purged when the consumer fails to probe or later unbinds.  This obviates
> +the need to explicitly delete the link in the ``->remove`` callback or in
> +the error path of the ``->probe`` callback.
> +
> +Limitations
> +===========
> +
> +Driver authors should be aware that a driver presence dependency (i.e. when
> +``DL_FLAG_STATELESS`` is not specified on link addition) may cause probing of
> +the consumer to be deferred indefinitely.  This can become a problem if the
> +consumer is required to probe before a certain initcall level is reached.
> +Worse, if the supplier driver is blacklisted or missing, the consumer will
> +never be probed.
> +
> +Sometimes drivers depend on optional resources.  They are able to operate
> +in a degraded mode (reduced feature set or performance) when those resources
> +are not present.  An example is an SPI controller that can use a DMA engine
> +or work in PIO mode.  The controller can determine presence of the optional
> +resources at probe time but on non-presence there is no way to know whether
> +they will become available in the near future (due to a supplier driver
> +probing) or never.  Consequently it cannot be determined whether to defer
> +probing or not.  It would be possible to notify drivers when optional
> +resources become available after probing, but it would come at a high cost
> +for drivers as switching between modes of operation at runtime based on the
> +availability of such resources would be much more complex than a mechanism
> +based on probe deferral.  In any case optional resources are beyond the
> +scope of device links.
> +
> +Examples
> +========
> +
> +* An MMU device exists alongside a busmaster device, both are in the same
> +  power domain.  The MMU implements DMA address translation for the busmaster
> +  device and shall be runtime resumed and kept active whenever and as long
> +  as the busmaster device is active.  The busmaster device's driver shall
> +  not bind before the MMU is bound.  To achieve this, a device link with
> +  runtime PM integration is added from the busmaster device (consumer)
> +  to the MMU device (supplier).  The effect with regards to runtime PM
> +  is the same as if the MMU was the parent of the master device.
> +
> +  The fact that both devices share the same power domain would normally
> +  suggest usage of a :c:type:`struct dev_pm_domain` or :c:type:`struct
> +  generic_pm_domain`, however these are not independent devices that
> +  happen to share a power switch, but rather the MMU device serves the
> +  busmaster device and is useless without it.  A device link creates a
> +  synthetic hierarchical relationship between the devices and is thus
> +  more apt.
> +
> +* A Thunderbolt host controller comprises a number of PCIe hotplug ports
> +  and an NHI device to manage the PCIe switch.  On resume from system sleep,
> +  the NHI device needs to re-establish PCI tunnels to attached devices
> +  before the hotplug ports can resume.  If the hotplug ports were children
> +  of the NHI, this resume order would automatically be enforced by the
> +  PM core, but unfortunately they're aunts.  The solution is to add
> +  device links from the hotplug ports (consumers) to the NHI device
> +  (supplier).  A driver presence dependency is not necessary for this
> +  use case.
> +
> +* Discrete GPUs in hybrid graphics laptops often feature an HDA controller
> +  for HDMI/DP audio.  In the device hierarchy the HDA controller is a sibling
> +  of the VGA device, yet both share the same power domain and the HDA
> +  controller is only ever needed when an HDMI/DP display is attached to the
> +  VGA device.  A device link from the HDA controller (consumer) to the
> +  VGA device (supplier) aptly represents this relationship.
> +
> +* ACPI allows definition of a device start order by way of _DEP objects.
> +  A classical example is when ACPI power management methods on one device
> +  are implemented in terms of I\ :sup:`2`\ C accesses and require a specific
> +  I\ :sup:`2`\ C controller to be present and functional for the power
> +  management of the device in question to work.
> +
> +* In some SoCs a functional dependency exists from display, video codec and
> +  video processing IP cores on transparent memory access IP cores that handle
> +  burst access and compression/decompression.
> +
> +Alternatives
> +============
> +
> +* A :c:type:`struct dev_pm_domain` can be used to override the bus,
> +  class or device type callbacks.  It is intended for devices sharing
> +  a single on/off switch, however it does not guarantee a specific
> +  suspend/resume ordering, this needs to be implemented separately.
> +  It also does not by itself track the runtime PM status of the involved
> +  devices and turn off the power switch only when all of them are runtime
> +  suspended.  Furthermore it cannot be used to enforce a specific shutdown
> +  ordering or a driver presence dependency.
> +
> +* A :c:type:`struct generic_pm_domain` is a lot more heavyweight than a
> +  device link and does not allow for shutdown ordering or driver presence
> +  dependencies.  It also cannot be used on ACPI systems.
> +
> +Implementation
> +==============
> +
> +The device hierarchy, which -- as the name implies -- is a tree,
> +becomes a directed acyclic graph once device links are added.
> +
> +Ordering of these devices during suspend/resume is determined by the
> +dpm_list.  During shutdown it is determined by the devices_kset.  With
> +no device links present, the two lists are a flattened, one-dimensional
> +representations of the device tree such that a device is placed behind
> +all its ancestors.  That is achieved by traversing the ACPI namespace
> +or OpenFirmware device tree top-down and appending devices to the lists
> +as they are discovered.
> +
> +Once device links are added, the lists need to satisfy the additional
> +constraint that a device is placed behind all its suppliers, recursively.
> +To ensure this, upon addition of the device link the consumer and the
> +entire sub-graph below it (all children and consumers of the consumer)
> +are moved to the end of the list.  (Call to :c:func:`device_reorder_to_tail()`
> +from :c:func:`device_link_add()`.)
> +
> +To prevent introduction of dependency loops into the graph, it is
> +verified upon device link addition that the supplier is not dependent
> +on the consumer or any children or consumers of the consumer.
> +(Call to :c:func:`device_is_dependent()` from :c:func:`device_link_add()`.)
> +If that constraint is violated, :c:func:`device_link_add()` will return
> +``NULL`` and a ``WARNING`` will be logged.
> +
> +Notably this also prevents the addition of a device link from a parent
> +device to a child.  However the converse is allowed, i.e. a device link
> +from a child to a parent.  Since the driver core already guarantees
> +correct suspend/resume and shutdown ordering between parent and child,
> +such a device link only makes sense if a driver presence dependency is
> +needed on top of that.  In this case driver authors should weigh
> +carefully if a device link is at all the right tool for the purpose.
> +A more suitable approach might be to simply use deferred probing or
> +add a device flag causing the parent driver to be probed before the
> +child one.
> +
> +State machine
> +=============
> +
> +.. kernel-doc:: include/linux/device.h
> +   :functions: device_link_state
> +
> +::
> +
> +                 .=============================.
> +                 |                             |
> +                 v                             |
> + DORMANT <=> AVAILABLE <=> CONSUMER_PROBE => ACTIVE
> +    ^                                          |
> +    |                                          |
> +    '============ SUPPLIER_UNBIND <============'
> +
> +* The initial state of a device link is automatically determined by
> +  :c:func:`device_link_add()` based on the driver presence on the supplier
> +  and consumer.  If the link is created before any devices are probed, it
> +  is set to ``DL_STATE_DORMANT``.
> +
> +* When a supplier device is bound to a driver, links to its consumers
> +  progress to ``DL_STATE_AVAILABLE``.
> +  (Call to :c:func:`device_links_driver_bound()` from
> +  :c:func:`driver_bound()`.)
> +
> +* Before a consumer device is probed, presence of supplier drivers is
> +  verified by checking that links to suppliers are in ``DL_STATE_AVAILABLE``
> +  state.  The state of the links is updated to ``DL_STATE_CONSUMER_PROBE``.
> +  (Call to :c:func:`device_links_check_suppliers()` from
> +  :c:func:`really_probe()`.)
> +  This prevents the supplier from unbinding.
> +  (Call to :c:func:`wait_for_device_probe()` from
> +  :c:func:`device_links_unbind_consumers()`.)
> +
> +* If the probe fails, links to suppliers revert back to ``DL_STATE_AVAILABLE``.
> +  (Call to :c:func:`device_links_no_driver()` from :c:func:`really_probe()`.)
> +
> +* If the probe succeeds, links to suppliers progress to ``DL_STATE_ACTIVE``.
> +  (Call to :c:func:`device_links_driver_bound()` from :c:func:`driver_bound()`.)
> +
> +* When the consumer's driver is later on removed, links to suppliers revert
> +  back to ``DL_STATE_AVAILABLE``.
> +  (Call to :c:func:`__device_links_no_driver()` from
> +  :c:func:`device_links_driver_cleanup()`, which in turn is called from
> +  :c:func:`__device_release_driver()`.)
> +
> +* Before a supplier's driver is removed, links to consumers that are not
> +  bound to a driver are updated to ``DL_STATE_SUPPLIER_UNBIND``.
> +  (Call to :c:func:`device_links_busy()` from
> +  :c:func:`__device_release_driver()`.)
> +  This prevents the consumers from binding.
> +  (Call to :c:func:`device_links_check_suppliers()` from
> +  :c:func:`really_probe()`.)
> +  Consumers that are bound are freed from their driver; consumers that are
> +  probing are waited for until they are done.
> +  (Call to :c:func:`device_links_unbind_consumers()` from
> +  :c:func:`__device_release_driver()`.)
> +  Once all links to consumers are in ``DL_STATE_SUPPLIER_UNBIND`` state,
> +  the supplier driver is released and the links revert to ``DL_STATE_DORMANT``.
> +  (Call to :c:func:`device_links_driver_cleanup()` from
> +  :c:func:`__device_release_driver()`.)
> +
> +API
> +===
> +
> +.. kernel-doc:: drivers/base/core.c
> +   :functions: device_link_add device_link_del
> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index 2872ca1..2aed045 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -16,6 +16,14 @@ Core utilities
>     local_ops
>     workqueue
>  
> +Devices
> +=======
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   device_link
> +
>  Interfaces for kernel debugging
>  ===============================
>  


-- 
Thanks,
Mauro

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Jonathan Corbet]

On Mon, 5 Dec 2016 10:07:39 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:

> > +Usage
> > +=====  
> 
> You should be using, instead:
> 
> Usage
> -----
> 
> (and the same '-' symbol for all sections of this chapter)
> 
> The way you did, in thesis, ReST should be putting all tags at the
> same level as the first one, as they're all using '='. 

Actually, he did exactly what the documentation guide says to do.  Since
it only has the markers below the heading, it will still be at a lower
level than the top heading, which has markers both above and below.

jon

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Mauro Carvalho Chehab]

Em Mon, 5 Dec 2016 06:05:07 -0700
Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org> escreveu:

> On Mon, 5 Dec 2016 10:07:39 -0200
> Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> wrote:
> 
> > > +Usage
> > > +=====  
> > 
> > You should be using, instead:
> > 
> > Usage
> > -----
> > 
> > (and the same '-' symbol for all sections of this chapter)
> > 
> > The way you did, in thesis, ReST should be putting all tags at the
> > same level as the first one, as they're all using '='. 
> 
> Actually, he did exactly what the documentation guide says to do. 

Ah, OK! I guess I misread that section of the documentation.

> Since
> it only has the markers below the heading, it will still be at a lower
> level than the top heading, which has markers both above and below.

Yes, I noticed that it is on a lower level than the initial one with
markers above and below.


Lukas,

Documentation seems OK, from ReST PoV. Didn't read the documents
and the C code to be sure that they match the implementation.

Reviewed-by: Mauro Carvalho Chehab <mchehab-JsYNTwtnfakRB7SZvlqPiA@public.gmane.org>


-- 
Thanks,
Mauro

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Jonathan Corbet]

On Sun, 4 Dec 2016 13:10:04 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> Document device links as introduced in v4.10 with commits:
>     4bdb35506b89 ("driver core: Add a wrapper around
>                    __device_release_driver()")
>     9ed9895370ae ("driver core: Functional dependencies tracking
>                    support")
>     8c73b4288496 ("PM / sleep: Make async suspend/resume of devices use
>                    device links")
>     21d5c57b3726 ("PM / runtime: Use device links")
>     baa8809f6097 ("PM / runtime: Optimize the use of device links")

I went ahead and applied this one to docs-next.  I did, however, take the
liberty of shifting it over to the driver-api manual.

Thanks,

jon

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Luis R. Rodriguez]

On Sun, Dec 04, 2016 at 01:10:04PM +0100, Lukas Wunner wrote:
> Document device links as introduced in v4.10 with commits:
>     4bdb35506b89 ("driver core: Add a wrapper around
>                    __device_release_driver()")
>     9ed9895370ae ("driver core: Functional dependencies tracking
>                    support")
>     8c73b4288496 ("PM / sleep: Make async suspend/resume of devices use
>                    device links")
>     21d5c57b3726 ("PM / runtime: Use device links")
>     baa8809f6097 ("PM / runtime: Optimize the use of device links")
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

First, thanks for doing this work.

> ---
>  Documentation/core-api/device_link.rst | 279 +++++++++++++++++++++++++++++++++
>  Documentation/core-api/index.rst       |   8 +
>  2 files changed, 287 insertions(+)
>  create mode 100644 Documentation/core-api/device_link.rst
> 
> diff --git a/Documentation/core-api/device_link.rst b/Documentation/core-api/device_link.rst
> new file mode 100644
> index 0000000..5f57134
> --- /dev/null
> +++ b/Documentation/core-api/device_link.rst
> @@ -0,0 +1,279 @@
> +============
> +Device links
> +============
> +
> +By default, the driver core only enforces dependencies between devices
> +that are borne out of a parent/child relationship within the device
> +hierarchy: 

This "device hierarchy" was rather confusing to grasp, I specially hard
a hard time understanding *why* the device links work did not do any
topological sorting at all. I'm also afraid this is not just "tribal
knowledge" -- not everyone was able to answer my questions about these
things, Rafafel however did do justice to some degree and I did provide notes
from this. I think it would be important then to jot down to help humans
grok:

  o where the implicit order we embrace comes from
  o what mechanisms are used to help provide some of this order
  o why this is *not* sufficient

The last one justifies the work. And more importantly, it may help
replace a whole bunch of other parallel work which had similar type of
solutions. To what extent it can do that well remains to be seen but
from what I can tell, that's future work worth looking into.

<-- snip -->

> +Limitations
> +===========
> +
> +Driver authors should be aware that a driver presence dependency (i.e. when
> +``DL_FLAG_STATELESS`` is not specified on link addition) may cause probing of
> +the consumer to be deferred indefinitely.  This can become a problem if the
> +consumer is required to probe before a certain initcall level is reached.
> +Worse, if the supplier driver is blacklisted or missing, the consumer will
> +never be probed.
> +
> +Sometimes drivers depend on optional resources.  They are able to operate
> +in a degraded mode (reduced feature set or performance) when those resources
> +are not present.  An example is an SPI controller that can use a DMA engine
> +or work in PIO mode.  The controller can determine presence of the optional
> +resources at probe time but on non-presence there is no way to know whether
> +they will become available in the near future (due to a supplier driver
> +probing) or never.  Consequently it cannot be determined whether to defer
> +probing or not.  It would be possible to notify drivers when optional
> +resources become available after probing, but it would come at a high cost
> +for drivers as switching between modes of operation at runtime based on the
> +availability of such resources would be much more complex than a mechanism
> +based on probe deferral.  In any case optional resources are beyond the
> +scope of device links.

Well you also forgot to mention the issues with deferred probe. It uses
deferred probe so if for whatever reason your subsystem or driver has issues
with it, this won't help.

<-- snip -->
> +* ACPI allows definition of a device start order by way of _DEP objects.
> +  A classical example is when ACPI power management methods on one device
> +  are implemented in terms of I\ :sup:`2`\ C accesses and require a specific
> +  I\ :sup:`2`\ C controller to be present and functional for the power
> +  management of the device in question to work.

Here is an example of current mechanisms used which to driver developers
provides implicit order --its all magical. This has can have limitations,
its important to annotate how this is limiting and also why this perhaps
was not considered as an enhancement in ACPI space. Ie, if semantics
existed in ACPI space for dependency info, why are we now left to our hims
for some cases on driver code?

> +
> +Alternatives
> +============
> +
> +* A :c:type:`struct dev_pm_domain` can be used to override the bus,
> +  class or device type callbacks.  It is intended for devices sharing
> +  a single on/off switch, however it does not guarantee a specific
> +  suspend/resume ordering, this needs to be implemented separately.
> +  It also does not by itself track the runtime PM status of the involved
> +  devices and turn off the power switch only when all of them are runtime
> +  suspended.  Furthermore it cannot be used to enforce a specific shutdown
> +  ordering or a driver presence dependency.
> +
> +* A :c:type:`struct generic_pm_domain` is a lot more heavyweight than a
> +  device link and does not allow for shutdown ordering or driver presence
> +  dependencies.  It also cannot be used on ACPI systems.

I provided a list of other frameworks in the kernel which have
their own solutions which are worth looking into, if anything to at
least determine if we have older frameworks to replace or to use
them to help complement device links framework to help with other
areas other "firmware tools" clearly are lacking.

> +
> +Implementation
> +==============
> +
> +The device hierarchy, which -- as the name implies -- is a tree,
> +becomes a directed acyclic graph once device links are added.

<insert sarcasm>

Oh look a DAG without any sort. How did that happen?

</end sarcasm>
> +
> +Ordering of these devices during suspend/resume is determined by the
> +dpm_list.

And where did order from that come from? My notes provided answers to
these questions.

  Luis

Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Lukas Wunner]

[got complaints on length of Cc: header, so trimming a bit]

On Tue, Dec 06, 2016 at 02:41:50AM +0100, Luis R. Rodriguez wrote:
> On Sun, Dec 04, 2016 at 01:10:04PM +0100, Lukas Wunner wrote:
> > +============
> > +Device links
> > +============
> > +
> > +By default, the driver core only enforces dependencies between devices
> > +that are borne out of a parent/child relationship within the device
> > +hierarchy: 
> 
> This "device hierarchy" was rather confusing to grasp, I specially hard
> a hard time understanding *why* the device links work did not do any
> topological sorting at all. I'm also afraid this is not just "tribal
> knowledge" -- not everyone was able to answer my questions about these
> things, Rafafel however did do justice to some degree and I did provide notes
> from this. I think it would be important then to jot down to help humans
> grok:
> 
>   o where the implicit order we embrace comes from
>   o what mechanisms are used to help provide some of this order

I'm already explaining this further down in the "Implementation" section:

	"That is achieved by traversing the ACPI namespace or OpenFirmware
	 device tree top-down and appending devices to the lists as they
	 are discovered."

If this is not sufficiently clear, please let me know how to improve it.

For didactic reasons I can't explain everything in the opening paragraph,
but the device hierarchy is exposed in sysfs and I think even casual
users might have seen it, certainly driver authors, and so the term
"device hierarchy" may evoke the right connotations and readers may even
realize that it's pre-determined by the platform, the kernel basically
just discovers it.


>   o why this is *not* sufficient

Well the introduction section already explains this abstractly:

	"Sometimes there is a need to represent device dependencies
	 beyond the mere parent/child relationship, e.g. between
	 siblings, and have the driver core automatically take care
	 of them.  Secondly, the driver core by default does not
	 enforce any driver presence dependencies, i.e. that one
	 device must be bound to a driver before another one can
	 probe or function correctly. [...] Device links allow
	 representation of such dependencies in the driver core."

And the "Examples" section further down presents specific use cases.

So I'd expect that the need for device links is already made
sufficiently clear to the reader.  Again, please let me know
if you have specific ideas to improve this.


> > +Limitations
> > +===========
> > +
>
> Well you also forgot to mention the issues with deferred probe. It uses
> deferred probe so if for whatever reason your subsystem or driver has issues
> with it, this won't help.

This very section does mention that deferred probing may cause issues:

	"Driver authors should be aware that a driver presence dependency
	 (i.e. when DL_FLAG_STATELESS is not specified on link addition)
	 may cause probing of the consumer to be deferred indefinitely.
	 This can become a problem if the consumer is required to probe
	 before a certain initcall level is reached.  Worse, if the
	 supplier driver is blacklisted or missing, the consumer will
	 never be probed."

Again I'd surmise that the deferred probing issue is sufficiently
explained, if you think otherwise please explain more specifically
what is missing.


> > +* ACPI allows definition of a device start order by way of _DEP objects.
> > +  A classical example is when ACPI power management methods on one device
> > +  are implemented in terms of I\ :sup:`2`\ C accesses and require a specific
> > +  I\ :sup:`2`\ C controller to be present and functional for the power
> > +  management of the device in question to work.
> 
> Here is an example of current mechanisms used which to driver developers
> provides implicit order --its all magical. This has can have limitations,
> its important to annotate how this is limiting and also why this perhaps
> was not considered as an enhancement in ACPI space. Ie, if semantics
> existed in ACPI space for dependency info, why are we now left to our hims
> for some cases on driver code?

If I understood Rafael correctly, Linux currently does not handle _DEP
objects correctly or at all.  So this isn't an alternative to device links,
but rather something that we can finally handle correctly with device links.
(@Rafael:  Please correct me if I'm wrong.)

Hence this is listed in the "Examples" section.  It's a future use case for
device links.

In drivers/acpi/scan.c I can see that _DEP objects are scanned and stored
on an acpi_dep_list, but that list isn't used anywhere else.  Presumably
in the future (once device links have landed in Linus' tree) we're going
to traverse the list after scanning the namespace and add a device link
for each dependency.


> > +Alternatives
> > +============
> > +
> > +* A :c:type:`struct dev_pm_domain` can be used to override the bus,
> > +  class or device type callbacks.  It is intended for devices sharing
> > +  a single on/off switch, however it does not guarantee a specific
> > +  suspend/resume ordering, this needs to be implemented separately.
> > +  It also does not by itself track the runtime PM status of the involved
> > +  devices and turn off the power switch only when all of them are runtime
> > +  suspended.  Furthermore it cannot be used to enforce a specific shutdown
> > +  ordering or a driver presence dependency.
> > +
> > +* A :c:type:`struct generic_pm_domain` is a lot more heavyweight than a
> > +  device link and does not allow for shutdown ordering or driver presence
> > +  dependencies.  It also cannot be used on ACPI systems.
> 
> I provided a list of other frameworks in the kernel which have
> their own solutions which are worth looking into, if anything to at
> least determine if we have older frameworks to replace or to use
> them to help complement device links framework to help with other
> areas other "firmware tools" clearly are lacking.

So you mentioned explicitly "DAPM, the DRM / Audio component framework,
v4l async solution".

By "DRM / Audio component framework" I assume you're referring to
vga_switcheroo.  That one I know fairly well and as I've explained
in previous e-mails it's kind of broken and I'm actually looking
into replacing it with device links myself.  Therefore I've listed
this as a use case in the "Examples" section, because we'll hopefully
get rid of it soon.

The two other ones, "DAPM" and "v4l async solution", I know nothing
about.

Marek Szyprowski wrote:  "components and v4l-async solutions are for
resolving only probe and registration issues and they are some kind
of pool for grouping devices and triggering special callback once all
requested devices in the pool have probed"

It would probably be good if someone familiar with "DAPM" and
"v4l async solution" could look at the device links documentation,
determine whether they're a potential use case of or alternative to
device links, and amend the documentation accordingly.


> > +
> > +Implementation
> > +==============
> > +
> > +The device hierarchy, which -- as the name implies -- is a tree,
> > +becomes a directed acyclic graph once device links are added.
> 
> <insert sarcasm>
> 
> Oh look a DAG without any sort. How did that happen?
> 
> </end sarcasm>
> > +
> > +Ordering of these devices during suspend/resume is determined by the
> > +dpm_list.
> 
> And where did order from that come from? My notes provided answers to
> these questions.

Well, see above, the order is achieved by traversing the device tree
provided by the platform (either ACPI or DT) top-down.  That is already
mentioned in the "Implementation" section.  So I'm not sure what piece
of information is missing?

Thanks,

Lukas

Re: [PATCH 0/2] Device links documentation

[Jonathan Corbet]

[Trimming the CC list a bit; claws-mail chokes on the full thing for some
reason]

On Sun, 4 Dec 2016 13:10:04 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> @Jonathan Corbet:  Is core-api the right place to put this? An
> alternative would be Documentation/driver-api, but unlike core-api
> it contains less prose text but mostly just bare API docs gleaned
> from kernel-doc.

The amount of prose isn't really the determining factor here...  To me,
this is very much driver-oriented documentation, so I would rather see it
going into the driver-api sub-book.

Thanks for putting it together,

jon

Re: [PATCH 2/2] driver core: Silence device links sphinx warning

[Mauro Carvalho Chehab]

Em Sun, 4 Dec 2016 13:10:04 +0100
Lukas Wunner <lukas@wunner.de> escreveu:

> Silence this warning emitted by sphinx:
> include/linux/device.h:938: warning: No description found for parameter 'links'
> 
> While at it, fix typos in comments of device links code.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Silvio Fricke <silvio.fricke@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/base/core.c    | 4 ++--
>  include/linux/device.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d0c9df5..8c25e68 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -172,7 +172,7 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
>   *
>   * The supplier device is required to be registered when this function is called
>   * and NULL will be returned if that is not the case.  The consumer device need
> - * not be registerd, however.
> + * not be registered, however.
>   */
>  struct device_link *device_link_add(struct device *consumer,
>  				    struct device *supplier, u32 flags)
> @@ -225,7 +225,7 @@ struct device_link *device_link_add(struct device *consumer,
>  	INIT_LIST_HEAD(&link->c_node);
>  	link->flags = flags;
>  
> -	/* Deterine the initial link state. */
> +	/* Determine the initial link state. */
>  	if (flags & DL_FLAG_STATELESS) {
>  		link->status = DL_STATE_NONE;
>  	} else {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 3896af4..87edfdf 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -813,6 +813,7 @@ struct dev_links_info {
>   * 		on.  This shrinks the "Board Support Packages" (BSPs) and
>   * 		minimizes board-specific #ifdefs in drivers.
>   * @driver_data: Private pointer for driver specific info.
> + * @links:	Links to suppliers and consumers of this device.
>   * @power:	For device power management.
>   * 		See Documentation/power/admin-guide/devices.rst for details.
>   * @pm_domain:	Provide callbacks that are executed during system suspend,

Hmm... I'm not seeing "links" at driver-core-next:
	https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/include/linux/device.h?h=driver-core-next

On what tree did you base this patch?

-- 
Thanks,
Mauro

Re: [PATCH 2/2] driver core: Silence device links sphinx warning

[Lukas Wunner]

On Mon, Dec 05, 2016 at 10:20:53AM -0200, Mauro Carvalho Chehab wrote:
> Em Sun, 4 Dec 2016 13:10:04 +0100 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> escreveu:
> > Silence this warning emitted by sphinx:
> > include/linux/device.h:938: warning: No description found for parameter 'links'
> > 
> > While at it, fix typos in comments of device links code.
> > 
> > Cc: Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> > Cc: Silvio Fricke <silvio.fricke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  drivers/base/core.c    | 4 ++--
> >  include/linux/device.h | 1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d0c9df5..8c25e68 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -172,7 +172,7 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
> >   *
> >   * The supplier device is required to be registered when this function is called
> >   * and NULL will be returned if that is not the case.  The consumer device need
> > - * not be registerd, however.
> > + * not be registered, however.
> >   */
> >  struct device_link *device_link_add(struct device *consumer,
> >  				    struct device *supplier, u32 flags)
> > @@ -225,7 +225,7 @@ struct device_link *device_link_add(struct device *consumer,
> >  	INIT_LIST_HEAD(&link->c_node);
> >  	link->flags = flags;
> >  
> > -	/* Deterine the initial link state. */
> > +	/* Determine the initial link state. */
> >  	if (flags & DL_FLAG_STATELESS) {
> >  		link->status = DL_STATE_NONE;
> >  	} else {
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 3896af4..87edfdf 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -813,6 +813,7 @@ struct dev_links_info {
> >   * 		on.  This shrinks the "Board Support Packages" (BSPs) and
> >   * 		minimizes board-specific #ifdefs in drivers.
> >   * @driver_data: Private pointer for driver specific info.
> > + * @links:	Links to suppliers and consumers of this device.
> >   * @power:	For device power management.
> >   * 		See Documentation/power/admin-guide/devices.rst for details.
> >   * @pm_domain:	Provide callbacks that are executed during system suspend,
> 
> Hmm... I'm not seeing "links" at driver-core-next:
> 	https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/include/linux/device.h?h=driver-core-next
> 
> On what tree did you base this patch?

You're looking at the right tree, just maybe not the right line. :-)
It's in line 887:

	https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/include/linux/device.h?h=driver-core-next#n887

Thanks,

Lukas

Re: [PATCH 2/2] driver core: Silence device links sphinx warning

[Mauro Carvalho Chehab]

Em Mon, 5 Dec 2016 13:44:49 +0100
Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> escreveu:

> On Mon, Dec 05, 2016 at 10:20:53AM -0200, Mauro Carvalho Chehab wrote:
> > Em Sun, 4 Dec 2016 13:10:04 +0100 Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> escreveu:  
> > > Silence this warning emitted by sphinx:
> > > include/linux/device.h:938: warning: No description found for parameter 'links'
> > > 
> > > While at it, fix typos in comments of device links code.
> > > 
> > > Cc: Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > > Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> > > Cc: Silvio Fricke <silvio.fricke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > > ---
> > >  drivers/base/core.c    | 4 ++--
> > >  include/linux/device.h | 1 +
> > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index d0c9df5..8c25e68 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -172,7 +172,7 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
> > >   *
> > >   * The supplier device is required to be registered when this function is called
> > >   * and NULL will be returned if that is not the case.  The consumer device need
> > > - * not be registerd, however.
> > > + * not be registered, however.
> > >   */
> > >  struct device_link *device_link_add(struct device *consumer,
> > >  				    struct device *supplier, u32 flags)
> > > @@ -225,7 +225,7 @@ struct device_link *device_link_add(struct device *consumer,
> > >  	INIT_LIST_HEAD(&link->c_node);
> > >  	link->flags = flags;
> > >  
> > > -	/* Deterine the initial link state. */
> > > +	/* Determine the initial link state. */
> > >  	if (flags & DL_FLAG_STATELESS) {
> > >  		link->status = DL_STATE_NONE;
> > >  	} else {
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 3896af4..87edfdf 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -813,6 +813,7 @@ struct dev_links_info {
> > >   * 		on.  This shrinks the "Board Support Packages" (BSPs) and
> > >   * 		minimizes board-specific #ifdefs in drivers.
> > >   * @driver_data: Private pointer for driver specific info.
> > > + * @links:	Links to suppliers and consumers of this device.
> > >   * @power:	For device power management.
> > >   * 		See Documentation/power/admin-guide/devices.rst for details.
> > >   * @pm_domain:	Provide callbacks that are executed during system suspend,  
> > 
> > Hmm... I'm not seeing "links" at driver-core-next:
> > 	https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/include/linux/device.h?h=driver-core-next
> > 
> > On what tree did you base this patch?  
> 
> You're looking at the right tree, just maybe not the right line. :-)

Ah, OK! yeah, I was tricked by this:
	@@ -813,6 +813,7 @@ struct dev_links_info {

Need more caffeine ;)

> It's in line 887:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/include/linux/device.h?h=driver-core-next#n887

Patch looks good on my eyes.

Reviewed-by: Mauro Carvalho Chehab <mchehab-JsYNTwtnfakRB7SZvlqPiA@public.gmane.org>

-- 
Thanks,
Mauro

Re: [PATCH 2/2] driver core: Silence device links sphinx warning

[Greg Kroah-Hartman]

On Sun, Dec 04, 2016 at 01:10:04PM +0100, Lukas Wunner wrote:
> Silence this warning emitted by sphinx:
> include/linux/device.h:938: warning: No description found for parameter 'links'
> 
> While at it, fix typos in comments of device links code.
> 
> Cc: Rafael J. Wysocki <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
> Cc: Silvio Fricke <silvio.fricke-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/base/core.c    | 4 ++--
>  include/linux/device.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)

I'll take this patch now to keep the warnings from showing up in
4.10-rc1.

thanks,

greg k-h

Re: [PATCH 2/2] driver core: Silence device links sphinx warning

[Rafael J. Wysocki]

On Sun, Dec 4, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Silence this warning emitted by sphinx:
> include/linux/device.h:938: warning: No description found for parameter 'links'
>
> While at it, fix typos in comments of device links code.
>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Silvio Fricke <silvio.fricke@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and thanks for fixing these!

[Adds note to self to spellcheck patches next time.]

> ---
>  drivers/base/core.c    | 4 ++--
>  include/linux/device.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d0c9df5..8c25e68 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -172,7 +172,7 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
>   *
>   * The supplier device is required to be registered when this function is called
>   * and NULL will be returned if that is not the case.  The consumer device need
> - * not be registerd, however.
> + * not be registered, however.
>   */
>  struct device_link *device_link_add(struct device *consumer,
>                                     struct device *supplier, u32 flags)
> @@ -225,7 +225,7 @@ struct device_link *device_link_add(struct device *consumer,
>         INIT_LIST_HEAD(&link->c_node);
>         link->flags = flags;
>
> -       /* Deterine the initial link state. */
> +       /* Determine the initial link state. */
>         if (flags & DL_FLAG_STATELESS) {
>                 link->status = DL_STATE_NONE;
>         } else {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 3896af4..87edfdf 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -813,6 +813,7 @@ struct dev_links_info {
>   *             on.  This shrinks the "Board Support Packages" (BSPs) and
>   *             minimizes board-specific #ifdefs in drivers.
>   * @driver_data: Private pointer for driver specific info.
> + * @links:     Links to suppliers and consumers of this device.
>   * @power:     For device power management.
>   *             See Documentation/power/admin-guide/devices.rst for details.
>   * @pm_domain: Provide callbacks that are executed during system suspend,
> --
> 2.10.2
>