Re: [PATCH v2 3/4] dt-bindings: Add post-init-supplier property

Saravana Kannan
2024-02-21T04:14:13Z

On Thu, Feb 15, 2024 at 4:15 AM Conor Dooley <conor@kernel.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@kernel.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@google.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@google.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@1000: 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@1000: 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@2000: 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@2000: 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-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
> > ...
>
> 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