Re: [PATCH v1 04/17] ACPI: processor: idle: Rearrange acpi_processor_evaluate_lpi()

["lihuisong (C)" <lihuisong@huawei.com>]

On 7/9/2026 8:33 PM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>
> Rearrange acpi_processor_evaluate_lpi() to make it somewhat easier to
> follow and diagnose (if need be).  In particular:
>
>   * Rename some local variables and reorder their definitions.
>
>   * Change the type of local variables used for storing firmware-provided
>     values to unsigned int (they cannot be negative).
>
>   * Eliminate local variable "loop" that is redundant.
>
>   * Avoid explicit pointer arithmetic.
>
>   * Print the correct number of _LPI state packages in the final debug
>     message.
>
> No intentional functional impact beyond debug output.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/processor_idle.c | 64 +++++++++++++++++++----------------
>   1 file changed, 34 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8682f779249..cd506e9e5a84 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -872,12 +872,12 @@ static int obj_get_integer(union acpi_object *obj, u32 *value)
>   static int acpi_processor_evaluate_lpi(acpi_handle handle,
>   				       struct acpi_lpi_states_array *info)
>   {
> -	acpi_status status;
> -	int ret = 0;
> -	int pkg_count, state_idx = 1, loop;
>   	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object *lpi_data;
> +	union acpi_object *lpi_data, *lpi_pkg;
> +	unsigned int lpi_pkg_count, state_idx;
>   	struct acpi_lpi_state *lpi_state;
> +	acpi_status status;
> +	int ret = 0;
>   
>   	status = acpi_evaluate_object(handle, "_LPI", NULL, &buffer);
>   	if (ACPI_FAILURE(status)) {
> @@ -895,41 +895,46 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>   		goto end;
>   	}
>   
> -	pkg_count = lpi_data->package.elements[2].integer.value;
> +	lpi_pkg_count = lpi_data->package.elements[2].integer.value;
>   
>   	/* Validate number of power states. */
> -	if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
> +	if (!lpi_pkg_count || lpi_pkg_count != lpi_data->package.count - 3) {
>   		acpi_handle_debug(handle, "Invalid _LPI state count\n");
>   		ret = -ENODATA;
>   		goto end;
>   	}
>   
> -	lpi_state = kzalloc_objs(*lpi_state, pkg_count);
> +	lpi_state = kzalloc_objs(*lpi_state, lpi_pkg_count);
>   	if (!lpi_state) {
>   		ret = -ENOMEM;
>   		goto end;
>   	}
>   
> -	info->size = pkg_count;
> +	info->size = lpi_pkg_count;
>   	info->entries = lpi_state;
>   
> -	/* LPI States start at index 3 */
> -	for (loop = 3; state_idx <= pkg_count; loop++, state_idx++, lpi_state++) {
> -		union acpi_object *element, *pkg_elem, *obj;
> +	/* _LPI State packages start at index 3. */
> +	lpi_pkg = &lpi_data->package.elements[3];
>   
> -		element = &lpi_data->package.elements[loop];
> -		if (element->type != ACPI_TYPE_PACKAGE || element->package.count < 7)
> +	for (state_idx = 1; state_idx <= lpi_pkg_count;
> +	     state_idx++, lpi_state++, lpi_pkg++) {
> +		union acpi_object *lpi_pkg_elem, *obj;
> +
> +		lpi_state->index = state_idx;
> +
> +		if (lpi_pkg->type != ACPI_TYPE_PACKAGE || lpi_pkg->package.count < 7)
>   			continue;

The first place to verify the count of LPI package.

>   
> -		pkg_elem = element->package.elements;
> +		lpi_pkg_elem = lpi_pkg->package.elements;
>   
> -		obj = pkg_elem + 6;
> +		/* Get the entry method first and skip the state if that fails. */
> +		obj = &lpi_pkg_elem[6];
>   		if (obj->type == ACPI_TYPE_BUFFER) {
>   			struct acpi_power_register *reg;
>   
>   			if (obj->buffer.length < sizeof(*reg)) {
>   				acpi_handle_debug(handle,
> -					"Invalid register data for _LPI state %d\n",
> +					"Invalid register data for _LPI state %u\n",
>   					state_idx);
>   				continue;
>   			}
> @@ -937,7 +942,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>   			reg = (struct acpi_power_register *)obj->buffer.pointer;
>   			if (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
>   				acpi_handle_debug(handle,
> -					"Unsupported entry method for _LPI state %d\n",
> +					"Unsupported entry method for _LPI state %u\n",
>   					state_idx);
>   				continue;
>   			}
> @@ -949,36 +954,35 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>   			lpi_state->address = obj->integer.value;
>   		} else {
>   			acpi_handle_debug(handle,
> -					  "Invalid entry method for _LPI state %d\n",
> +					  "Invalid entry method for _LPI state %u\n",
>   					  state_idx);
>   			continue;
>   		}
>   
> -		lpi_state->index = state_idx;
> -		if (obj_get_integer(pkg_elem + 0, &lpi_state->min_residency)) {
> +		if (obj_get_integer(&lpi_pkg_elem[0], &lpi_state->min_residency)) {
>   			acpi_handle_debug(handle,
> -					  "Assuming 10 us min. residency for _LPI state %d\n",
> +					  "Assuming 10 us min. residency for _LPI state %u\n",
>   					  state_idx);
>   			lpi_state->min_residency = 10;
>   		}
>   
> -		if (obj_get_integer(pkg_elem + 1, &lpi_state->wake_latency)) {
> +		if (obj_get_integer(&lpi_pkg_elem[1], &lpi_state->wake_latency)) {
>   			acpi_handle_debug(handle,
> -					  "Assuming 10 us wake latency for _LPI state %d\n",
> +					  "Assuming 10 us wake latency for _LPI state %u\n",
>   					  state_idx);
>   			lpi_state->wake_latency = 10;
>   		}
>   
> -		if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
> +		if (obj_get_integer(&lpi_pkg_elem[2], &lpi_state->flags))
>   			lpi_state->flags = 0;
>   
> -		if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
> +		if (obj_get_integer(&lpi_pkg_elem[3], &lpi_state->arch_flags))
>   			lpi_state->arch_flags = 0;
>   
> -		if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
> +		if (obj_get_integer(&lpi_pkg_elem[4], &lpi_state->res_cnt_freq))
>   			lpi_state->res_cnt_freq = 1;
>   
> -		if (obj_get_integer(pkg_elem + 5, &lpi_state->enable_parent_state))
> +		if (obj_get_integer(&lpi_pkg_elem[5], &lpi_state->enable_parent_state))
>   			lpi_state->enable_parent_state = 0;
>   
>   		/* Skip elements [7-8] i.e. Residency/Usage counters. */
> @@ -987,16 +991,16 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>   		 * Avoid out-of-bounds access if the size of the package is less
>   		 * than expected.
>   		 */
> -		if (element->package.count < 10)
> +		if (lpi_pkg->package.count < 10)
>   			continue;
>   
second place.

I think the package count should be compared with 10 and placed at the 
begin of the loop.
Even the optional element in LPI package also should be define NULL 
register or NULL string based on Spec 6.6.
And they still occupy at least one position. For example, "State Name" 
must be "" if platform doesn't support it.
So this packet count is at least 10.
> -		obj = pkg_elem + 9;
> +		obj = &lpi_pkg_elem[9];
>   		if (obj->type == ACPI_TYPE_STRING)
>   			strscpy(lpi_state->desc, obj->string.pointer,
>   				ACPI_CX_DESC_LEN);
>   	}
>   
> -	acpi_handle_debug(handle, "Found %d power states\n", state_idx);
> +	acpi_handle_debug(handle, "Found %u power states\n", lpi_pkg_count);
>   end:
>   	kfree(buffer.pointer);
>   	return ret;