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

[lihuisong (C)]

On 1/15/2026 9:06 PM, Rafael J. Wysocki wrote:
> On Thu, Jan 15, 2026 at 1:09 PM lihuisong (C) <lihuisong@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >    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?

>