ACPI: processor: idle: Enhance LPI verification and

[PATCH 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by Huisong Li 2 months, 2 weeks ago

The platform used LPI need check if the LPI support and the entry
method is valid by the acpi_processor_ffh_lpi_probe(). But the return
of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
isn't verified by any caller.

What's more, acpi_processor_get_power_info() is a more logical place for
verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
So move acpi_processor_ffh_lpi_probe() from the latter to the former and
verify its return.

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 5f86297c8b23..cdf86874a87a 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1252,7 +1252,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;
 
 	acpi_processor_setup_cpuidle_cx(pr, dev);
 	return 0;
@@ -1264,7 +1264,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 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by Rafael J. Wysocki 3 weeks, 4 days ago

On Tue, Nov 25, 2025 at 7:52 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The platform used LPI need check if the LPI support and the entry
> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
> isn't verified by any caller.
>
> What's more, acpi_processor_get_power_info() is a more logical place for
> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
> verify its return.
>
> 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 5f86297c8b23..cdf86874a87a 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1252,7 +1252,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;
>
>         acpi_processor_setup_cpuidle_cx(pr, dev);
>         return 0;
> @@ -1264,7 +1264,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;
>  }
> --

Please reorder this behind the next patch in the series.

Re: [PATCH 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by lihuisong (C) 3 weeks, 4 days ago

Hi Rafael,

On 1/15/2026 1:27 AM, Rafael J. Wysocki wrote:
> On Tue, Nov 25, 2025 at 7:52 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The platform used LPI need check if the LPI support and the entry
>> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
>> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
>> isn't verified by any caller.
>>
>> What's more, acpi_processor_get_power_info() is a more logical place for
>> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
>> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
>> verify its return.
>>
>> 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 5f86297c8b23..cdf86874a87a 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1252,7 +1252,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;
>>
>>          acpi_processor_setup_cpuidle_cx(pr, dev);
>>          return 0;
>> @@ -1264,7 +1264,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;
>>   }
>> --
> Please reorder this behind the next patch in the series.
Patch 2/3 depends on this patch.
So I don't know how to reorder this patch.
>

Re: [PATCH 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by Rafael J. Wysocki 3 weeks, 4 days ago

On Thu, Jan 15, 2026 at 1:09 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
> Hi Rafael,
>
> On 1/15/2026 1:27 AM, Rafael J. Wysocki wrote:
> > On Tue, Nov 25, 2025 at 7:52 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> The platform used LPI need check if the LPI support and the entry
> >> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
> >> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
> >> isn't verified by any caller.
> >>
> >> What's more, acpi_processor_get_power_info() is a more logical place for
> >> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
> >> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
> >> verify its return.
> >>
> >> 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 5f86297c8b23..cdf86874a87a 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -1252,7 +1252,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;
> >>
> >>          acpi_processor_setup_cpuidle_cx(pr, dev);
> >>          return 0;
> >> @@ -1264,7 +1264,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;
> >>   }
> >> --
> > Please reorder this behind the next patch in the series.
> Patch 2/3 depends on this patch.
> So I don't know how to reorder this patch.

I should have been more precise, sorry.

Please first convert acpi_processor_setup_cpuidle_dev() to a void
function and then make the changes from this patch on top of that.

