[PATCH v1 0/9] ACPI: processor: idle: enhance and cleancode for cpuidle state

[PATCH v1 0/9] ACPI: processor: idle: enhance and cleancode for cpuidle state

[Huisong Li]

This series is aimed to enhance the verification to the validity of
_LPI object and LPI state. And do some cleancodes.

Huisong Li (9):
  ACPI: processor: idle: raise up log level when evaluate LPI failed
  ACPI: processor: idle: Return failure if entry method is not buffer or
    integer type
  ACPI: processor: idle: Return failure when get lpi_state->arch_flags
    failed
  ACPI: processor: idle: Move the initialization of state->flags to
    acpi_processor_setup_cstates
  ACPI: processor: idle: Add the verification of processor FFH LPI state
  ACPI: processor: idle: Do not change power states if get power info
    failed
  ACPI: processor: idle: Remove died codes about the verification of
    cstate count
  ACPI: processor: idle: Redefine setup idle functions to void
  ACPI: processor: idle: Redefine acpi_processor_setup_cpuidle_dev to
    void

 drivers/acpi/processor_idle.c | 102 ++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 49 deletions(-)

-- 
2.33.0

[PATCH v1 1/9] ACPI: processor: idle: raise up log level when evaluate LPI failed

[Huisong Li]

According to ACPI spec, LPI package must be ACPI_TYPE_PACKAGE and
the count of package must be greater than 4. And the count contained
in package needs to be equal to the value of count field in LPI package.
All are illegal and return failure. It is better for these verification
to use error level log instead of debug so as to get detailed logs directly
when initialization fails.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 22b051b94a86..5acf12a0441f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -908,7 +908,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
 	/* There must be at least 4 elements = 3 elements + 1 package */
 	if (!lpi_data || lpi_data->type != ACPI_TYPE_PACKAGE ||
 	    lpi_data->package.count < 4) {
-		pr_debug("not enough elements in _LPI\n");
+		pr_err("not enough elements in _LPI\n");
 		ret = -ENODATA;
 		goto end;
 	}
@@ -917,7 +917,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
 
 	/* Validate number of power states. */
 	if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
-		pr_debug("count given by _LPI is not valid\n");
+		pr_err("count given by _LPI is not valid\n");
 		ret = -ENODATA;
 		goto end;
 	}
-- 
2.33.0

[PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[Huisong Li]

According to ACPI spec, entry method in LPI sub-package must be buffer
or integer. However, acpi_processor_evaluate_lpi() regeards it as success
and treat it as an effective LPI state. This is unreasonable and needs to
return failure to prevent other problems from occurring.

Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 5acf12a0441f..681587f2614b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -958,7 +958,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
 			lpi_state->entry_method = ACPI_CSTATE_INTEGER;
 			lpi_state->address = obj->integer.value;
 		} else {
-			continue;
+			pr_err("Entry method in LPI sub-package must be buffer or integer.\n");
+			ret = -EINVAL;
+			goto end;
 		}
 
 		/* elements[7,8] skipped for now i.e. Residency/Usage counter*/
-- 
2.33.0

[PATCH v1 3/9] ACPI: processor: idle: Return failure when get lpi_state->arch_flags failed

[Huisong Li]

The architecture specific context loss flags is important for ARM.
And this flag is used to control the execution of different code
flows in acpi_processor_ffh_lpi_enter().

So it is better to return failure when get lpi_state->arch_flags
failed.

Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 681587f2614b..f36f9514b6c7 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -984,8 +984,11 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
 		if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
 			lpi_state->flags = 0;
 
-		if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
-			lpi_state->arch_flags = 0;
+		if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags)) {
+			pr_err("Get architecture specific context loss flags failed.\n");
+			ret = -EINVAL;
+			goto end;
+		}
 
 		if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
 			lpi_state->res_cnt_freq = 1;
-- 
2.33.0

[PATCH v1 4/9] ACPI: processor: idle: Move the initialization of state->flags to acpi_processor_setup_cstates

[Huisong Li]

The acpi_processor_setup_cpuidle_cx() is called by
acpi_processor_setup_cpuidle_dev() which is used to setup cpuidle device.
However, acpi_processor_setup_cpuidle_cx() also initializes the states
in acpi_idle_driver, which isn't good. And acpi_processor_setup_cstates()
is aimed to initializes cstates in acpi_idle_driver. So the initialization
of state->flags should be here.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f36f9514b6c7..5684925338b3 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -737,13 +737,11 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 {
 	int i, count = ACPI_IDLE_STATE_START;
 	struct acpi_processor_cx *cx;
-	struct cpuidle_state *state;
 
 	if (max_cstate == 0)
 		max_cstate = 1;
 
 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER && i <= max_cstate; i++) {
-		state = &acpi_idle_driver.states[count];
 		cx = &pr->power.states[i];
 
 		if (!cx->valid)
@@ -751,15 +749,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 
 		per_cpu(acpi_cstate[count], dev->cpu) = cx;
 
-		if (lapic_timer_needs_broadcast(pr, cx))
-			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
-
-		if (cx->type == ACPI_STATE_C3) {
-			state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
-			if (pr->flags.bm_check)
-				state->flags |= CPUIDLE_FLAG_RCU_IDLE;
-		}
-
 		count++;
 		if (count == CPUIDLE_STATE_MAX)
 			break;
@@ -818,6 +807,15 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
 		if (cx->type != ACPI_STATE_C1 && !acpi_idle_fallback_to_c1(pr))
 			state->enter_s2idle = acpi_idle_enter_s2idle;
 
+		if (lapic_timer_needs_broadcast(pr, cx))
+			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+
+		if (cx->type == ACPI_STATE_C3) {
+			state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
+			if (pr->flags.bm_check)
+				state->flags |= CPUIDLE_FLAG_RCU_IDLE;
+		}
+
 		count++;
 		if (count == CPUIDLE_STATE_MAX)
 			break;
-- 
2.33.0

[PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Huisong Li]

Both ARM64 and RISCV architecture would validate Entry Method of LPI
state and SBI HSM or PSCI cpu suspend. Driver should return failure
if FFH of LPI state are not ok.

Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 5684925338b3..b0d6b51ee363 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
 
 	dev->cpu = pr->id;
 	if (pr->flags.has_lpi)
-		return acpi_processor_ffh_lpi_probe(pr->id);
+		return 0;
 
 	return acpi_processor_setup_cpuidle_cx(pr, dev);
 }
@@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
 
 	ret = acpi_processor_get_lpi_info(pr);
 	if (ret)
-		ret = acpi_processor_get_cstate_info(pr);
+		return acpi_processor_get_cstate_info(pr);
+
+	if (pr->flags.has_lpi) {
+		ret = acpi_processor_ffh_lpi_probe(pr->id);
+		if (ret)
+			pr_err("Processor FFH LPI state is invalid.\n");
+	}
 
 	return ret;
 }
-- 
2.33.0

[PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[Huisong Li]

Driver will update power states when processor power states have been
changed. To prevent any other abnormal issues, here add the verification
for the result of getting power information, don't change power states
and one error log when get power information failed.

Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index b0d6b51ee363..92b231f5d514 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1315,6 +1315,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
 	int cpu;
 	struct acpi_processor *_pr;
 	struct cpuidle_device *dev;
+	int ret = 0;
 
 	if (disabled_by_idle_boot_param())
 		return 0;
@@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
 		}
 
 		/* Populate Updated C-state information */
-		acpi_processor_get_power_info(pr);
-		acpi_processor_setup_cpuidle_states(pr);
+		ret = acpi_processor_get_power_info(pr);
+		if (ret)
+			pr_err("Get processor-%u power information failed.\n",
+			       pr->id);
+		else
+			acpi_processor_setup_cpuidle_states(pr);
 
 		/* Enable all cpuidle devices */
 		for_each_online_cpu(cpu) {
 			_pr = per_cpu(processors, cpu);
 			if (!_pr || !_pr->flags.power_setup_done)
 				continue;
-			acpi_processor_get_power_info(_pr);
-			if (_pr->flags.power) {
+			ret = acpi_processor_get_power_info(_pr);
+			if (!ret && _pr->flags.power) {
 				dev = per_cpu(acpi_cpuidle_device, cpu);
 				acpi_processor_setup_cpuidle_dev(_pr, dev);
 				cpuidle_enable_device(dev);
@@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
 		cpus_read_unlock();
 	}
 
-	return 0;
+	return ret;
 }
 
 void acpi_processor_register_idle_driver(void)
-- 
2.33.0

[PATCH v1 7/9] ACPI: processor: idle: Remove died codes about the verification of cstate count

[Huisong Li]

The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
be called after successfully obtaining the power information. These setup
functions have their own main role, but also verify the validity of cstate
count.

Actually, the acpi_processor_get_power_info_cst() will return failure
if the cstate count is zero and acpi_processor_get_power_info() will return
failure.

So the verification of cstate count in these functions are died code.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 92b231f5d514..2d672afc7498 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
 	return 0;
 }
 
-static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
-					   struct cpuidle_device *dev)
+static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
+					    struct cpuidle_device *dev)
 {
 	int i, count = ACPI_IDLE_STATE_START;
 	struct acpi_processor_cx *cx;
@@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 		if (count == CPUIDLE_STATE_MAX)
 			break;
 	}
-
-	if (!count)
-		return -EINVAL;
-
-	return 0;
 }
 
-static int acpi_processor_setup_cstates(struct acpi_processor *pr)
+static void acpi_processor_setup_cstates(struct acpi_processor *pr)
 {
 	int i, count;
 	struct acpi_processor_cx *cx;
@@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
 	}
 
 	drv->state_count = count;
-
-	if (!count)
-		return -EINVAL;
-
-	return 0;
 }
 
 static inline void acpi_processor_cstate_first_run_checks(void)
@@ -1246,7 +1236,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 	if (pr->flags.has_lpi)
 		return acpi_processor_setup_lpi_states(pr);
 
-	return acpi_processor_setup_cstates(pr);
+	acpi_processor_setup_cstates(pr);
+	return 0;
 }
 
 /**
@@ -1266,7 +1257,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
 	if (pr->flags.has_lpi)
 		return 0;
 
-	return acpi_processor_setup_cpuidle_cx(pr, dev);
+	acpi_processor_setup_cpuidle_cx(pr, dev);
+	return 0;
 }
 
 static int acpi_processor_get_power_info(struct acpi_processor *pr)
-- 
2.33.0

[PATCH v1 8/9] ACPI: processor: idle: Redefine setup idle functions to void

[Huisong Li]

Notice that the acpi_processor_setup_cpuidle_states() don't need to
return any value because their callers don't check them anyway.
In addition, acpi_processor_setup_lpi_states() wouldn't execute with
failure. So redefine setup idle functions to void.

No intentional functional impact.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 2d672afc7498..9f456a6fa584 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1183,7 +1183,7 @@ static int acpi_idle_lpi_enter(struct cpuidle_device *dev,
 	return -EINVAL;
 }
 
-static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
+static void acpi_processor_setup_lpi_states(struct acpi_processor *pr)
 {
 	int i;
 	struct acpi_lpi_state *lpi;
@@ -1191,7 +1191,7 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
 	struct cpuidle_driver *drv = &acpi_idle_driver;
 
 	if (!pr->flags.has_lpi)
-		return -EOPNOTSUPP;
+		return;
 
 	for (i = 0; i < pr->power.count && i < CPUIDLE_STATE_MAX; i++) {
 		lpi = &pr->power.lpi_states[i];
@@ -1209,8 +1209,6 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
 	}
 
 	drv->state_count = i;
-
-	return 0;
 }
 
 /**
@@ -1219,13 +1217,13 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
  *
  * @pr: the ACPI processor
  */
-static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+static void acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 {
 	int i;
 	struct cpuidle_driver *drv = &acpi_idle_driver;
 
 	if (!pr->flags.power_setup_done || !pr->flags.power)
-		return -EINVAL;
+		return;
 
 	drv->safe_state_index = -1;
 	for (i = ACPI_IDLE_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
@@ -1233,11 +1231,12 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 		drv->states[i].desc[0] = '\0';
 	}
 
-	if (pr->flags.has_lpi)
-		return acpi_processor_setup_lpi_states(pr);
+	if (pr->flags.has_lpi) {
+		acpi_processor_setup_lpi_states(pr);
+		return;
+	}
 
 	acpi_processor_setup_cstates(pr);
-	return 0;
 }
 
 /**
-- 
2.33.0

[PATCH v1 9/9] ACPI: processor: idle: Redefine acpi_processor_setup_cpuidle_dev to void

[Huisong Li]

Notice that the acpi_processor_setup_cpuidle_dev() don't need to
return any value because their callers don't check them anyway.
So redefine the function to void.

No intentional functional impact.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/processor_idle.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9f456a6fa584..1c9f2911ef6c 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1246,18 +1246,17 @@ static void acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
  * @pr: the ACPI processor
  * @dev : the cpuidle device
  */
-static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
-					    struct cpuidle_device *dev)
+static void acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
+					     struct cpuidle_device *dev)
 {
 	if (!pr->flags.power_setup_done || !pr->flags.power || !dev)
-		return -EINVAL;
+		return;
 
 	dev->cpu = pr->id;
 	if (pr->flags.has_lpi)
-		return 0;
+		return;
 
 	acpi_processor_setup_cpuidle_cx(pr, dev);
-	return 0;
 }
 
 static int acpi_processor_get_power_info(struct acpi_processor *pr)
-- 
2.33.0

Re: [PATCH v1 1/9] ACPI: processor: idle: raise up log level when evaluate LPI failed

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> According to ACPI spec, LPI package must be ACPI_TYPE_PACKAGE and
> the count of package must be greater than 4. And the count contained
> in package needs to be equal to the value of count field in LPI package.
> All are illegal and return failure. It is better for these verification
> to use error level log instead of debug so as to get detailed logs directly
> when initialization fails.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 22b051b94a86..5acf12a0441f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -908,7 +908,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>         /* There must be at least 4 elements = 3 elements + 1 package */
>         if (!lpi_data || lpi_data->type != ACPI_TYPE_PACKAGE ||
>             lpi_data->package.count < 4) {
> -               pr_debug("not enough elements in _LPI\n");
> +               pr_err("not enough elements in _LPI\n");
>                 ret = -ENODATA;
>                 goto end;
>         }
> @@ -917,7 +917,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>
>         /* Validate number of power states. */
>         if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
> -               pr_debug("count given by _LPI is not valid\n");
> +               pr_err("count given by _LPI is not valid\n");
>                 ret = -ENODATA;
>                 goto end;
>         }
> --

They are pr_debug() on purpose because they are not useful to anyone
other than the people who work on _LPI implementations in firmware or
debug firmware issues.  They do not indicate kernel functional issues
in particular.

Re: [PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> According to ACPI spec, entry method in LPI sub-package must be buffer
> or integer. However, acpi_processor_evaluate_lpi() regeards it as success
> and treat it as an effective LPI state.

Is that the case?  AFAICS, it just gets to the next state in this case
and what's wrong with that?

> This is unreasonable and needs to
> return failure to prevent other problems from occurring.
>
> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 5acf12a0441f..681587f2614b 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -958,7 +958,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>                         lpi_state->entry_method = ACPI_CSTATE_INTEGER;
>                         lpi_state->address = obj->integer.value;
>                 } else {
> -                       continue;
> +                       pr_err("Entry method in LPI sub-package must be buffer or integer.\n");
> +                       ret = -EINVAL;
> +                       goto end;
>                 }
>
>                 /* elements[7,8] skipped for now i.e. Residency/Usage counter*/
> --

Re: [PATCH v1 3/9] ACPI: processor: idle: Return failure when get lpi_state->arch_flags failed

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The architecture specific context loss flags is important for ARM.
> And this flag is used to control the execution of different code
> flows in acpi_processor_ffh_lpi_enter().
>
> So it is better to return failure when get lpi_state->arch_flags
> failed.

A failure means no idle states at all.

Wouldn't it be better to skip the state with invalid arch flags?

> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 681587f2614b..f36f9514b6c7 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -984,8 +984,11 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>                 if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
>                         lpi_state->flags = 0;
>
> -               if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
> -                       lpi_state->arch_flags = 0;
> +               if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags)) {
> +                       pr_err("Get architecture specific context loss flags failed.\n");
> +                       ret = -EINVAL;
> +                       goto end;
> +               }
>
>                 if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
>                         lpi_state->res_cnt_freq = 1;
> --
> 2.33.0
>

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> Both ARM64 and RISCV architecture would validate Entry Method of LPI
> state and SBI HSM or PSCI cpu suspend. Driver should return failure
> if FFH of LPI state are not ok.

First of all, I cannot parse this changelog, so I don't know the
motivation for the change.

Second, if _LPI is ever used on x86, the
acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
get in the way.

Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?

> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 5684925338b3..b0d6b51ee363 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>
>         dev->cpu = pr->id;
>         if (pr->flags.has_lpi)
> -               return acpi_processor_ffh_lpi_probe(pr->id);
> +               return 0;
>
>         return acpi_processor_setup_cpuidle_cx(pr, dev);
>  }
> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>
>         ret = acpi_processor_get_lpi_info(pr);
>         if (ret)
> -               ret = acpi_processor_get_cstate_info(pr);
> +               return acpi_processor_get_cstate_info(pr);
> +
> +       if (pr->flags.has_lpi) {
> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
> +               if (ret)
> +                       pr_err("Processor FFH LPI state is invalid.\n");
> +       }
>
>         return ret;
>  }
> --
> 2.33.0
>

Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> Driver will update power states when processor power states have been
> changed. To prevent any other abnormal issues, here add the verification
> for the result of getting power information, don't change power states
> and one error log when get power information failed.

But the old states may not be usable any more in that case.

If you want to check the acpi_processor_get_power_info(), it should
disable ACPi idle entirely on failures.

> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index b0d6b51ee363..92b231f5d514 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1315,6 +1315,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>         int cpu;
>         struct acpi_processor *_pr;
>         struct cpuidle_device *dev;
> +       int ret = 0;
>
>         if (disabled_by_idle_boot_param())
>                 return 0;
> @@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>                 }
>
>                 /* Populate Updated C-state information */
> -               acpi_processor_get_power_info(pr);
> -               acpi_processor_setup_cpuidle_states(pr);
> +               ret = acpi_processor_get_power_info(pr);
> +               if (ret)
> +                       pr_err("Get processor-%u power information failed.\n",
> +                              pr->id);
> +               else
> +                       acpi_processor_setup_cpuidle_states(pr);
>
>                 /* Enable all cpuidle devices */
>                 for_each_online_cpu(cpu) {
>                         _pr = per_cpu(processors, cpu);
>                         if (!_pr || !_pr->flags.power_setup_done)
>                                 continue;
> -                       acpi_processor_get_power_info(_pr);
> -                       if (_pr->flags.power) {
> +                       ret = acpi_processor_get_power_info(_pr);
> +                       if (!ret && _pr->flags.power) {
>                                 dev = per_cpu(acpi_cpuidle_device, cpu);
>                                 acpi_processor_setup_cpuidle_dev(_pr, dev);
>                                 cpuidle_enable_device(dev);
> @@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>                 cpus_read_unlock();
>         }
>
> -       return 0;
> +       return ret;
>  }
>
>  void acpi_processor_register_idle_driver(void)
> --
> 2.33.0
>

Re: [PATCH v1 7/9] ACPI: processor: idle: Remove died codes about the verification of cstate count

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
> be called after successfully obtaining the power information. These setup
> functions have their own main role, but also verify the validity of cstate
> count.
>
> Actually, the acpi_processor_get_power_info_cst() will return failure
> if the cstate count is zero and acpi_processor_get_power_info() will return
> failure.
>
> So the verification of cstate count in these functions are died code.

It is useless overhead rather because the conditions checked cannot be true.

> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 92b231f5d514..2d672afc7498 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
>         return 0;
>  }
>
> -static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> -                                          struct cpuidle_device *dev)
> +static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> +                                           struct cpuidle_device *dev)
>  {
>         int i, count = ACPI_IDLE_STATE_START;
>         struct acpi_processor_cx *cx;
> @@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>                 if (count == CPUIDLE_STATE_MAX)
>                         break;
>         }
> -
> -       if (!count)
> -               return -EINVAL;
> -
> -       return 0;
>  }
>
> -static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> +static void acpi_processor_setup_cstates(struct acpi_processor *pr)
>  {
>         int i, count;
>         struct acpi_processor_cx *cx;
> @@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>         }
>
>         drv->state_count = count;
> -
> -       if (!count)
> -               return -EINVAL;
> -
> -       return 0;
>  }
>
>  static inline void acpi_processor_cstate_first_run_checks(void)
> @@ -1246,7 +1236,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>         if (pr->flags.has_lpi)
>                 return acpi_processor_setup_lpi_states(pr);
>
> -       return acpi_processor_setup_cstates(pr);
> +       acpi_processor_setup_cstates(pr);
> +       return 0;
>  }
>
>  /**
> @@ -1266,7 +1257,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>         if (pr->flags.has_lpi)
>                 return 0;
>
> -       return acpi_processor_setup_cpuidle_cx(pr, dev);
> +       acpi_processor_setup_cpuidle_cx(pr, dev);
> +       return 0;
>  }
>
>  static int acpi_processor_get_power_info(struct acpi_processor *pr)
> --
> 2.33.0
>

Re: [PATCH v1 4/9] ACPI: processor: idle: Move the initialization of state->flags to acpi_processor_setup_cstates

[Rafael J. Wysocki]

On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The acpi_processor_setup_cpuidle_cx() is called by
> acpi_processor_setup_cpuidle_dev() which is used to setup cpuidle device.
> However, acpi_processor_setup_cpuidle_cx() also initializes the states
> in acpi_idle_driver, which isn't good. And acpi_processor_setup_cstates()
> is aimed to initializes cstates in acpi_idle_driver. So the initialization
> of state->flags should be here.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/acpi/processor_idle.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index f36f9514b6c7..5684925338b3 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -737,13 +737,11 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>  {
>         int i, count = ACPI_IDLE_STATE_START;
>         struct acpi_processor_cx *cx;
> -       struct cpuidle_state *state;
>
>         if (max_cstate == 0)
>                 max_cstate = 1;
>
>         for (i = 1; i < ACPI_PROCESSOR_MAX_POWER && i <= max_cstate; i++) {
> -               state = &acpi_idle_driver.states[count];
>                 cx = &pr->power.states[i];
>
>                 if (!cx->valid)
> @@ -751,15 +749,6 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>
>                 per_cpu(acpi_cstate[count], dev->cpu) = cx;
>
> -               if (lapic_timer_needs_broadcast(pr, cx))
> -                       state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> -
> -               if (cx->type == ACPI_STATE_C3) {
> -                       state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
> -                       if (pr->flags.bm_check)
> -                               state->flags |= CPUIDLE_FLAG_RCU_IDLE;
> -               }
> -
>                 count++;
>                 if (count == CPUIDLE_STATE_MAX)
>                         break;
> @@ -818,6 +807,15 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>                 if (cx->type != ACPI_STATE_C1 && !acpi_idle_fallback_to_c1(pr))
>                         state->enter_s2idle = acpi_idle_enter_s2idle;
>
> +               if (lapic_timer_needs_broadcast(pr, cx))
> +                       state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +
> +               if (cx->type == ACPI_STATE_C3) {
> +                       state->flags |= CPUIDLE_FLAG_TLB_FLUSHED;
> +                       if (pr->flags.bm_check)
> +                               state->flags |= CPUIDLE_FLAG_RCU_IDLE;
> +               }
> +
>                 count++;
>                 if (count == CPUIDLE_STATE_MAX)
>                         break;
> --

Applied with rewritten subject and changelog as 6.19 material, thanks!

Re: [PATCH v1 1/9] ACPI: processor: idle: raise up log level when evaluate LPI failed

[lihuisong (C)]

在 2025/10/22 3:29, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> According to ACPI spec, LPI package must be ACPI_TYPE_PACKAGE and
>> the count of package must be greater than 4. And the count contained
>> in package needs to be equal to the value of count field in LPI package.
>> All are illegal and return failure. It is better for these verification
>> to use error level log instead of debug so as to get detailed logs directly
>> when initialization fails.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 22b051b94a86..5acf12a0441f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -908,7 +908,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>>          /* There must be at least 4 elements = 3 elements + 1 package */
>>          if (!lpi_data || lpi_data->type != ACPI_TYPE_PACKAGE ||
>>              lpi_data->package.count < 4) {
>> -               pr_debug("not enough elements in _LPI\n");
>> +               pr_err("not enough elements in _LPI\n");
>>                  ret = -ENODATA;
>>                  goto end;
>>          }
>> @@ -917,7 +917,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>>
>>          /* Validate number of power states. */
>>          if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
>> -               pr_debug("count given by _LPI is not valid\n");
>> +               pr_err("count given by _LPI is not valid\n");
>>                  ret = -ENODATA;
>>                  goto end;
>>          }
>> --
> They are pr_debug() on purpose because they are not useful to anyone
> other than the people who work on _LPI implementations in firmware or
> debug firmware issues.  They do not indicate kernel functional issues
> in particular.
ok, get this purpose. Thanks.

Re: [PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[lihuisong (C)]

在 2025/10/22 3:34, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> According to ACPI spec, entry method in LPI sub-package must be buffer
>> or integer. However, acpi_processor_evaluate_lpi() regeards it as success
>> and treat it as an effective LPI state.
> Is that the case?  AFAICS, it just gets to the next state in this case
> and what's wrong with that?
The flatten_lpi_states() would consider the state with illegal entry 
method sub-package as a valid one
if the flag of this state is enabled(ACPI_LPI_STATE_FLAGS_ENABLED is set).
And then cpuidle governor would use it because the caller of 
acpi_processor_ffh_lpi_probe() also don't see the return value.
>
>> This is unreasonable and needs to
>> return failure to prevent other problems from occurring.
>>
>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 5acf12a0441f..681587f2614b 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -958,7 +958,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>>                          lpi_state->entry_method = ACPI_CSTATE_INTEGER;
>>                          lpi_state->address = obj->integer.value;
>>                  } else {
>> -                       continue;
>> +                       pr_err("Entry method in LPI sub-package must be buffer or integer.\n");
>> +                       ret = -EINVAL;
>> +                       goto end;
>>                  }
>>
>>                  /* elements[7,8] skipped for now i.e. Residency/Usage counter*/
>> --

Re: [PATCH v1 3/9] ACPI: processor: idle: Return failure when get lpi_state->arch_flags failed

[lihuisong (C)]

在 2025/10/22 3:36, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The architecture specific context loss flags is important for ARM.
>> And this flag is used to control the execution of different code
>> flows in acpi_processor_ffh_lpi_enter().
>>
>> So it is better to return failure when get lpi_state->arch_flags
>> failed.
> A failure means no idle states at all.
Actually, I didn't know why driver should continue to do cpu idle 
scaling if the idle state doesn't meet the developer's expectations.🙂
> Wouldn't it be better to skip the state with invalid arch flags?
This arch flags is important.  And acpi_processor_ffh_lpi_enter will use it.
There is no other place to verify its validity. so here do it.
This check is just to prevent potential issues in cpuidle scaling later.
>
>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 681587f2614b..f36f9514b6c7 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -984,8 +984,11 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>>                  if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
>>                          lpi_state->flags = 0;
>>
>> -               if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
>> -                       lpi_state->arch_flags = 0;
>> +               if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags)) {
>> +                       pr_err("Get architecture specific context loss flags failed.\n");
>> +                       ret = -EINVAL;
>> +                       goto end;
>> +               }
>>
>>                  if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
>>                          lpi_state->res_cnt_freq = 1;
>> --
>> 2.33.0
>>

Re: [PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[Rafael J. Wysocki]

On Thu, Oct 23, 2025 at 11:25 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/22 3:34, Rafael J. Wysocki 写道:
> > On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> According to ACPI spec, entry method in LPI sub-package must be buffer
> >> or integer. However, acpi_processor_evaluate_lpi() regeards it as success
> >> and treat it as an effective LPI state.
> > Is that the case?  AFAICS, it just gets to the next state in this case
> > and what's wrong with that?
> The flatten_lpi_states() would consider the state with illegal entry
> method sub-package as a valid one
> if the flag of this state is enabled(ACPI_LPI_STATE_FLAGS_ENABLED is set).
> And then cpuidle governor would use it because the caller of
> acpi_processor_ffh_lpi_probe() also don't see the return value.

So the problem appears to be that lpi_state increments in every step
of the loop, but it should only increment if the given state is valid.

Re: [PATCH v1 3/9] ACPI: processor: idle: Return failure when get lpi_state->arch_flags failed

[Rafael J. Wysocki]

On Thu, Oct 23, 2025 at 11:59 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/22 3:36, Rafael J. Wysocki 写道:
> > On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> The architecture specific context loss flags is important for ARM.
> >> And this flag is used to control the execution of different code
> >> flows in acpi_processor_ffh_lpi_enter().
> >>
> >> So it is better to return failure when get lpi_state->arch_flags
> >> failed.
> > A failure means no idle states at all.
> Actually, I didn't know why driver should continue to do cpu idle
> scaling if the idle state doesn't meet the developer's expectations.🙂

There may be a firmware bug in one state description, but it doesn't
mean that the other states are unusable, does it?

> > Wouldn't it be better to skip the state with invalid arch flags?

> This arch flags is important.  And acpi_processor_ffh_lpi_enter will use it.
> There is no other place to verify its validity. so here do it.
> This check is just to prevent potential issues in cpuidle scaling later.

I'm saying to treat this particular state as invalid and skip it
without rejecting all of the other states that may be valid.

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[lihuisong (C)]

在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
>> if FFH of LPI state are not ok.
> First of all, I cannot parse this changelog, so I don't know the
> motivation for the change.
Sorry for your confusion.
> Second, if _LPI is ever used on x86, the
> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
> get in the way.
AFAICS, it's also ok if X86 platform use LPI.
>
> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM 
and RISCV.
But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the 
return value.
In addition, from the name of acpi_processor_setup_cpuidle_dev(), its 
main purpose is to setup cpudile device rather than to verify LPI.
So I move it to a more prominent position and redefine the 
acpi_processor_setup_cpuidle_dev to void in patch 9/9.
>
>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 5684925338b3..b0d6b51ee363 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>
>>          dev->cpu = pr->id;
>>          if (pr->flags.has_lpi)
>> -               return acpi_processor_ffh_lpi_probe(pr->id);
>> +               return 0;
>>
>>          return acpi_processor_setup_cpuidle_cx(pr, dev);
>>   }
>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>>
>>          ret = acpi_processor_get_lpi_info(pr);
>>          if (ret)
>> -               ret = acpi_processor_get_cstate_info(pr);
>> +               return acpi_processor_get_cstate_info(pr);
>> +
>> +       if (pr->flags.has_lpi) {
>> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
>> +               if (ret)
>> +                       pr_err("Processor FFH LPI state is invalid.\n");
>> +       }
>>
>>          return ret;
>>   }
>> --
>> 2.33.0
>>

Re: [PATCH v1 7/9] ACPI: processor: idle: Remove died codes about the verification of cstate count

[lihuisong (C)]

