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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <339a202a-86aa-46f5-b45d-aea653f3e382@huawei.com>
Date: Tue, 4 Nov 2025 17:54:45 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: <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 v2 4/7] ACPI: processor: idle: Disable ACPI idle if get
 power information failed in power notify


在 2025/11/4 2:09, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@...wei.com> 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@...wei.com>
>> ---
>>   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)
>> --