Message-ID: <20240214-stable-anytime-b51b898d87af@spud>
Date: Wed, 14 Feb 2024 18:48:59 +0000
From: Conor Dooley <conor@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
Cc: Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Ard Biesheuvel <ardb@...nel.org>,
	Frank Rowand <frowand.list@...il.com>, Len Brown <lenb@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	kernel-team@...roid.com, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-efi@...r.kernel.org,
	linux-acpi@...r.kernel.org
Subject: Re: [PATCH v2 3/4] dt-bindings: Add post-init-supplier property

On Mon, Feb 12, 2024 at 01:31:44PM -0800, Saravana Kannan wrote:
> The post-init-supplier property can be used to break a dependency cycle by
> marking some supplier(s) as a post device initialization supplier(s). This
> allows an OS to do a better job at ordering initialization and
> suspend/resume of the devices in a dependency cycle.
> 
> Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> ---
>  .../bindings/post-init-supplier.yaml          | 101 ++++++++++++++++++
>  MAINTAINERS                                   |  13 +--
>  2 files changed, 108 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/post-init-supplier.yaml
> 
> diff --git a/Documentation/devicetree/bindings/post-init-supplier.yaml b/Documentation/devicetree/bindings/post-init-supplier.yaml
> new file mode 100644
> index 000000000000..aab75b667259
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/post-init-supplier.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2020, Google LLC. All rights reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/post-init-supplier.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Post device initialization supplier
> +
> +maintainers:
> +  - Saravana Kannan <saravanak@...gle.com>
> +
> +description: |
> +  This property is used to indicate that the device(s) pointed to by the
> +  property are not needed for the initialization of the device that lists this
> +  property.

> This property is meaningful only when pointing to direct suppliers
> +  of a device that are pointed to by other properties in the device.

I don't think this sentence makes sense, or at least it is not easy to
parse. It implies that it can "point to" other properties too - but
that's not the case. It is only valid to "point to" these suppliers.
I'd drop this entirely.

> +
> +  A device can list its suppliers in devicetree using one or more of the
> +  standard devicetree bindings. By default, it would be safe to assume the
> +  supplier device can be initialized before the consumer device is initialized.

"it would be safe to assume" seems odd wording to me - I feel like the
default is stronger than "safe to assume". I'd just drop the "would be
safe to assume and replace with "is assumed".

> +
> +  However, that assumption cannot be made when there are cyclic dependencies
> +  between devices. Since each device is a supplier (directly or indirectly) of
> +  the others in the cycle, there is no guaranteed safe order for initializing
> +  the devices in a cycle. We can try to initialize them in an arbitrary order
> +  and eventually successfully initialize all of them, but that doesn't always
> +  work well.
> +
> +  For example, say,
> +  * The device tree has the following cyclic dependency X -> Y -> Z -> X (where
> +    -> denotes "depends on").
> +  * But X is not needed to fully initialize Z (X might be needed only when a
> +    specific functionality is requested post initialization).
> +
> +  If all the other -> are mandatory initialization dependencies, then trying to
> +  initialize the devices in a loop (or arbitrarily) will always eventually end
> +  up with the devices being initialized in the order Z, Y and X.
> +
> +  However, if Y is an optional supplier for X (where X provides limited
> +  functionality when Y is not initialized and providing its services), then
> +  trying to initialize the devices in a loop (or arbitrarily) could end up with
> +  the devices being initialized in the following order:
> +
> +  * Z, Y and X - All devices provide full functionality
> +  * Z, X and Y - X provides partial functionality
> +  * X, Z and Y - X provides partial functionality
> +
> +  However, we always want to initialize the devices in the order Z, Y and X
> +  since that provides the full functionality without interruptions.
> +
> +  One alternate option that might be suggested is to have the driver for X
> +  notice that Y became available at a later point and adjust the functionality
> +  it provides. However, other userspace applications could have started using X
> +  with the limited functionality before Y was available and it might not be
> +  possible to transparently transition X or the users of X to full
> +  functionality while X is in use.
> +
> +  Similarly, when it comes to suspend (resume) ordering, it's unclear which
> +  device in a dependency cycle needs to be suspended/resumed first and trying
> +  arbitrary orders can result in system crashes or instability.
> +
> +  Explicitly calling out which link in a cycle needs to be broken when
> +  determining the order, simplifies things a lot, improves efficiency, makes
> +  the behavior more deterministic and maximizes the functionality that can be
> +  provided without interruption.
> +
> +  This property is used to provide this additional information between devices
> +  in a cycle by telling which supplier(s) is not needed for initializing the
> +  device that lists this property.
> +
> +  In the example above, Z would list X as a post-init-supplier and the
> +  initialization dependency would become X -> Y -> Z -/-> X. So the best order
> +  to initialize them become clear: Z, Y and then X.

Otherwise, I think this is a great description, describing the use case
well :)

> +
> +select: true
> +properties:
> +  post-init-supplier:
> +    # One or more suppliers can be marked as post initialization supplier
> +    description:
> +      List of phandles to suppliers that are not needed for initializing or
> +      resuming this device.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +      items:
> +        maxItems: 1

Rob's bot rightfully complains here about invalid syntax. What you
actually want to enforce here is any number of device phandles, but
these phandles all contain only the label and no indices etc, right?

> +
> +examples:
> +  - |
> +    gcc: clock-controller@...0 {
> +        compatible = "vendor,soc4-gcc", "vendor,soc1-gcc";
> +        reg = <0x1000 0x80>;
> +        clocks = <&dispcc 0x1>

This clearly was never tested, Rob's bot warnings aside. You're missing
a ; at EOL here and with the other clock below. 

Cheers,
Conor.

> +        #clock-cells = <1>;
> +        post-init-supplier = <&dispcc>;
> +    };
> +    dispcc: clock-controller@...0 {
> +        compatible = "vendor,soc4-dispcc", "vendor,soc1-dispcc";
> +        reg = <0x2000 0x80>;
> +        clocks = <&gcc 0xdd>
> +        #clock-cells = <1>;
> +    };

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists