linux-kernel - Re: [PATCH v3 2/5] ACPI / sleep: move acpi_processor

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]

Date:	Wed, 17 Feb 2016 12:03:05 +0000
From:	Sudeep Holla <sudeep.holla@....com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Sudeep Holla <sudeep.holla@....com>, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-ia64@...r.kernel.org,
	x86@...nel.org, Al Stone <al.stone@...aro.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Mahesh Sivasubramanian <msivasub@...eaurora.org>,
	Ashwin Chaugule <ashwin.chaugule@...aro.org>,
	Prashanth Prakash <pprakash@...eaurora.org>
Subject: Re: [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c



On 16/02/16 20:13, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:43 PM Sudeep Holla wrote:
>> acpi_processor_sleep is neither related nor used by CPUIdle framework.
>> It's used in system suspend/resume path as a syscore operation. It makes
>> more sense to move it to acpi/sleep.c where all the S-state transition
>> (a.k.a. Linux system suspend/hiberate) related code are present.
>>
>> Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
>> it's not compiled on architecture like ARM64 where S-states are not
>> yet defined in ACPI.
>>
>> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>
> To me this goes in the right direction, but I'd take it a bit further.
>
>> ---
>>   drivers/acpi/processor_driver.c |  2 --
>>   drivers/acpi/processor_idle.c   | 37 -------------------------------------
>>   drivers/acpi/sleep.c            | 35 +++++++++++++++++++++++++++++++++++
>>   include/acpi/processor.h        |  8 --------
>>   4 files changed, 35 insertions(+), 47 deletions(-)
>>

[...]

>> @@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
>>   static inline void acpi_sleep_suspend_setup(void) {}
>>   #endif /* !CONFIG_SUSPEND */
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static u32 saved_bm_rld;
>> +
>> +static int acpi_processor_suspend(void)
>
> Why do we need mention processor in the function name here (and below)?
>
> I'd call it something like acpi_save/restore_bm_rld() (maybe with a short comment
> explaining what the BM RLD is).
>

Sure, I had thought so initially and wanted to do that in a separate
patch for easy of review but totally forgot later. Thanks for pointing
it out. Updated patch on it's way.

-- 
Regards,
Sudeep