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

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

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?

>