Message-ID: <CAGETcx8EBta8dUSELUJ6_ibZABnnhSYX0VEGa8s-CbHFYuskkQ@mail.gmail.com>
Date: Tue, 20 Feb 2024 20:13: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

On Thu, Feb 15, 2024 at 4:15 AM Conor Dooley <conor@...nel.org> wrote:
>
> On Wed, Feb 14, 2024 at 03:32:31PM -0800, Saravana Kannan wrote:
> > 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.
>
> Because, to me, it reads as if you can put extra stuff in here that will
> be ignored if not "pointed to" by another property. The word
> "meaningful" is what implies that you can.
>
> > 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.
>
> If you wanna keep it, I would just go for what you said in this
> response - that this property does not make devices into suppliers and
> is only to mark existing suppliers as post-init. I think that rules out
> putting other devices in there.

Sounds good.

> > > - 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?
>
> Aye, both items: and maxItems: need to lose a level of indent. That
> said, its not actually restricting anything. I fixed it up locally and
> you can put as many elements as you like into each phandle and it does
> not care. Maybe Rob can tell what is going wrong there..

I made that fix and now I'm getting this:
$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/post-init-suppliers.yaml
  DTEX    Documentation/devicetree/bindings/post-init-suppliers.example.dts
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
'oneOf' conditional failed, one must be fixed:
        'unevaluatedProperties' is a required property
        'additionalProperties' is a required property
        hint: Either unevaluatedProperties or additionalProperties
must be present
        from schema $id: http://devicetree.org/meta-schemas/core.yaml#
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
/mnt/android/linus-tree/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml:
ignoring, error in schema: properties
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-suppliers.yaml:
ignoring, error in schema:
/mnt/android/linus-tree/Documentation/devicetree/bindings/soc/tegra/nvidia,tegra20-pmc.yaml:
ignoring, error in schema: allOf: 0: then: properties: pinmux
/mnt/android/linus-tree/Documentation/devicetree/bindings/net/lantiq,pef2256.yaml:
ignoring, error in schema: properties: lantiq,data-rate-bps
/mnt/android/linus-tree/Documentation/devicetree/bindings/post-init-supplier.yaml:
ignoring, error in schema:
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml:
ignoring, error in schema: properties: honeywell,pmax-pascal
/mnt/android/linus-tree/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:
ignoring, error in schema: properties: honeywell,pmax-pascal
  DTC_CHK Documentation/devicetree/bindings/post-init-suppliers.example.dtb
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@...0: failed to match any schema with
compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@...0: failed to match any schema with
compatible: ['vendor,soc4-gcc', 'vendor,soc1-gcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@...0: failed to match any schema with
compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']
Documentation/devicetree/bindings/post-init-suppliers.example.dtb:0:0:
/example-0/clock-controller@...0: failed to match any schema with
compatible: ['vendor,soc4-dispcc', 'vendor,soc1-dispcc']

But I guess the "oneOf" error is because the yaml is being treated as
a description of a DT node and not a schema?

Rob,

Can you let me know how to move ahead with this? I'll do the fixes
that Conor suggested in v3.

-Saravana

>
> >
> > 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
> > ...
>
> Yup, that is about right, although you snipped out the actual complaint.
>
> >
> > 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-suppliersyaml
> > 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
> > ...
>
> That is part of the actual complaint:
>
> make dt_binding_check W=1 -j 30 DT_SCHEMA_FILES=post-init-supplier.yaml
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/post-init-supplier.example.dts
> Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: mapping values are not allowed here
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/post-init-supplier.example.dts] Error 1
> make[2]: *** Deleting file 'Documentation/devicetree/bindings/post-init-supplier.example.dts'
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: [error] syntax error: mapping values are not allowed here (syntax)
>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> ./Documentation/devicetree/bindings/post-init-supplier.yaml:84:12: mapping values are not allowed here
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> /stuff/linux-dt/Documentation/devicetree/bindings/post-init-supplier.yaml: ignoring, error parsing file
> make[1]: *** [/stuff/linux-dt/Makefile:1432: dt_binding_check] Error 2
> make: *** [Makefile:240: __sub-make] Error 2

Powered by blists - more mailing lists