Re: [PATCH 4/5] ACPI / property: Allow holes in reference properties

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Wed, Sep 28, 2016 at 11:36:10PM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 23, 2016 at 4:57 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > DT allows holes or empty phandles for references. This is used for example
> > in SPI subsystem where some chip selects are native and others are regular
> > GPIOs. In ACPI _DSD we currently do not support this but instead the
> > preceding reference "consumes" all following integer arguments.
> >
> > For example we would like to support something like the below ASL fragment
> > for SPI:
> >
> >   Package () {
> >       "cs-gpios",
> >       Package () {
> >           ^GPIO, 19, 0, 0, // GPIO CS0
> >           0,               // Native CS
> >           ^GPIO, 20, 0, 0, // GPIO CS1
> >       }
> >   }
> >
> > The zero in the middle means "no entry" or NULL reference. To support this
> > we add a new function acpi_node_get_property_reference_fixed_args() that
> > takes number of expected arguments as parameter. Function returns -ENOENT
> > if the corresponding index resolves to the "no entry" reference.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/acpi/property.c | 116 +++++++++++++++++++++++++++++++++++-------------
> >  include/linux/acpi.h    |   3 ++
> >  2 files changed, 89 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index f2fd3fee588a..b255c4442a05 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -472,6 +472,7 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> >   * @data: ACPI device data object containing the property
> >   * @propname: Name of the property
> >   * @index: Index of the reference to return
> > + * @num_args: Maximum number of arguments after each reference
> >   * @args: Location to store the returned reference with optional arguments
> >   *
> >   * Find property with @name, verifify that it is a package containing at least
> > @@ -485,8 +486,8 @@ static int acpi_data_get_property_array(struct acpi_device_data *data,
> >   * Return: %0 on success, negative error code on failure.
> >   */
> >  static int acpi_data_get_property_reference(struct acpi_device_data *data,
> > -                                           const char *propname, size_t index,
> > -                                           struct acpi_reference_args *args)
> > +       const char *propname, size_t index, size_t num_args,
> > +       struct acpi_reference_args *args)
> >  {
> >         const union acpi_object *element, *end;
> >         const union acpi_object *obj;
> > @@ -532,41 +533,52 @@ static int acpi_data_get_property_reference(struct acpi_device_data *data,
> >         while (element < end) {
> >                 u32 nargs, i;
> >
> > -               if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
> > -                       return -EPROTO;
> > -
> > -               ret = acpi_bus_get_device(element->reference.handle, &device);
> > -               if (ret)
> > -                       return -ENODEV;
> > -
> > -               element++;
> > -               nargs = 0;
> > -
> > -               /* assume following integer elements are all args */
> > -               for (i = 0; element + i < end; i++) {
> > -                       int type = element[i].type;
> > +               if (element->type == ACPI_TYPE_LOCAL_REFERENCE) {
> > +                       ret = acpi_bus_get_device(element->reference.handle,
> > +                                                 &device);
> > +                       if (ret)
> > +                               return -ENODEV;
> > +
> > +                       nargs = 0;
> > +                       element++;
> > +
> > +                       /* assume following integer elements are all args */
> > +                       for (i = 0; element + i < end && i < num_args; i++) {
> > +                               int type = element[i].type;
> > +
> > +                               if (type == ACPI_TYPE_INTEGER)
> > +                                       nargs++;
> > +                               else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> > +                                       break;
> > +                               else
> > +                                       return -EPROTO;
> > +                       }
> >
> > -                       if (type == ACPI_TYPE_INTEGER)
> > -                               nargs++;
> > -                       else if (type == ACPI_TYPE_LOCAL_REFERENCE)
> > -                               break;
> > -                       else
> > +                       if (nargs > MAX_ACPI_REFERENCE_ARGS)
> >                                 return -EPROTO;
> > -               }
> >
> > -               if (idx++ == index) {
> > -                       args->adev = device;
> > -                       args->nargs = nargs;
> > -                       for (i = 0; i < nargs; i++)
> > -                               args->args[i] = element[i].integer.value;
> > +                       if (idx == index) {
> > +                               args->adev = device;
> > +                               args->nargs = nargs;
> > +                               for (i = 0; i < nargs; i++)
> > +                                       args->args[i] = element[i].integer.value;
> >
> > -                       return 0;
> > +                               return 0;
> > +                       }
> > +
> > +                       element += nargs;
> > +               } else if (element->type == ACPI_TYPE_INTEGER) {
> > +                       if (idx == index)
> > +                               return -ENOENT;
> > +                       element++;
> > +               } else {
> > +                       return -EPROTO;
> >                 }
> >
> > -               element += nargs;
> > +               idx++;
> >         }
> >
> > -       return -EPROTO;
> > +       return -ENODATA;
> >  }
> >
> >  /**
> > @@ -582,10 +594,54 @@ int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
> >  {
> >         struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
> >
> > -       return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
> > +       if (!data)
> > +               return -EINVAL;
> > +
> > +       return acpi_data_get_property_reference(data, name, index,
> > +               MAX_ACPI_REFERENCE_ARGS, args);
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
> 
> gpiolib-acpi is the only user of this AFAICS, so why don't we change
> the definition of acpi_node_get_property_reference() to take one extra
> argument and update the caller to pass MAX_ACPI_REFERENCE_ARGS?

Unfortunately there are these (which I think should not call that
function directly anyway):

drivers/net/ethernet/apm/xgene/xgene_enet_hw.c: status = acpi_node_get_property_reference(fw_node, "phy-handle", 0,
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c:      rc = acpi_node_get_property_reference(
drivers/net/ethernet/hisilicon/hns/hns_enet.c:          ret = acpi_node_get_property_reference(dev->fwnode,

So we would need to update those as well. Not a big deal, though.