Message-ID: <CAJZ5v0hTkVzuKQu4WFOxtdfC-uXTBYkBr77UurXj_zfDbgV0vg@mail.gmail.com>
Date: Tue, 28 Oct 2025 15:10:39 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "lihuisong (C)" <lihuisong@...wei.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, lenb@...nel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Sudeep.Holla@....com, linuxarm@...wei.com, 
	jonathan.cameron@...wei.com, zhanjie9@...ilicon.com, zhenglifeng1@...wei.com, 
	yubowen8@...wei.com
Subject: Re: [PATCH v1 5/9] ACPI: processor: idle: Add the verification of
 processor FFH LPI state

On Tue, Oct 28, 2025 at 1:45 PM lihuisong (C) <lihuisong@...wei.com> wrote:
>
>
> 在 2025/10/27 20:28, Rafael J. Wysocki 写道:
> > On Mon, Oct 27, 2025 at 2:43 AM lihuisong (C) <lihuisong@...wei.com> wrote:
> >>
> >> 在 2025/10/26 20:40, Rafael J. Wysocki 写道:
> >>> On Fri, Oct 24, 2025 at 11:40 AM lihuisong (C) <lihuisong@...wei.com> wrote:
> >>>> 在 2025/10/23 18:35, Rafael J. Wysocki 写道:
> >>>>> On Thu, Oct 23, 2025 at 12:17 PM lihuisong (C) <lihuisong@...wei.com> wrote:
> >>>>>> 在 2025/10/22 3:42, Rafael J. Wysocki 写道:
> >>>>>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@...wei.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@...wei.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.

Powered by blists - more mailing lists