| Message ID | 20220525130123.767410-3-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI: Buffer property and reference as string support | expand |
On Wed, May 25, 2022 at 04:01:17PM +0300, Sakari Ailus wrote: > ACPICA allows associating additional information (i.e. pointers with > specific tag) to acpi_handles. The acpi_device's are associated to > acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the > _DSD data nodes. > > This allows direct data node references in properties, implemented later on > in the series. ... > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > +{ > + struct acpi_data_node *dn; > + > + list_for_each_entry(dn, &data->subnodes, sibling) { > + acpi_status status; > + int ret; > + > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > + if (ACPI_FAILURE(status)) { > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > + return 0; > + } > + > + ret = acpi_tie_nondev_subnodes(&dn->data); > + if (ret) > + return ret; > + } > + > + return 0; > +} ... > - if (!adev->data.pointer) { > + if (!adev->data.pointer || > + acpi_tie_nondev_subnodes(&adev->data) < 0) { > + acpi_untie_nondev_subnodes(&adev->data); I don't know this part of the code, but this looks unusual. Shouldn't _tie() take care of proper error path itself? Also, it's a bit strange to call _untie() when _tie() wasn't called. > acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n"); > ACPI_FREE(buf.pointer); > }
On Wed, May 25, 2022 at 3:01 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > ACPICA allows associating additional information (i.e. pointers with > specific tag) to acpi_handles. The acpi_device's are associated to > acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the > _DSD data nodes. > > This allows direct data node references in properties, implemented later on > in the series. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/acpi/property.c | 42 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index bc9a645f8bb77..f8c83ee6c8d59 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -340,6 +340,43 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid, > return props; > } > > +static void acpi_nondev_subnode_tag(acpi_handle handle, void *context) > +{ > +} > + > +static void acpi_untie_nondev_subnodes(struct acpi_device_data *data) > +{ > + struct acpi_data_node *dn; > + > + list_for_each_entry(dn, &data->subnodes, sibling) { > + acpi_detach_data(dn->handle, acpi_nondev_subnode_tag); > + > + acpi_untie_nondev_subnodes(&dn->data); > + } > +} > + > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > +{ > + struct acpi_data_node *dn; > + > + list_for_each_entry(dn, &data->subnodes, sibling) { > + acpi_status status; > + int ret; > + > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > + if (ACPI_FAILURE(status)) { > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > + return 0; > + } > + > + ret = acpi_tie_nondev_subnodes(&dn->data); > + if (ret) > + return ret; Is it actually possible that this returns anything different from 0? > + } > + > + return 0; > +} > + > static bool acpi_extract_properties(const union acpi_object *desc, > struct acpi_device_data *data) > { > @@ -419,7 +456,9 @@ void acpi_init_properties(struct acpi_device *adev) > &adev->data, acpi_fwnode_handle(adev))) > adev->data.pointer = buf.pointer; > > - if (!adev->data.pointer) { > + if (!adev->data.pointer || > + acpi_tie_nondev_subnodes(&adev->data) < 0) { > + acpi_untie_nondev_subnodes(&adev->data); > acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n"); > ACPI_FREE(buf.pointer); > } > @@ -462,6 +501,7 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list) > > void acpi_free_properties(struct acpi_device *adev) > { > + acpi_untie_nondev_subnodes(&adev->data); > acpi_destroy_nondev_subnodes(&adev->data.subnodes); > ACPI_FREE((void *)adev->data.pointer); > adev->data.of_compatible = NULL; > -- > 2.30.2 >
Hi Rafael, On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote: > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > > +{ > > + struct acpi_data_node *dn; > > + > > + list_for_each_entry(dn, &data->subnodes, sibling) { > > + acpi_status status; > > + int ret; > > + > > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > > + return 0; > > + } > > + > > + ret = acpi_tie_nondev_subnodes(&dn->data); > > + if (ret) > > + return ret; > > Is it actually possible that this returns anything different from 0? acpi_attach_data() involves allocating memory and resolving a reference. Both can fail.
Hi Andy, On Wed, May 25, 2022 at 08:20:04PM +0300, Andy Shevchenko wrote: > On Wed, May 25, 2022 at 04:01:17PM +0300, Sakari Ailus wrote: > > ACPICA allows associating additional information (i.e. pointers with > > specific tag) to acpi_handles. The acpi_device's are associated to > > acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the > > _DSD data nodes. > > > > This allows direct data node references in properties, implemented later on > > in the series. > > ... > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > > +{ > > + struct acpi_data_node *dn; > > + > > + list_for_each_entry(dn, &data->subnodes, sibling) { > > + acpi_status status; > > + int ret; > > + > > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > > + return 0; > > + } > > + > > + ret = acpi_tie_nondev_subnodes(&dn->data); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > ... > > > - if (!adev->data.pointer) { > > + if (!adev->data.pointer || > > > + acpi_tie_nondev_subnodes(&adev->data) < 0) { > > + acpi_untie_nondev_subnodes(&adev->data); > > I don't know this part of the code, but this looks unusual. Shouldn't _tie() > take care of proper error path itself? It could, but I'd need another function for recursive use. You're basically asking to move these two lines into a new function called from here only. > > Also, it's a bit strange to call _untie() when _tie() wasn't called. How does this happen? If you mean not keeping track which nodes have been tied and which have not, it could be done. But there's no harm from detaching data that has not been attached, it's a nop.
On Fri, May 27, 2022 at 11:02 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote: > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > > > +{ > > > + struct acpi_data_node *dn; > > > + > > > + list_for_each_entry(dn, &data->subnodes, sibling) { > > > + acpi_status status; > > > + int ret; > > > + > > > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > > > + if (ACPI_FAILURE(status)) { > > > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > > > + return 0; > > > + } > > > + > > > + ret = acpi_tie_nondev_subnodes(&dn->data); > > > + if (ret) > > > + return ret; > > > > Is it actually possible that this returns anything different from 0? > > acpi_attach_data() involves allocating memory and resolving a reference. > Both can fail. Yes, they can, but the value returned by acpi_attach_data() is effectively ignored above (except for printing the error message, which BTW could be "info" and provide more information). I don't see how acpi_tie_nondev_subnodes() can produce a nonzero return value.
Hi Rafael, On Fri, May 27, 2022 at 07:04:39PM +0200, Rafael J. Wysocki wrote: > On Fri, May 27, 2022 at 11:02 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote: > > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > > > > +{ > > > > + struct acpi_data_node *dn; > > > > + > > > > + list_for_each_entry(dn, &data->subnodes, sibling) { > > > > + acpi_status status; > > > > + int ret; > > > > + > > > > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > > > > + if (ACPI_FAILURE(status)) { > > > > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > > > > + return 0; > > > > + } > > > > + > > > > + ret = acpi_tie_nondev_subnodes(&dn->data); > > > > + if (ret) > > > > + return ret; > > > > > > Is it actually possible that this returns anything different from 0? > > > > acpi_attach_data() involves allocating memory and resolving a reference. > > Both can fail. > > Yes, they can, but the value returned by acpi_attach_data() is > effectively ignored above (except for printing the error message, > which BTW could be "info" and provide more information). Oops. Good point. I intended this to return an error here. I don't have strong opinion on which way to go though. How about changing that to -ENOMEM? I think this is basically a decision on whether any subnodes could be referenced if ore or more of them could not. I don't expect this to happen in practice. > > I don't see how acpi_tie_nondev_subnodes() can produce a nonzero return value.
On Fri, May 27, 2022 at 10:59 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Fri, May 27, 2022 at 07:04:39PM +0200, Rafael J. Wysocki wrote: > > On Fri, May 27, 2022 at 11:02 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > On Thu, May 26, 2022 at 09:19:17PM +0200, Rafael J. Wysocki wrote: > > > > > +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) > > > > > +{ > > > > > + struct acpi_data_node *dn; > > > > > + > > > > > + list_for_each_entry(dn, &data->subnodes, sibling) { > > > > > + acpi_status status; > > > > > + int ret; > > > > > + > > > > > + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); > > > > > + if (ACPI_FAILURE(status)) { > > > > > + acpi_handle_err(dn->handle, "Can't tag data node\n"); > > > > > + return 0; > > > > > + } > > > > > + > > > > > + ret = acpi_tie_nondev_subnodes(&dn->data); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Is it actually possible that this returns anything different from 0? > > > > > > acpi_attach_data() involves allocating memory and resolving a reference. > > > Both can fail. > > > > Yes, they can, but the value returned by acpi_attach_data() is > > effectively ignored above (except for printing the error message, > > which BTW could be "info" and provide more information). > > Oops. Good point. > > I intended this to return an error here. I don't have strong opinion on > which way to go though. How about changing that to -ENOMEM? It might as well return bool and let the caller worry about the error handling. > I think this is basically a decision on whether any subnodes could be > referenced if ore or more of them could not. I don't expect this to happen > in practice. So is having a partial description of something useful? I guess it may or may not be, depending on the use case. If there's any use case in which it may be useful, I would ignore the attach errors and address missing stuff elsewhere.
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index bc9a645f8bb77..f8c83ee6c8d59 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -340,6 +340,43 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid, return props; } +static void acpi_nondev_subnode_tag(acpi_handle handle, void *context) +{ +} + +static void acpi_untie_nondev_subnodes(struct acpi_device_data *data) +{ + struct acpi_data_node *dn; + + list_for_each_entry(dn, &data->subnodes, sibling) { + acpi_detach_data(dn->handle, acpi_nondev_subnode_tag); + + acpi_untie_nondev_subnodes(&dn->data); + } +} + +static int acpi_tie_nondev_subnodes(struct acpi_device_data *data) +{ + struct acpi_data_node *dn; + + list_for_each_entry(dn, &data->subnodes, sibling) { + acpi_status status; + int ret; + + status = acpi_attach_data(dn->handle, acpi_nondev_subnode_tag, dn); + if (ACPI_FAILURE(status)) { + acpi_handle_err(dn->handle, "Can't tag data node\n"); + return 0; + } + + ret = acpi_tie_nondev_subnodes(&dn->data); + if (ret) + return ret; + } + + return 0; +} + static bool acpi_extract_properties(const union acpi_object *desc, struct acpi_device_data *data) { @@ -419,7 +456,9 @@ void acpi_init_properties(struct acpi_device *adev) &adev->data, acpi_fwnode_handle(adev))) adev->data.pointer = buf.pointer; - if (!adev->data.pointer) { + if (!adev->data.pointer || + acpi_tie_nondev_subnodes(&adev->data) < 0) { + acpi_untie_nondev_subnodes(&adev->data); acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n"); ACPI_FREE(buf.pointer); } @@ -462,6 +501,7 @@ static void acpi_destroy_nondev_subnodes(struct list_head *list) void acpi_free_properties(struct acpi_device *adev) { + acpi_untie_nondev_subnodes(&adev->data); acpi_destroy_nondev_subnodes(&adev->data.subnodes); ACPI_FREE((void *)adev->data.pointer); adev->data.of_compatible = NULL;
ACPICA allows associating additional information (i.e. pointers with specific tag) to acpi_handles. The acpi_device's are associated to acpi_handle's in acpi_tie_acpi_dev() in scan.c, do the same here for the _DSD data nodes. This allows direct data node references in properties, implemented later on in the series. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/property.c | 42 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)