在 2025/10/22 3:51, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
>> be called after successfully obtaining the power information. These setup
>> functions have their own main role, but also verify the validity of cstate
>> count.
>>
>> Actually, the acpi_processor_get_power_info_cst() will return failure
>> if the cstate count is zero and acpi_processor_get_power_info() will return
>> failure.
>>
>> So the verification of cstate count in these functions are died code.
> It is useless overhead rather because the conditions checked cannot be true.
Yes.
This patch actually prepares for patch 8/9 and patch 9/9.
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 22 +++++++---------------
>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 92b231f5d514..2d672afc7498 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
>>          return 0;
>>   }
>>
>> -static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> -                                          struct cpuidle_device *dev)
>> +static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> +                                           struct cpuidle_device *dev)
>>   {
>>          int i, count = ACPI_IDLE_STATE_START;
>>          struct acpi_processor_cx *cx;
>> @@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>>                  if (count == CPUIDLE_STATE_MAX)
>>                          break;
>>          }
>> -
>> -       if (!count)
>> -               return -EINVAL;
>> -
>> -       return 0;
>>   }
>>
>> -static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>> +static void acpi_processor_setup_cstates(struct acpi_processor *pr)
>>   {
>>          int i, count;
>>          struct acpi_processor_cx *cx;
>> @@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>>          }
>>
>>          drv->state_count = count;
>> -
>> -       if (!count)
>> -               return -EINVAL;
>> -
>> -       return 0;
>>   }
>>
>>   static inline void acpi_processor_cstate_first_run_checks(void)
>> @@ -1246,7 +1236,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>>          if (pr->flags.has_lpi)
>>                  return acpi_processor_setup_lpi_states(pr);
>>
>> -       return acpi_processor_setup_cstates(pr);
>> +       acpi_processor_setup_cstates(pr);
>> +       return 0;
>>   }
>>
>>   /**
>> @@ -1266,7 +1257,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>          if (pr->flags.has_lpi)
>>                  return 0;
>>
>> -       return acpi_processor_setup_cpuidle_cx(pr, dev);
>> +       acpi_processor_setup_cpuidle_cx(pr, dev);
>> +       return 0;
>>   }
>>
>>   static int acpi_processor_get_power_info(struct acpi_processor *pr)
>> --
>> 2.33.0
>>

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Rafael J. Wysocki]

On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> > On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> Both ARM64 and RISCV architecture would validate Entry Method of LPI
> >> state and SBI HSM or PSCI cpu suspend. Driver should return failure
> >> if FFH of LPI state are not ok.
> > First of all, I cannot parse this changelog, so I don't know the
> > motivation for the change.
> Sorry for your confusion.
> > Second, if _LPI is ever used on x86, the
> > acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
> > get in the way.
> AFAICS, it's also ok if X86 platform use LPI.

No, because it returns an error by default as it stands today.

> >
> > Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
> and RISCV.
> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
> return value.
> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
> main purpose is to setup cpudile device rather than to verify LPI.

That's fair enough.

Also, the list of idle states belongs to the cpuidle driver, not to a
cpuidle device.

> So I move it to a more prominent position and redefine the
> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
> >
> >> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> ---
> >>   drivers/acpi/processor_idle.c | 10 ++++++++--
> >>   1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index 5684925338b3..b0d6b51ee363 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> >>
> >>          dev->cpu = pr->id;
> >>          if (pr->flags.has_lpi)
> >> -               return acpi_processor_ffh_lpi_probe(pr->id);
> >> +               return 0;
> >>
> >>          return acpi_processor_setup_cpuidle_cx(pr, dev);
> >>   }
> >> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
> >>
> >>          ret = acpi_processor_get_lpi_info(pr);
> >>          if (ret)

So I think it would be better to check it here, that is

if (!ret) {
       ret = acpi_processor_ffh_lpi_probe(pr->id));
       if (!ret)
               return 0;

       pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
       pr->flags.has_lpi = 0;
}

return acpi_processor_get_cstate_info(pr);

And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.

> >> -               ret = acpi_processor_get_cstate_info(pr);
> >> +               return acpi_processor_get_cstate_info(pr);
> >> +
> >> +       if (pr->flags.has_lpi) {
> >> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
> >> +               if (ret)
> >> +                       pr_err("Processor FFH LPI state is invalid.\n");
> >> +       }
> >>
> >>          return ret;
> >>   }
> >> --

Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[lihuisong (C)]

在 2025/10/22 3:49, Rafael J. Wysocki 写道:
> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>> Driver will update power states when processor power states have been
>> changed. To prevent any other abnormal issues, here add the verification
>> for the result of getting power information, don't change power states
>> and one error log when get power information failed.
> But the old states may not be usable any more in that case.
Yes
>
> If you want to check the acpi_processor_get_power_info(), it should
> disable ACPi idle entirely on failures.
 From the modification of this patch, this cpuidle device will be 
disabled if the acpi_processor_get_power_info(）fails to get on this device.
And the cpuidle of the device will be disabled according to the 
definition of cpuidle_not_available().
We should not call disable_cpuidle() to disable cpuidle of all CPUs.
So the modification in this patch is enough, right?
>
>> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_idle.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index b0d6b51ee363..92b231f5d514 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1315,6 +1315,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>          int cpu;
>>          struct acpi_processor *_pr;
>>          struct cpuidle_device *dev;
>> +       int ret = 0;
>>
>>          if (disabled_by_idle_boot_param())
>>                  return 0;
>> @@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>                  }
>>
>>                  /* Populate Updated C-state information */
>> -               acpi_processor_get_power_info(pr);
>> -               acpi_processor_setup_cpuidle_states(pr);
>> +               ret = acpi_processor_get_power_info(pr);
>> +               if (ret)
>> +                       pr_err("Get processor-%u power information failed.\n",
>> +                              pr->id);
>> +               else
>> +                       acpi_processor_setup_cpuidle_states(pr);
>>
>>                  /* Enable all cpuidle devices */
>>                  for_each_online_cpu(cpu) {
>>                          _pr = per_cpu(processors, cpu);
>>                          if (!_pr || !_pr->flags.power_setup_done)
>>                                  continue;
>> -                       acpi_processor_get_power_info(_pr);
>> -                       if (_pr->flags.power) {
>> +                       ret = acpi_processor_get_power_info(_pr);
>> +                       if (!ret && _pr->flags.power) {
>>                                  dev = per_cpu(acpi_cpuidle_device, cpu);
>>                                  acpi_processor_setup_cpuidle_dev(_pr, dev);
>>                                  cpuidle_enable_device(dev);
>> @@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>                  cpus_read_unlock();
>>          }
>>
>> -       return 0;
>> +       return ret;
>>   }
>>
>>   void acpi_processor_register_idle_driver(void)
>> --
>> 2.33.0
>>

Re: [PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[lihuisong (C)]

在 2025/10/23 18:07, Rafael J. Wysocki 写道:
> On Thu, Oct 23, 2025 at 11:25 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/22 3:34, Rafael J. Wysocki 写道:
>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> According to ACPI spec, entry method in LPI sub-package must be buffer
>>>> or integer. However, acpi_processor_evaluate_lpi() regeards it as success
>>>> and treat it as an effective LPI state.
>>> Is that the case?  AFAICS, it just gets to the next state in this case
>>> and what's wrong with that?
>> The flatten_lpi_states() would consider the state with illegal entry
>> method sub-package as a valid one
>> if the flag of this state is enabled(ACPI_LPI_STATE_FLAGS_ENABLED is set).
>> And then cpuidle governor would use it because the caller of
>> acpi_processor_ffh_lpi_probe() also don't see the return value.
> So the problem appears to be that lpi_state increments in every step
> of the loop, but it should only increment if the given state is valid.
Yes,
So set the flag of the state with illegal entry method sub-package to 
zero so that this invalid LPI state will be skiped in 
flatten_lpi_states(), ok?

Re: [PATCH v1 3/9] ACPI: processor: idle: Return failure when get lpi_state->arch_flags failed

[lihuisong (C)]

在 2025/10/23 18:09, Rafael J. Wysocki 写道:
> On Thu, Oct 23, 2025 at 11:59 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/22 3:36, Rafael J. Wysocki 写道:
>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> The architecture specific context loss flags is important for ARM.
>>>> And this flag is used to control the execution of different code
>>>> flows in acpi_processor_ffh_lpi_enter().
>>>>
>>>> So it is better to return failure when get lpi_state->arch_flags
>>>> failed.
>>> A failure means no idle states at all.
>> Actually, I didn't know why driver should continue to do cpu idle
>> scaling if the idle state doesn't meet the developer's expectations.🙂
> There may be a firmware bug in one state description, but it doesn't
> mean that the other states are unusable, does it?
Ack
>
>>> Wouldn't it be better to skip the state with invalid arch flags?
>> This arch flags is important.  And acpi_processor_ffh_lpi_enter will use it.
>> There is no other place to verify its validity. so here do it.
>> This check is just to prevent potential issues in cpuidle scaling later.
> I'm saying to treat this particular state as invalid and skip it
> without rejecting all of the other states that may be valid.

Ok, handle this case the same as illegal entry method in patch 2/9 if it 
is ok to you.

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[lihuisong (C)]

在 2025/10/23 18:35, Rafael J. Wysocki 写道:
> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
>>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
>>>> if FFH of LPI state are not ok.
>>> First of all, I cannot parse this changelog, so I don't know the
>>> motivation for the change.
>> Sorry for your confusion.
>>> Second, if _LPI is ever used on x86, the
>>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
>>> get in the way.
>> AFAICS, it's also ok if X86 platform use LPI.
> No, because it returns an error by default as it stands today.
>
>>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
>> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
>> and RISCV.
>> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
>> return value.
>> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
>> main purpose is to setup cpudile device rather than to verify LPI.
> That's fair enough.
>
> Also, the list of idle states belongs to the cpuidle driver, not to a
> cpuidle device.
>
>> So I move it to a more prominent position and redefine the
>> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>>    drivers/acpi/processor_idle.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index 5684925338b3..b0d6b51ee363 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>>>
>>>>           dev->cpu = pr->id;
>>>>           if (pr->flags.has_lpi)
>>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
>>>> +               return 0;
>>>>
>>>>           return acpi_processor_setup_cpuidle_cx(pr, dev);
>>>>    }
>>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>>>>
>>>>           ret = acpi_processor_get_lpi_info(pr);
>>>>           if (ret)
> So I think it would be better to check it here, that is
>
> if (!ret) {
>         ret = acpi_processor_ffh_lpi_probe(pr->id));
>         if (!ret)
>                 return 0;
>
>         pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
>         pr->flags.has_lpi = 0;
> }
>
> return acpi_processor_get_cstate_info(pr);
>
> And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
Sorry, I don't understand why pr->flags.has_lpi is true if 
acpi_processor_ffh_lpi_probe() return failure.
In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe(). 
this function will return EOPNOTSUPP.
>
>>>> -               ret = acpi_processor_get_cstate_info(pr);
>>>> +               return acpi_processor_get_cstate_info(pr);
>>>> +
>>>> +       if (pr->flags.has_lpi) {
>>>> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
>>>> +               if (ret)
>>>> +                       pr_err("Processor FFH LPI state is invalid.\n");
>>>> +       }
>>>>
>>>>           return ret;
>>>>    }
>>>> --

Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[Rafael J. Wysocki]

