Re: [PATCH v3 2/8] ACPI: property: Tie data nodes to acpi handles

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

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.

-- 
Kind regards,

Sakari Ailus