Re: [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify

[lihuisong (C)]

在 2025/11/4 2:09, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@xxxxxxxxxx> wrote:
> > The old states may not be usable any more if get power information
> > failed in power notify. The ACPI idle should be disabled entirely.
> How does it actually disable anything?  It only changes the
> acpi_processor_power_state_has_changed() return value AFAICS, but that
> return value isn't checked.
The acpi_processor_power_state_has_changed() will disable all cpuidle 
device first.
AFAICS, the disabled cpuidle_device would not do cpuidle, please see 
cpuidle_not_available() and cpuidle_idle_call().
It's enough for this?
> > Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
> So how does it fix anything?
>
> > Signed-off-by: Huisong Li <lihuisong@xxxxxxxxxx>
> > ---
> >   drivers/acpi/processor_idle.c | 22 +++++++++++++++++-----
> >   1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index c73df5933691..4627b00257e6 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -1317,6 +1317,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;
> > @@ -1345,8 +1346,18 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> >                          cpuidle_disable_device(dev);
> >                  }
> >
> > -               /* Populate Updated C-state information */
> > -               acpi_processor_get_power_info(pr);
> > +               /*
> > +                * Populate Updated C-state information
> > +                * The same idle state is used for all CPUs, cpuidle of all CPUs
> > +                * should be disabled.
> > +                */
> > +               ret = acpi_processor_get_power_info(pr);
> > +               if (ret) {
> > +                       pr_err("Get processor-%u power information failed, disable cpuidle of all CPUs\n",
> > +                              pr->id);
> pr_info() at most, preferably pr_debug() or maybe pr_info_once().
Ack, pr_info_once is good to me.
> > +                       goto release_lock;
> "unlock" would be a better name.
Ack
> > +               }
> > +
> >                  acpi_processor_setup_cpuidle_states(pr);
> >
> >                  /* Enable all cpuidle devices */
> > @@ -1354,18 +1365,19 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> >                          _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);
> This does not need to be called if _pr->flags.power is unset.  Why are
> you changing this?

_pr->flags.power is set in acpi_processor_get_power_info().
Ok, I know what you mean.
_pr->flags.power is unset if acpi_processor_get_power_info fail to execute.
But it may be the old value. So here should be necessary.

> > +                       if (!ret && _pr->flags.power) {
> >                                  dev = per_cpu(acpi_cpuidle_device, cpu);
> >                                  acpi_processor_setup_cpuidle_dev(_pr, dev);
> >                                  cpuidle_enable_device(dev);
> >                          }
> If it succeeds for the next CPU, the return value will be still 0, won't it?
I think it is 0.
Do we need to do something for it, like, adding debug log?
> >                  }
> > +release_lock:
> >                  cpuidle_resume_and_unlock();
> >                  cpus_read_unlock();
> >          }
> >
> > -       return 0;
> > +       return ret;
> >   }
> >
> >   void acpi_processor_register_idle_driver(void)
> > --