On Fri, Oct 24, 2025 at 11:10 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/22 3:49, Rafael J. Wysocki 写道:
> > On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> Driver will update power states when processor power states have been
> >> changed. To prevent any other abnormal issues, here add the verification
> >> for the result of getting power information, don't change power states
> >> and one error log when get power information failed.
> > But the old states may not be usable any more in that case.
> Yes
> >
> > If you want to check the acpi_processor_get_power_info(), it should
> > disable ACPi idle entirely on failures.
>  From the modification of this patch, this cpuidle device will be
> disabled if the acpi_processor_get_power_info(）fails to get on this device.
> And the cpuidle of the device will be disabled according to the
> definition of cpuidle_not_available().
> We should not call disable_cpuidle() to disable cpuidle of all CPUs.

Since the same idle state data is used for all CPUs, I'd say cpuidle
should be disabled for all of them in that case.

Alternatively, check if it works for any of them and apply the data
from the CPU where it works to all of them.  If it doesn't work for
any of them, there's nothing to apply.

> So the modification in this patch is enough, right?
> >
> >> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> ---
> >>   drivers/acpi/processor_idle.c | 15 ++++++++++-----
> >>   1 file changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index b0d6b51ee363..92b231f5d514 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -1315,6 +1315,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> >>          int cpu;
> >>          struct acpi_processor *_pr;
> >>          struct cpuidle_device *dev;
> >> +       int ret = 0;
> >>
> >>          if (disabled_by_idle_boot_param())
> >>                  return 0;
> >> @@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> >>                  }
> >>
> >>                  /* Populate Updated C-state information */
> >> -               acpi_processor_get_power_info(pr);
> >> -               acpi_processor_setup_cpuidle_states(pr);
> >> +               ret = acpi_processor_get_power_info(pr);
> >> +               if (ret)
> >> +                       pr_err("Get processor-%u power information failed.\n",
> >> +                              pr->id);
> >> +               else
> >> +                       acpi_processor_setup_cpuidle_states(pr);
> >>
> >>                  /* Enable all cpuidle devices */
> >>                  for_each_online_cpu(cpu) {
> >>                          _pr = per_cpu(processors, cpu);
> >>                          if (!_pr || !_pr->flags.power_setup_done)
> >>                                  continue;
> >> -                       acpi_processor_get_power_info(_pr);
> >> -                       if (_pr->flags.power) {
> >> +                       ret = acpi_processor_get_power_info(_pr);
> >> +                       if (!ret && _pr->flags.power) {
> >>                                  dev = per_cpu(acpi_cpuidle_device, cpu);
> >>                                  acpi_processor_setup_cpuidle_dev(_pr, dev);
> >>                                  cpuidle_enable_device(dev);
> >> @@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> >>                  cpus_read_unlock();
> >>          }
> >>
> >> -       return 0;
> >> +       return ret;
> >>   }
> >>
> >>   void acpi_processor_register_idle_driver(void)
> >> --
> >> 2.33.0
> >>

Re: [PATCH v1 2/9] ACPI: processor: idle: Return failure if entry method is not buffer or integer type

[Rafael J. Wysocki]

On Fri, Oct 24, 2025 at 11:25 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/23 18:07, Rafael J. Wysocki 写道:
> > On Thu, Oct 23, 2025 at 11:25 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2025/10/22 3:34, Rafael J. Wysocki 写道:
> >>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>> According to ACPI spec, entry method in LPI sub-package must be buffer
> >>>> or integer. However, acpi_processor_evaluate_lpi() regeards it as success
> >>>> and treat it as an effective LPI state.
> >>> Is that the case?  AFAICS, it just gets to the next state in this case
> >>> and what's wrong with that?
> >> The flatten_lpi_states() would consider the state with illegal entry
> >> method sub-package as a valid one
> >> if the flag of this state is enabled(ACPI_LPI_STATE_FLAGS_ENABLED is set).
> >> And then cpuidle governor would use it because the caller of
> >> acpi_processor_ffh_lpi_probe() also don't see the return value.
> > So the problem appears to be that lpi_state increments in every step
> > of the loop, but it should only increment if the given state is valid.
> Yes,
> So set the flag of the state with illegal entry method sub-package to
> zero so that this invalid LPI state will be skiped in
> flatten_lpi_states(), ok?

Sounds reasonable.

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Rafael J. Wysocki]

On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
> > On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> >>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
> >>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
> >>>> if FFH of LPI state are not ok.
> >>> First of all, I cannot parse this changelog, so I don't know the
> >>> motivation for the change.
> >> Sorry for your confusion.
> >>> Second, if _LPI is ever used on x86, the
> >>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
> >>> get in the way.
> >> AFAICS, it's also ok if X86 platform use LPI.
> > No, because it returns an error by default as it stands today.
> >
> >>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
> >> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
> >> and RISCV.
> >> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
> >> return value.
> >> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
> >> main purpose is to setup cpudile device rather than to verify LPI.
> > That's fair enough.
> >
> > Also, the list of idle states belongs to the cpuidle driver, not to a
> > cpuidle device.
> >
> >> So I move it to a more prominent position and redefine the
> >> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
> >>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> ---
> >>>>    drivers/acpi/processor_idle.c | 10 ++++++++--
> >>>>    1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >>>> index 5684925338b3..b0d6b51ee363 100644
> >>>> --- a/drivers/acpi/processor_idle.c
> >>>> +++ b/drivers/acpi/processor_idle.c
> >>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> >>>>
> >>>>           dev->cpu = pr->id;
> >>>>           if (pr->flags.has_lpi)
> >>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
> >>>> +               return 0;
> >>>>
> >>>>           return acpi_processor_setup_cpuidle_cx(pr, dev);
> >>>>    }
> >>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
> >>>>
> >>>>           ret = acpi_processor_get_lpi_info(pr);
> >>>>           if (ret)
> > So I think it would be better to check it here, that is
> >
> > if (!ret) {
> >         ret = acpi_processor_ffh_lpi_probe(pr->id));
> >         if (!ret)
> >                 return 0;
> >
> >         pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
> >         pr->flags.has_lpi = 0;
> > }
> >
> > return acpi_processor_get_cstate_info(pr);
> >
> > And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
> Sorry, I don't understand why pr->flags.has_lpi is true if
> acpi_processor_ffh_lpi_probe() return failure.

It is set by acpi_processor_get_lpi_info() on success and
acpi_processor_ffh_lpi_probe() does not update it.

> In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe().
> this function will return EOPNOTSUPP.

Which is exactly why it is a problem.  x86 has no reason to implement
it because FFH always works there.

> >
> >>>> -               ret = acpi_processor_get_cstate_info(pr);
> >>>> +               return acpi_processor_get_cstate_info(pr);
> >>>> +
> >>>> +       if (pr->flags.has_lpi) {
> >>>> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
> >>>> +               if (ret)
> >>>> +                       pr_err("Processor FFH LPI state is invalid.\n");
> >>>> +       }
> >>>>
> >>>>           return ret;
> >>>>    }
> >>>> --

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[lihuisong (C)]

在 2025/10/26 20:40, Rafael J. Wysocki 写道:
> On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
>>> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>>>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
>>>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
>>>>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
>>>>>> if FFH of LPI state are not ok.
>>>>> First of all, I cannot parse this changelog, so I don't know the
>>>>> motivation for the change.
>>>> Sorry for your confusion.
>>>>> Second, if _LPI is ever used on x86, the
>>>>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
>>>>> get in the way.
>>>> AFAICS, it's also ok if X86 platform use LPI.
>>> No, because it returns an error by default as it stands today.
>>>
>>>>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
>>>> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
>>>> and RISCV.
>>>> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
>>>> return value.
>>>> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
>>>> main purpose is to setup cpudile device rather than to verify LPI.
>>> That's fair enough.
>>>
>>> Also, the list of idle states belongs to the cpuidle driver, not to a
>>> cpuidle device.
>>>
>>>> So I move it to a more prominent position and redefine the
>>>> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
>>>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> ---
>>>>>>     drivers/acpi/processor_idle.c | 10 ++++++++--
>>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>>>> index 5684925338b3..b0d6b51ee363 100644
>>>>>> --- a/drivers/acpi/processor_idle.c
>>>>>> +++ b/drivers/acpi/processor_idle.c
>>>>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>>>>>
>>>>>>            dev->cpu = pr->id;
>>>>>>            if (pr->flags.has_lpi)
>>>>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
>>>>>> +               return 0;
>>>>>>
>>>>>>            return acpi_processor_setup_cpuidle_cx(pr, dev);
>>>>>>     }
>>>>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>>>>>>
>>>>>>            ret = acpi_processor_get_lpi_info(pr);
>>>>>>            if (ret)
>>> So I think it would be better to check it here, that is
>>>
>>> if (!ret) {
>>>          ret = acpi_processor_ffh_lpi_probe(pr->id));
>>>          if (!ret)
>>>                  return 0;
>>>
>>>          pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
>>>          pr->flags.has_lpi = 0;
>>> }
>>>
>>> return acpi_processor_get_cstate_info(pr);
>>>
>>> And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
>> Sorry, I don't understand why pr->flags.has_lpi is true if
>> acpi_processor_ffh_lpi_probe() return failure.
> It is set by acpi_processor_get_lpi_info() on success and
> acpi_processor_ffh_lpi_probe() does not update it.
The acpi_processor_get_lpi_info() will return failure on X86 platform 
because this function first call acpi_processor_ffh_lpi_probe().
And acpi_processor_ffh_lpi_probe return EOPNOTSUPP because X86 platform 
doesn't implement it.
So I think pr->flags.has_lpi is false on X86 plaform.
>
>> In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe().
>> this function will return EOPNOTSUPP.
> Which is exactly why it is a problem.  x86 has no reason to implement
> it because FFH always works there.
Sorry, I still don't understand why X86 has no reason to implement it.
I simply think that X86 doesn't need it.
AFAICS, the platform doesn't need to get LPI info if this platform 
doesn't implement acpi_processor_ffh_lpi_probe().
>
>>>>>> -               ret = acpi_processor_get_cstate_info(pr);
>>>>>> +               return acpi_processor_get_cstate_info(pr);
>>>>>> +
>>>>>> +       if (pr->flags.has_lpi) {
>>>>>> +               ret = acpi_processor_ffh_lpi_probe(pr->id);
>>>>>> +               if (ret)
>>>>>> +                       pr_err("Processor FFH LPI state is invalid.\n");
>>>>>> +       }
>>>>>>
>>>>>>            return ret;
>>>>>>     }
>>>>>> --

Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[lihuisong (C)]

