Message-ID: <CAGETcx-tBjfaLQqmGW=ap2N5FLK_gvTzxskA6sVsr_SUEpvomA@mail.gmail.com>
Date: Wed, 14 Feb 2024 15:32:31 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Conor Dooley <conor@...nel.org>
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

Hi Conon,

On Wed, Feb 14, 2024 at 10:49 AM Conor Dooley <conor@...nel.org> wrote:
>
> 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

I don't see how this sentence implies this. But open to suggestions on
how to reword it. I don't want to drop this line entirely though
because I'm trying to make it clear that this doesn't make a device
(that's not previously a supplier) into a supplier. It only down
grades an existing supplier to a post device initialization supplier.

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

Sounds good.

>
> > +
> > +  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 :)

Thanks! I always spend more time writing documentation and commit text
than the time I spend writing code.

>
> > +
> > +select: true
> > +properties:
> > +  post-init-supplier:

[Merging your other email here]

> Also, this should likely be pluralised, to match "clocks" "resets"
> "interrupts" etc.

Good point. Done.

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

I added these two lines based on Rob's feedback. Is the indentation
that's wrong?

Yeah, I'm trying to run the dts checker, but I haven't be able to get
it to work on my end. See my email to Rob on the v1 series about this.

$ make DT_CHECKER_FLAGS=-m dt_binding_check

The best I could get out of it is a bunch of error reports on other
files and then:
..
<snip>/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error parsing file
..

I also tried to use DT_SCHEMA_FILES so I can only test this one file,
but that wasn't working either:

$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
or
$ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=<path to
the .patch file>

Results in this error early on in the output:
..
usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA]
[--list-files] [-f {parsable,standard,colored,github,auto}] [-s]
[--no-warnings] [-v] [FILE_OR_DIR ...]
yamllint: error: one of the arguments FILE_OR_DIR - is required
..
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error parsing file
..

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

Correct.

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

Yup. I'm unable to get the test to run.

Thanks,
Saravana

Powered by blists - more mailing lists