Re: [PATCH 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by lihuisong (C) 3 weeks, 3 days ago

On 1/15/2026 9:06 PM, Rafael J. Wysocki wrote:
> On Thu, Jan 15, 2026 at 1:09 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>> Hi Rafael,
>>
>> On 1/15/2026 1:27 AM, Rafael J. Wysocki wrote:
>>> On Tue, Nov 25, 2025 at 7:52 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> The platform used LPI need check if the LPI support and the entry
>>>> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
>>>> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
>>>> isn't verified by any caller.
>>>>
>>>> What's more, acpi_processor_get_power_info() is a more logical place for
>>>> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
>>>> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
>>>> verify its return.
>>>>
>>>> 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 5f86297c8b23..cdf86874a87a 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -1252,7 +1252,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;
>>>>
>>>>           acpi_processor_setup_cpuidle_cx(pr, dev);
>>>>           return 0;
>>>> @@ -1264,7 +1264,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;
>>>>    }
>>>> --
>>> Please reorder this behind the next patch in the series.
>> Patch 2/3 depends on this patch.
>> So I don't know how to reorder this patch.
> I should have been more precise, sorry.
>
> Please first convert acpi_processor_setup_cpuidle_dev() to a void
> function and then make the changes from this patch on top of that.
The acpi_processor_ffh_lpi_probe may return an error.
And acpi_processor_setup_cpuidle_dev can pass the error code to its 
caller(Although the caller ignored it currently).
It may be inapproprate to convert acpi_processor_setup_cpuidle_dev() to 
a void function directly if we doesn't move acpi_processor_ffh_lpi_probe 
out first.
So I first relocate the position of acpi_processor_ffh_lpi_probe. Then 
changing it to a void function would be more logical.

Or we need to drop the return value of acpi_processor_ffh_lpi_probe and 
convert acpi_processor_setup_cpuidle_dev to a void function, like:
-->

-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 acpi_processor_ffh_lpi_probe();
+               acpi_processor_ffh_lpi_probe();
+		return;
         }
  
  	acpi_processor_setup_cpuidle_cx(pr, dev);
-	return 0;
  }

What do you think now?

>

Re: [PATCH 1/3] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe

Posted by Rafael J. Wysocki 3 weeks, 3 days ago

On Fri, Jan 16, 2026 at 7:38 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> On 1/15/2026 9:06 PM, Rafael J. Wysocki wrote:
> > On Thu, Jan 15, 2026 at 1:09 PM lihuisong (C) <lihuisong@huawei.com> wrote:
> >> Hi Rafael,
> >>
> >> On 1/15/2026 1:27 AM, Rafael J. Wysocki wrote:
> >>> On Tue, Nov 25, 2025 at 7:52 AM Huisong Li <lihuisong@huawei.com> wrote:
> >>>> The platform used LPI need check if the LPI support and the entry
> >>>> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
> >>>> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
> >>>> isn't verified by any caller.
> >>>>
> >>>> What's more, acpi_processor_get_power_info() is a more logical place for
> >>>> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
> >>>> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
> >>>> verify its return.
> >>>>
> >>>> 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 5f86297c8b23..cdf86874a87a 100644
> >>>> --- a/drivers/acpi/processor_idle.c
> >>>> +++ b/drivers/acpi/processor_idle.c
> >>>> @@ -1252,7 +1252,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;
> >>>>
> >>>>           acpi_processor_setup_cpuidle_cx(pr, dev);
> >>>>           return 0;
> >>>> @@ -1264,7 +1264,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;
> >>>>    }
> >>>> --
> >>> Please reorder this behind the next patch in the series.
> >> Patch 2/3 depends on this patch.
> >> So I don't know how to reorder this patch.
> > I should have been more precise, sorry.
> >
> > Please first convert acpi_processor_setup_cpuidle_dev() to a void
> > function and then make the changes from this patch on top of that.
> The acpi_processor_ffh_lpi_probe may return an error.
> And acpi_processor_setup_cpuidle_dev can pass the error code to its
> caller(Although the caller ignored it currently).

If all of its callers ignore its return value, it can and arguably
should be a void function.

> It may be inapproprate to convert acpi_processor_setup_cpuidle_dev() to
> a void function directly if we doesn't move acpi_processor_ffh_lpi_probe
> out first.
> So I first relocate the position of acpi_processor_ffh_lpi_probe. Then
> changing it to a void function would be more logical.
>
> Or we need to drop the return value of acpi_processor_ffh_lpi_probe and
> convert acpi_processor_setup_cpuidle_dev to a void function, like:
> -->
>
> -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 acpi_processor_ffh_lpi_probe();
> +               acpi_processor_ffh_lpi_probe();
> +               return;
>          }
>
>         acpi_processor_setup_cpuidle_cx(pr, dev);
> -       return 0;
>   }
>
> What do you think now?

Well, please see above.