在 2025/10/26 20:34, Rafael J. Wysocki 写道:
> On Fri, Oct 24, 2025 at 11:10 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/22 3:49, Rafael J. Wysocki 写道:
>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> Driver will update power states when processor power states have been
>>>> changed. To prevent any other abnormal issues, here add the verification
>>>> for the result of getting power information, don't change power states
>>>> and one error log when get power information failed.
>>> But the old states may not be usable any more in that case.
>> Yes
>>> If you want to check the acpi_processor_get_power_info(), it should
>>> disable ACPi idle entirely on failures.
>>   From the modification of this patch, this cpuidle device will be
>> disabled if the acpi_processor_get_power_info(）fails to get on this device.
>> And the cpuidle of the device will be disabled according to the
>> definition of cpuidle_not_available().
>> We should not call disable_cpuidle() to disable cpuidle of all CPUs.
> Since the same idle state data is used for all CPUs, I'd say cpuidle
Yes.
 From the current implementation perspective, the idle state is 
initialized by the first available CPU.
If there is one CPU get power management information failed later, the 
ACPI idle driver doesn't disable cpuidle of all CPUs and
just doesn't register cpudile_device and enable cpuidle_device.
> should be disabled for all of them in that case.
I can understand this. I think it is reasonable.
What do you think how to disable cpuidle of all CPUs here?
How about call disable_cpuidle() and disable all cpuidle devices?
>
> Alternatively, check if it works for any of them and apply the data
> from the CPU where it works to all of them.  If it doesn't work for
> any of them, there's nothing to apply.

How should we check if the idle states can work to all of CPUs?

>
>> So the modification in this patch is enough, right?
>>>> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>>    drivers/acpi/processor_idle.c | 15 ++++++++++-----
>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index b0d6b51ee363..92b231f5d514 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -1315,6 +1315,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>>>           int cpu;
>>>>           struct acpi_processor *_pr;
>>>>           struct cpuidle_device *dev;
>>>> +       int ret = 0;
>>>>
>>>>           if (disabled_by_idle_boot_param())
>>>>                   return 0;
>>>> @@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>>>                   }
>>>>
>>>>                   /* Populate Updated C-state information */
>>>> -               acpi_processor_get_power_info(pr);
>>>> -               acpi_processor_setup_cpuidle_states(pr);
>>>> +               ret = acpi_processor_get_power_info(pr);
>>>> +               if (ret)
>>>> +                       pr_err("Get processor-%u power information failed.\n",
>>>> +                              pr->id);
>>>> +               else
>>>> +                       acpi_processor_setup_cpuidle_states(pr);
>>>>
>>>>                   /* Enable all cpuidle devices */
>>>>                   for_each_online_cpu(cpu) {
>>>>                           _pr = per_cpu(processors, cpu);
>>>>                           if (!_pr || !_pr->flags.power_setup_done)
>>>>                                   continue;
>>>> -                       acpi_processor_get_power_info(_pr);
>>>> -                       if (_pr->flags.power) {
>>>> +                       ret = acpi_processor_get_power_info(_pr);
>>>> +                       if (!ret && _pr->flags.power) {
>>>>                                   dev = per_cpu(acpi_cpuidle_device, cpu);
>>>>                                   acpi_processor_setup_cpuidle_dev(_pr, dev);
>>>>                                   cpuidle_enable_device(dev);
>>>> @@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>>>                   cpus_read_unlock();
>>>>           }
>>>>
>>>> -       return 0;
>>>> +       return ret;
>>>>    }
>>>>
>>>>    void acpi_processor_register_idle_driver(void)
>>>> --
>>>> 2.33.0
>>>>

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Rafael J. Wysocki]

On Mon, Oct 27, 2025 at 2:43 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/26 20:40, Rafael J. Wysocki 写道:
> > On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
> >>> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> >>>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
> >>>>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
> >>>>>> if FFH of LPI state are not ok.
> >>>>> First of all, I cannot parse this changelog, so I don't know the
> >>>>> motivation for the change.
> >>>> Sorry for your confusion.
> >>>>> Second, if _LPI is ever used on x86, the
> >>>>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
> >>>>> get in the way.
> >>>> AFAICS, it's also ok if X86 platform use LPI.
> >>> No, because it returns an error by default as it stands today.
> >>>
> >>>>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
> >>>> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
> >>>> and RISCV.
> >>>> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
> >>>> return value.
> >>>> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
> >>>> main purpose is to setup cpudile device rather than to verify LPI.
> >>> That's fair enough.
> >>>
> >>> Also, the list of idle states belongs to the cpuidle driver, not to a
> >>> cpuidle device.
> >>>
> >>>> So I move it to a more prominent position and redefine the
> >>>> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
> >>>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> >>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>>>> ---
> >>>>>>     drivers/acpi/processor_idle.c | 10 ++++++++--
> >>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >>>>>> index 5684925338b3..b0d6b51ee363 100644
> >>>>>> --- a/drivers/acpi/processor_idle.c
> >>>>>> +++ b/drivers/acpi/processor_idle.c
> >>>>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> >>>>>>
> >>>>>>            dev->cpu = pr->id;
> >>>>>>            if (pr->flags.has_lpi)
> >>>>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
> >>>>>> +               return 0;
> >>>>>>
> >>>>>>            return acpi_processor_setup_cpuidle_cx(pr, dev);
> >>>>>>     }
> >>>>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
> >>>>>>
> >>>>>>            ret = acpi_processor_get_lpi_info(pr);
> >>>>>>            if (ret)
> >>> So I think it would be better to check it here, that is
> >>>
> >>> if (!ret) {
> >>>          ret = acpi_processor_ffh_lpi_probe(pr->id));
> >>>          if (!ret)
> >>>                  return 0;
> >>>
> >>>          pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
> >>>          pr->flags.has_lpi = 0;
> >>> }
> >>>
> >>> return acpi_processor_get_cstate_info(pr);
> >>>
> >>> And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
> >> Sorry, I don't understand why pr->flags.has_lpi is true if
> >> acpi_processor_ffh_lpi_probe() return failure.
> > It is set by acpi_processor_get_lpi_info() on success and
> > acpi_processor_ffh_lpi_probe() does not update it.
> The acpi_processor_get_lpi_info() will return failure on X86 platform
> because this function first call acpi_processor_ffh_lpi_probe().
> And acpi_processor_ffh_lpi_probe return EOPNOTSUPP because X86 platform
> doesn't implement it.
> So I think pr->flags.has_lpi is false on X86 plaform.

On x86 it is 0, but what if acpi_processor_ffh_lpi_probe() fails on arm64, say?

> >
> >> In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe().
> >> this function will return EOPNOTSUPP.
> > Which is exactly why it is a problem.  x86 has no reason to implement
> > it because FFH always works there.
> Sorry, I still don't understand why X86 has no reason to implement it.
> I simply think that X86 doesn't need it.
> AFAICS, the platform doesn't need to get LPI info if this platform
> doesn't implement acpi_processor_ffh_lpi_probe().

Well, that's what is implemented in the current code, but it will need
to be changed if x86 is ever added and I'd rather avoid cleanups
making it harder to change.

Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

[Rafael J. Wysocki]

