linux-kernel - Re: [PATCH v1 2/4] ACPI: property: Add code comments explaining what is going on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMf5ZNW9t_6tfsjy@kekkonen.localdomain>
Date: Mon, 15 Sep 2025 14:32:52 +0300
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Linux ACPI <linux-acpi@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v1 2/4] ACPI: property: Add code comments explaining what
 is going on

Hi Rafael,

On Fri, Sep 12, 2025 at 09:40:58PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> In some places in the ACPI device properties handling code, it is
> unclear why the code is what it is.  Some assumptions are not documented
> and some pieces of code are based on experience that is not mentioned
> anywhere.
> 
> Add code comments explaining these things.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  drivers/acpi/property.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
> 
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -108,7 +108,18 @@ static bool acpi_nondev_subnode_extract(
>  	if (handle)
>  		acpi_get_parent(handle, &scope);
>  
> +	/*
> +	 * Extract properties from the _DSD-equivalent package pointed to by
> +	 * desc and use scope (if not NULL) for the completion of relative
> +	 * pathname segments.
> +	 *
> +	 * The extracted properties will be held in the new data node dn.
> +	 */
>  	result = acpi_extract_properties(scope, desc, &dn->data);
> +	/*
> +	 * Look for subnodes in the _DSD-equivalent package pointed to by desc
> +	 * and create child nodes of dn if there are any.
> +	 */
>  	if (acpi_enumerate_nondev_subnodes(scope, desc, &dn->data, &dn->fwnode))
>  		result = true;
>  
> @@ -153,6 +164,12 @@ static bool acpi_nondev_subnode_ok(acpi_
>  	acpi_handle handle;
>  	acpi_status status;
>  
> +	/*
> +	 * If the scope is unknown, the _DSD-equivalent package being parsed
> +	 * was embedded in an outer _DSD-equivalent package as a result of
> +	 * direct evaluation of an object pointed to by a reference.  In that
> +	 * case, using a pathname as the target object pointer is invalid.
> +	 */
>  	if (!scope)
>  		return false;
>  
> @@ -172,6 +189,10 @@ static bool acpi_add_nondev_subnodes(acp
>  	bool ret = false;
>  	int i;
>  
> +	/*
> +	 * Every element in the links package is expected to represent a link
> +	 * to a non-device node in a tree containing device-specific data.
> +	 */
>  	for (i = 0; i < links->package.count; i++) {
>  		union acpi_object *link, *desc;
>  		acpi_handle handle;
> @@ -182,22 +203,48 @@ static bool acpi_add_nondev_subnodes(acp
>  		if (link->package.count != 2)
>  			continue;
>  
> -		/* The first one must be a string. */
> +		/* The first one (the key) must be a string. */
>  		if (link->package.elements[0].type != ACPI_TYPE_STRING)
>  			continue;
>  
> -		/* The second one may be a string, a reference or a package. */
> +		/*
> +		 * The second one (the target) may be a string, a reference or
> +		 * a package.
> +		 */
>  		switch (link->package.elements[1].type) {
>  		case ACPI_TYPE_STRING:
> +			/*
> +			 * The string is expected to be a full pathname or a
> +			 * pathname segment relative to the given scope.  That
> +			 * pathname is expected to point to an object returning
> +			 * a package that contains _DSD-equivalent information.
> +			 */
>  			result = acpi_nondev_subnode_ok(scope, link, list,
>  							 parent);
>  			break;
>  		case ACPI_TYPE_LOCAL_REFERENCE:

I think you get ACPI_TYPE_LOCAL_REFERENCE only when you evaluate a
reference to a device object.

> +			/*
> +			 * The reference is expected to point to an object
> +			 * returning a package that contains _DSD-equivalent
> +			 * information.
> +			 */
>  			handle = link->package.elements[1].reference.handle;
>  			result = acpi_nondev_subnode_data_ok(handle, link, list,
>  							     parent);
>  			break;
>  		case ACPI_TYPE_PACKAGE:

And similarly, the result of an evaluation here is a package when a
reference points to a name object (i.e. a data node).

> +			/*
> +			 * This happens when the target package is embedded
> +			 * within the links package as a result of direct
> +			 * evaluation of an object pointed to by a reference.
> +			 *
> +			 * The target package is expected to contain _DSD-
> +			 * equivalent information, but the scope in which it
> +			 * is located in the original AML is unknown.  Thus
> +			 * it cannot contain pathname segments represented as
> +			 * strings because there is no way to build full
> +			 * pathnames out of them.
> +			 */
>  			desc = &link->package.elements[1];
>  			result = acpi_nondev_subnode_extract(desc, NULL, link,
>  							     list, parent);
> 

-- 
Kind regards,

Sakari Ailus