| Message ID | 20220525130123.767410-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | ACPI: Buffer property and reference as string support | expand |
On Wed, May 25, 2022 at 04:01:21PM +0300, Sakari Ailus wrote: > Unify functions reading ACPI property integer values into a single macro > using C99 _Generic(). > > Also use size_t for the counter instead of int. Thanks for an update! ... > +#define acpi_copy_property_array_uint(items, val, nval) \ > + ({ \ You can define local copies of (read-only) parameters and avoid adding parentheses each time you access them. > + size_t i; \ > + int ret = 0; \ > + \ > + for (i = 0; i < (nval); i++) { \ > + if ((items)[i].type != ACPI_TYPE_INTEGER) { \ > + ret = -EPROTO; \ > + break; \ > + } \ > + if ((items)[i].integer.value > _Generic((val), \ > + u8: U8_MAX, \ > + u16: U16_MAX, \ > + u32: U32_MAX, \ > + u64: U64_MAX, \ > + default: 0U)) { \ I think nobody will die if you add one more TAB to each line and make \ be consistent column wise. > + ret = -EOVERFLOW; \ > + break; \ > + } \ > + \ > + (val)[i] = (items)[i].integer.value; \ > + } \ > + ret; \ > + })
On Wed, May 25, 2022 at 04:01:16PM +0300, Sakari Ailus wrote: > The value acpi_add_nondev_subnodes() returns is bool so change the return > type of the function to match that. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: 445b0eb058f5 ("ACPI / property: Add support for data-only subnodes") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/acpi/property.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index d3173811614ec..bc9a645f8bb77 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -155,10 +155,10 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope, > return acpi_nondev_subnode_data_ok(handle, link, list, parent); > } > > -static int acpi_add_nondev_subnodes(acpi_handle scope, > - const union acpi_object *links, > - struct list_head *list, > - struct fwnode_handle *parent) > +static bool acpi_add_nondev_subnodes(acpi_handle scope, > + const union acpi_object *links, > + struct list_head *list, > + struct fwnode_handle *parent) > { > bool ret = false; > int i; > -- > 2.30.2 >
On Wed, May 25, 2022 at 04:01:18PM +0300, Sakari Ailus wrote: > The type of union acpi_object field type is acpi_object_type. Use that > instead of int. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/acpi/property.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index f8c83ee6c8d59..b36cb7e36e420 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -793,7 +793,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode, > * nor integer, return an error, we can't parse it. > */ > for (i = 0; element + i < end && i < num_args; i++) { > - int type = element[i].type; > + acpi_object_type type = element[i].type; > > if (type == ACPI_TYPE_LOCAL_REFERENCE) > break; > -- > 2.30.2 >
On Wed, May 25, 2022 at 04:01:23PM +0300, Sakari Ailus wrote: > Instead of adding a new property type, read buffer properties as integers. > Even though the internal representation in ACPI is different, the data > type is the same (byte) than on 8-bit integers. ... > + switch (proptype) { > + case DEV_PROP_STRING: > + break; > + case DEV_PROP_U8 ... DEV_PROP_U64: > + if (obj->type == ACPI_TYPE_BUFFER) { > + if (nval <= obj->buffer.length) > + break; > + return -EOVERFLOW; Why not traditional pattern and be consistent with default case? if (nval > obj->buffer.length) return -EOVERFLOW; break; > + } > + fallthrough; > + default: > + if (nval > obj->package.count) > + return -EOVERFLOW; I would add break statement here. > + } > if (nval == 0) > return -EINVAL;
On Wed, May 25, 2022 at 04:01:22PM +0300, Sakari Ailus wrote: > Add support for newly added buffer property UUID, as defined in the DSD > guide. ... > + if (check_mul_overflow((size_t)properties->package.count, Why do you need casting? Any issues on 32-bit compilation? Looking at the below code snippets, it seems you also can have a local copy with needed type and use it everywhere (as outer_package_count or so). But first question first... > + sizeof(*package) + sizeof(void *), > + &alloc_size) || > + check_add_overflow(sizeof(*props) + sizeof(*package), alloc_size, > + &alloc_size)) { > + acpi_handle_warn(handle, > + "can't allocate memory for %u buffer props", > + properties->package.count); > + return; > + } ... > + if (ACPI_FAILURE(status)) { > + acpi_handle_warn(handle, > + "can't evaluate \"%s\" as buffer\n", > + obj->string.pointer); I'm wondering if better to use %*pE here to show the full data of the buffer. > + continue; > + }
Hi Andy, On Wed, May 25, 2022 at 08:36:54PM +0300, Andy Shevchenko wrote: > On Wed, May 25, 2022 at 04:01:23PM +0300, Sakari Ailus wrote: > > Instead of adding a new property type, read buffer properties as integers. > > Even though the internal representation in ACPI is different, the data > > type is the same (byte) than on 8-bit integers. > > ... > > > + switch (proptype) { > > + case DEV_PROP_STRING: > > + break; > > + case DEV_PROP_U8 ... DEV_PROP_U64: > > + if (obj->type == ACPI_TYPE_BUFFER) { > > > + if (nval <= obj->buffer.length) > > + break; > > + return -EOVERFLOW; > > Why not traditional pattern and be consistent with default case? > > if (nval > obj->buffer.length) > return -EOVERFLOW; > break; Agreed. > > > + } > > + fallthrough; > > + default: > > + if (nval > obj->package.count) > > + return -EOVERFLOW; > > I would add break statement here. Sounds good.
Hi Andy, On Wed, May 25, 2022 at 08:44:11PM +0300, Andy Shevchenko wrote: > On Wed, May 25, 2022 at 04:01:22PM +0300, Sakari Ailus wrote: > > Add support for newly added buffer property UUID, as defined in the DSD > > guide. > > ... > > > + if (check_mul_overflow((size_t)properties->package.count, > > Why do you need casting? Any issues on 32-bit compilation? I think it can be removed. I'll see. > > Looking at the below code snippets, it seems you also can have a local > copy with needed type and use it everywhere (as outer_package_count or so). > But first question first... u32 should be fine. > > > + sizeof(*package) + sizeof(void *), > > + &alloc_size) || > > + check_add_overflow(sizeof(*props) + sizeof(*package), alloc_size, > > + &alloc_size)) { > > + acpi_handle_warn(handle, > > + "can't allocate memory for %u buffer props", > > + properties->package.count); > > + return; > > + } > > ... > > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_warn(handle, > > + "can't evaluate \"%s\" as buffer\n", > > + obj->string.pointer); > > I'm wondering if better to use %*pE here to show the full data of the buffer. The string is supposed to be printable. If it's not, something is seriously wrong. There's no harm though to prepare for this possibility. It'll make backslashes printing twice but that is hardly an issue in practice. > > > + continue; > > + } >
Hi Andy, On Wed, May 25, 2022 at 08:14:36PM +0300, Andy Shevchenko wrote: > On Wed, May 25, 2022 at 04:01:21PM +0300, Sakari Ailus wrote: > > Unify functions reading ACPI property integer values into a single macro > > using C99 _Generic(). > > > > Also use size_t for the counter instead of int. > > Thanks for an update! > > ... > > > +#define acpi_copy_property_array_uint(items, val, nval) \ > > + ({ \ > > You can define local copies of (read-only) parameters and avoid adding > parentheses each time you access them. Sounds good. > > > + size_t i; \ > > + int ret = 0; \ > > + \ > > + for (i = 0; i < (nval); i++) { \ > > + if ((items)[i].type != ACPI_TYPE_INTEGER) { \ > > + ret = -EPROTO; \ > > + break; \ > > + } \ > > + if ((items)[i].integer.value > _Generic((val), \ > > + u8: U8_MAX, \ > > + u16: U16_MAX, \ > > + u32: U32_MAX, \ > > + u64: U64_MAX, \ > > + default: 0U)) { \ > > I think nobody will die if you add one more TAB to each line and make \ be > consistent column wise. I think it's unlikely, too. Still the above is consistent with the rest of recently merged patches adding similar constructs.
On Wed, May 25, 2022 at 3:01 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hello everyone, > > This set adds support for _DSD buffer properties (specified by DSD Guide > <URL:https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.md>) as well as > support for references as strings. Reference property type was previously > supported for device objects only, whereas string references enable > referencing also _DSD sub-node objects --- also included in the set. > > The ACPICA patch has been submitted to upstream but not merged yet. > > This set currently prepares for data node string reference support and > does not add it anymore. > > since v2: > > - Use C99 _Generic() in patch unifying reading integer arrays. > > since v1: > > - Drop the ACPICA, data node child list initialisation and data node > string reference patches. > > Sakari Ailus (8): > ACPI: property: Return type of acpi_add_nondev_subnodes() should be > bool > ACPI: property: Tie data nodes to acpi handles > ACPI: property: Use acpi_object_type consistently in property ref > parsing > ACPI: property: Move property ref argument parsing into a new function > ACPI: property: Switch node property referencing from ifs to a switch > ACPI: property: Unify integer value reading functions > ACPI: property: Add support for parsing buffer property UUID > ACPI: property: Read buffer properties as integers > > drivers/acpi/property.c | 462 +++++++++++++++++++++++++++------------- > include/acpi/acpi_bus.h | 3 +- > include/linux/acpi.h | 2 +- > 3 files changed, 315 insertions(+), 152 deletions(-) > > -- FYI, I'm expecting a v4 of this series to be sent.
Hello everyone, This set adds support for _DSD buffer properties (specified by DSD Guide <URL:https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.md>) as well as support for references as strings. Reference property type was previously supported for device objects only, whereas string references enable referencing also _DSD sub-node objects --- also included in the set. The ACPICA patch has been submitted to upstream but not merged yet. This set currently prepares for data node string reference support and does not add it anymore. since v2: - Use C99 _Generic() in patch unifying reading integer arrays. since v1: - Drop the ACPICA, data node child list initialisation and data node string reference patches. Sakari Ailus (8): ACPI: property: Return type of acpi_add_nondev_subnodes() should be bool ACPI: property: Tie data nodes to acpi handles ACPI: property: Use acpi_object_type consistently in property ref parsing ACPI: property: Move property ref argument parsing into a new function ACPI: property: Switch node property referencing from ifs to a switch ACPI: property: Unify integer value reading functions ACPI: property: Add support for parsing buffer property UUID ACPI: property: Read buffer properties as integers drivers/acpi/property.c | 462 +++++++++++++++++++++++++++------------- include/acpi/acpi_bus.h | 3 +- include/linux/acpi.h | 2 +- 3 files changed, 315 insertions(+), 152 deletions(-)