On Mon, Oct 27, 2025 at 4:01 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/26 20:34, Rafael J. Wysocki 写道:
> > On Fri, Oct 24, 2025 at 11:10 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2025/10/22 3:49, Rafael J. Wysocki 写道:
> >>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>> Driver will update power states when processor power states have been
> >>>> changed. To prevent any other abnormal issues, here add the verification
> >>>> for the result of getting power information, don't change power states
> >>>> and one error log when get power information failed.
> >>> But the old states may not be usable any more in that case.
> >> Yes
> >>> If you want to check the acpi_processor_get_power_info(), it should
> >>> disable ACPi idle entirely on failures.
> >>   From the modification of this patch, this cpuidle device will be
> >> disabled if the acpi_processor_get_power_info(）fails to get on this device.
> >> And the cpuidle of the device will be disabled according to the
> >> definition of cpuidle_not_available().
> >> We should not call disable_cpuidle() to disable cpuidle of all CPUs.
> > Since the same idle state data is used for all CPUs, I'd say cpuidle
> Yes.
>  From the current implementation perspective, the idle state is
> initialized by the first available CPU.
> If there is one CPU get power management information failed later, the
> ACPI idle driver doesn't disable cpuidle of all CPUs and
> just doesn't register cpudile_device and enable cpuidle_device.
> > should be disabled for all of them in that case.
> I can understand this. I think it is reasonable.
> What do you think how to disable cpuidle of all CPUs here?
> How about call disable_cpuidle() and disable all cpuidle devices?
> >
> > Alternatively, check if it works for any of them and apply the data
> > from the CPU where it works to all of them.  If it doesn't work for
> > any of them, there's nothing to apply.
>
> How should we check if the idle states can work to all of CPUs?

I mean if there is a CPU with a valid list of idle states, that list
can be used for all of the other CPUs.

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[lihuisong (C)]

在 2025/10/27 20:28, Rafael J. Wysocki 写道:
> On Mon, Oct 27, 2025 at 2:43 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/26 20:40, Rafael J. Wysocki 写道:
>>> On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>>> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
>>>>> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>>>>>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
>>>>>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>>>>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
>>>>>>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
>>>>>>>> if FFH of LPI state are not ok.
>>>>>>> First of all, I cannot parse this changelog, so I don't know the
>>>>>>> motivation for the change.
>>>>>> Sorry for your confusion.
>>>>>>> Second, if _LPI is ever used on x86, the
>>>>>>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
>>>>>>> get in the way.
>>>>>> AFAICS, it's also ok if X86 platform use LPI.
>>>>> No, because it returns an error by default as it stands today.
>>>>>
>>>>>>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
>>>>>> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
>>>>>> and RISCV.
>>>>>> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
>>>>>> return value.
>>>>>> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
>>>>>> main purpose is to setup cpudile device rather than to verify LPI.
>>>>> That's fair enough.
>>>>>
>>>>> Also, the list of idle states belongs to the cpuidle driver, not to a
>>>>> cpuidle device.
>>>>>
>>>>>> So I move it to a more prominent position and redefine the
>>>>>> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
>>>>>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>> ---
>>>>>>>>      drivers/acpi/processor_idle.c | 10 ++++++++--
>>>>>>>>      1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>>>>>> index 5684925338b3..b0d6b51ee363 100644
>>>>>>>> --- a/drivers/acpi/processor_idle.c
>>>>>>>> +++ b/drivers/acpi/processor_idle.c
>>>>>>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>>>>>>>
>>>>>>>>             dev->cpu = pr->id;
>>>>>>>>             if (pr->flags.has_lpi)
>>>>>>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
>>>>>>>> +               return 0;
>>>>>>>>
>>>>>>>>             return acpi_processor_setup_cpuidle_cx(pr, dev);
>>>>>>>>      }
>>>>>>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>>>>>>>>
>>>>>>>>             ret = acpi_processor_get_lpi_info(pr);
>>>>>>>>             if (ret)
>>>>> So I think it would be better to check it here, that is
>>>>>
>>>>> if (!ret) {
>>>>>           ret = acpi_processor_ffh_lpi_probe(pr->id));
>>>>>           if (!ret)
>>>>>                   return 0;
>>>>>
>>>>>           pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
>>>>>           pr->flags.has_lpi = 0;
>>>>> }
>>>>>
>>>>> return acpi_processor_get_cstate_info(pr);
>>>>>
>>>>> And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
>>>> Sorry, I don't understand why pr->flags.has_lpi is true if
>>>> acpi_processor_ffh_lpi_probe() return failure.
>>> It is set by acpi_processor_get_lpi_info() on success and
>>> acpi_processor_ffh_lpi_probe() does not update it.
>> The acpi_processor_get_lpi_info() will return failure on X86 platform
>> because this function first call acpi_processor_ffh_lpi_probe().
>> And acpi_processor_ffh_lpi_probe return EOPNOTSUPP because X86 platform
>> doesn't implement it.
>> So I think pr->flags.has_lpi is false on X86 plaform.
> On x86 it is 0, but what if acpi_processor_ffh_lpi_probe() fails on arm64, say?
Arm64 supports the acpi_processor_ffh_lpi_probe().
So pr->flags.has_lpi is 1 on success.
>>>> In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe().
>>>> this function will return EOPNOTSUPP.
>>> Which is exactly why it is a problem.  x86 has no reason to implement
>>> it because FFH always works there.
>> Sorry, I still don't understand why X86 has no reason to implement it.
>> I simply think that X86 doesn't need it.
>> AFAICS, the platform doesn't need to get LPI info if this platform
>> doesn't implement acpi_processor_ffh_lpi_probe().
> Well, that's what is implemented in the current code, but it will need
> to be changed if x86 is ever added and I'd rather avoid cleanups
> making it harder to change.
What you mean is that X86 use LPI?
If X86 also define acpi_processor_ffh_lpi_probe and use LPI, this patch 
is also good to it.

Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of processor FFH LPI state

[Rafael J. Wysocki]

On Tue, Oct 28, 2025 at 1:45 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/10/27 20:28, Rafael J. Wysocki 写道:
> > On Mon, Oct 27, 2025 at 2:43 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>
> >> 在 2025/10/26 20:40, Rafael J. Wysocki 写道:
> >>> On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>>> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
> >>>>> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
> >>>>>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> >>>>>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>>>>>> Both ARM64 and RISCV architecture would validate Entry Method of LPI
> >>>>>>>> state and SBI HSM or PSCI cpu suspend. Driver should return failure
> >>>>>>>> if FFH of LPI state are not ok.
> >>>>>>> First of all, I cannot parse this changelog, so I don't know the
> >>>>>>> motivation for the change.
> >>>>>> Sorry for your confusion.
> >>>>>>> Second, if _LPI is ever used on x86, the
> >>>>>>> acpi_processor_ffh_lpi_probe() in acpi_processor_get_power_info() will
> >>>>>>> get in the way.
> >>>>>> AFAICS, it's also ok if X86 platform use LPI.
> >>>>> No, because it returns an error by default as it stands today.
> >>>>>
> >>>>>>> Why does the evaluation in acpi_processor_setup_cpuidle_dev() not work?
> >>>>>> The acpi_processor_ffh_lpi_probe does verify the validity of LPI for ARM
> >>>>>> and RISCV.
> >>>>>> But the caller of the acpi_processor_setup_cpuidle_dev()don't verify the
> >>>>>> return value.
> >>>>>> In addition, from the name of acpi_processor_setup_cpuidle_dev(), its
> >>>>>> main purpose is to setup cpudile device rather than to verify LPI.
> >>>>> That's fair enough.
> >>>>>
> >>>>> Also, the list of idle states belongs to the cpuidle driver, not to a
> >>>>> cpuidle device.
> >>>>>
> >>>>>> So I move it to a more prominent position and redefine the
> >>>>>> acpi_processor_setup_cpuidle_dev to void in patch 9/9.
> >>>>>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> >>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/acpi/processor_idle.c | 10 ++++++++--
> >>>>>>>>      1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >>>>>>>> index 5684925338b3..b0d6b51ee363 100644
> >>>>>>>> --- a/drivers/acpi/processor_idle.c
> >>>>>>>> +++ b/drivers/acpi/processor_idle.c
> >>>>>>>> @@ -1264,7 +1264,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> >>>>>>>>
> >>>>>>>>             dev->cpu = pr->id;
> >>>>>>>>             if (pr->flags.has_lpi)
> >>>>>>>> -               return acpi_processor_ffh_lpi_probe(pr->id);
> >>>>>>>> +               return 0;
> >>>>>>>>
> >>>>>>>>             return acpi_processor_setup_cpuidle_cx(pr, dev);
> >>>>>>>>      }
> >>>>>>>> @@ -1275,7 +1275,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
> >>>>>>>>
> >>>>>>>>             ret = acpi_processor_get_lpi_info(pr);
> >>>>>>>>             if (ret)
> >>>>> So I think it would be better to check it here, that is
> >>>>>
> >>>>> if (!ret) {
> >>>>>           ret = acpi_processor_ffh_lpi_probe(pr->id));
> >>>>>           if (!ret)
> >>>>>                   return 0;
> >>>>>
> >>>>>           pr_info("CPU%d: FFH LPI state is invalid\n", pr->id);
> >>>>>           pr->flags.has_lpi = 0;
> >>>>> }
> >>>>>
> >>>>> return acpi_processor_get_cstate_info(pr);
> >>>>>
> >>>>> And the default acpi_processor_ffh_lpi_probe() needs to be changed to return 0.
> >>>> Sorry, I don't understand why pr->flags.has_lpi is true if
> >>>> acpi_processor_ffh_lpi_probe() return failure.
> >>> It is set by acpi_processor_get_lpi_info() on success and
> >>> acpi_processor_ffh_lpi_probe() does not update it.
> >> The acpi_processor_get_lpi_info() will return failure on X86 platform
> >> because this function first call acpi_processor_ffh_lpi_probe().
> >> And acpi_processor_ffh_lpi_probe return EOPNOTSUPP because X86 platform
> >> doesn't implement it.
> >> So I think pr->flags.has_lpi is false on X86 plaform.
> > On x86 it is 0, but what if acpi_processor_ffh_lpi_probe() fails on arm64, say?
> Arm64 supports the acpi_processor_ffh_lpi_probe().
> So pr->flags.has_lpi is 1 on success.
> >>>> In addition, X86 platform doesn't define acpi_processor_ffh_lpi_probe().
> >>>> this function will return EOPNOTSUPP.
> >>> Which is exactly why it is a problem.  x86 has no reason to implement
> >>> it because FFH always works there.
> >> Sorry, I still don't understand why X86 has no reason to implement it.
> >> I simply think that X86 doesn't need it.
> >> AFAICS, the platform doesn't need to get LPI info if this platform
> >> doesn't implement acpi_processor_ffh_lpi_probe().
>
> > Well, that's what is implemented in the current code, but it will need
> > to be changed if x86 is ever added and I'd rather avoid cleanups
> > making it harder to change.
>
> What you mean is that X86 use LPI?

In the future, x86 may want to use _LPI, that's all.

> If X86 also define acpi_processor_ffh_lpi_probe and use LPI, this patch
> is also good to it.

Well, fair enough.