Re: [PATCH v8 02/10] ACPI: property: Parse data node string references in properties

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Rafael,

On Tue, May 23, 2023 at 01:21:12PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Mon, May 22, 2023 at 10:35 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > On Mon, May 22, 2023 at 06:38:37PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, May 22, 2023 at 6:28 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > > On Mon, May 22, 2023 at 05:29:48PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tue, May 16, 2023 at 1:24 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > On Fri, May 12, 2023 at 06:04:26PM +0200, Rafael J. Wysocki wrote:
> > > > > > > On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > Add support for parsing property references using strings, besides
> > > > > > > > reference objects that were previously supported. This allows also
> > > > > > > > referencing data nodes which was not possible with reference objects.
> > > > > > > >
> > > > > > > > Also add pr_fmt() macro to prefix printouts.
> > > > > > > >
> > > > > > > > While at it, update copyright.
> > > > > > >
> > > > > > > Although I said that it looked good to me, some minor improvements can
> > > > > > > still be made.
> > > > > > >
> > > > > > > First off, the above changelog is a bit terse.
> > > > > > >
> > > > > > > I think that it would help to provide an example of device properties
> > > > > > > that would not be parsed properly before the change and can be parsed
> > > > > > > now.
> > > > > > >
> > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/acpi/property.c | 110 ++++++++++++++++++++++++++++++++++------
> > > > > > > >  1 file changed, 94 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > > > > > > index b8d9eb9a433e..08831ffba26c 100644
> > > > > > > > --- a/drivers/acpi/property.c
> > > > > > > > +++ b/drivers/acpi/property.c
> > > > > > > > @@ -2,14 +2,17 @@
> > > > > > > >  /*
> > > > > > > >   * ACPI device specific properties support.
> > > > > > > >   *
> > > > > > > > - * Copyright (C) 2014, Intel Corporation
> > > > > > > > + * Copyright (C) 2014-2023, Intel Corporation
> > > > > > > >   * All rights reserved.
> > > > > > > >   *
> > > > > > > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > > > > - *          Darren Hart <dvhart@linux.intel.com>
> > > > > > > > - *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > + *         Darren Hart <dvhart@linux.intel.com>
> > > > > > > > + *         Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > I'm not sure if the whitespace change here is really useful.
> > > > > >
> > > > > > I did that to address a comment from Andy --- the earlier lines used spaces
> > > > > > for indentation.
> > > > > >
> > > > > > >
> > > > > > > > + *         Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > >   */
> > > > > > > >
> > > > > > > > +#define pr_fmt(fmt) "ACPI: " fmt
> > > > > > > > +
> > > > > > > >  #include <linux/acpi.h>
> > > > > > > >  #include <linux/device.h>
> > > > > > > >  #include <linux/export.h>
> > > > > > > > @@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
> > > > > > > >  static int acpi_get_ref_args(struct fwnode_reference_args *args,
> > > > > > > >                              struct fwnode_handle *ref_fwnode,
> > > > > > > >                              const union acpi_object **element,
> > > > > > > > -                            const union acpi_object *end, size_t num_args)
> > > > > > > > +                            const union acpi_object *end, size_t num_args,
> > > > > > > > +                            bool subnode_string)
> > > > > > >
> > > > > > > The meaning of the new argument isn't really clear.  it would be good
> > > > > > > to somehow help a casual reader of the code to find this out more
> > > > > > > easily.
> > > > > >
> > > > > > I can add comments to v9.
> > > > >
> > > > > If you can send me an example of ASL that will be parsed correctly
> > > > > after this change, but not before, it will help a bit.
> > > >
> > > > E.g. this bit from DisCo for Imaging 1.0 (Annex B.1):
> > > >
> > > >         Package () {
> > > >             "mipi-img-flash-leds",
> > > >             Package () {
> > > >                 "\\_SB.PCI0.I2C2.LEDD.LED0",
> > > >                 "\\_SB.PCI0.I2C2.LEDD.LED1"
> > > >             },
> > > >         },
> > > >
> > > > It's a property with a string reference to an ACPI non-device node,
> > > > although you can refer to device nodes as well.
> > >
> > > This example is missing the definition of LED0 or LED1 from which it
> > > would be clear that they are data nodes (or at least one of them is a
> > > data node).
> >
> > Ok, perhaps this one could work as a complete example, with a single
> > reference:
> >
> >         Package ()
> >         {
> >             "mipi-img-flash-leds",  "\\_SB.PCI0.I2C2.LEDD.LED0",
> >         }
> >
> >         Device (LEDD)
> >         {
> >             Name (_DSD, Package ()  // _DSD: Device-Specific Data
> >             {
> >                 ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), /* Hierarchical Data Extension */,
> >                 Package ()
> >                 {
> >                     Package ()
> >                     {
> >                         "mipi-img-flash-led-0",
> >                         "LED0",
> >                     }
> >                 },
> >             })
> >             Name (LED0, Package ()  // _DSD: Device-Specific Data
> >             {
> >                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
> >                 Package ()
> >                 {
> >                     Package ()
> >                     {
> >                         "mipi-img-max-current",
> >                         1000000,
> >                     }
> >                 }
> >             })
> >         }
> >
> 
> This works, thanks!
> 
> > >
> > > Also I'm kind of wondering about the "reference with arguments" part
> > > which seems to work differently depending on whether the reference is
> > > represented by a string or by a reference object.
> >
> > Yes. With (device) reference objects, it is possible currently to refer to
> > subnodes with the _DSD data extension child names of those nodes. This is
> > not done with string references as 1) any node can already be referenced so
> > there's no need to and 2) as node references are strings already, it's not
> > possible to distinguish node string references from _DSD data node names.
> > E.g.
> >
> >         "\\_SB.I2C0.LED0", "LED1"
> >
> >                            ^ ACPI object name or _DSD data node name?
> >
> 
> Has this behavior been documented anywhere?  Or is there any
> expectation to see anything like this shipping in production platform
> firmware?

Good question. Support for this was added by commit 4eb0c3bf5ee52f . AFAIR
it was intended to use this in DisCo for Imaging but after review (in a
rather liberal sense of the term) it was decided to use string-only
references, as in this patch.

I'm not aware of anyone needing this. They've been there for about five
years but I'd guess someone would complain if it stops working for them.

> 
> If any of the above isn't the case, I would be inclined to simply
> remove this special case and make both the "object reference" and
> "string" cases work in the same way and if someone needs to refer to a
> data node, they will just need to use a string (in which case it will
> be the only option).

Works for me.

-- 
Kind regards,

Sakari Ailus