2011/3/15 Rafael J. Wysocki <rjw@xxxxxxx>:
> Hi,
>
> Sorry, I didn't have the time to review the patch in detail before.
>
> On Saturday, March 12, 2011, Shriram Rajagopalan wrote:
>> HIBERNATION covers the main hibernation control code and freeze-thaw
>> pm events, that xen's save/restore also uses. Explicitly enabling
>> an independant hibernation functionality to enable xen's save/restore
>> is a bit ugly. Define a new user visible symbol HIBERNATION_INTERFACE
>> that "selects" HIBERNATION and covers the main hibernation control code
>> instead of HIBERNATION. This way, we can also make XEN_SAVE_RESTORE
>> "select" HIBERNATION, enabling only the freeze-thaw code.
>>
>> Signed-off-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>
>> ---
>> kernel/power/Kconfig | 9 +++++++--
>> kernel/power/hibernate.c | 4 ++++
>> kernel/power/main.c | 2 +-
>> kernel/power/user.c | 2 ++
>> 4 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
>> index 4603f08..493c678 100644
>> --- a/kernel/power/Kconfig
>> +++ b/kernel/power/Kconfig
>> @@ -19,10 +19,15 @@ config SUSPEND_FREEZER
>> Turning OFF this setting is NOT recommended! If in doubt, say Y.
>>
>> config HIBERNATION
>> - bool "Hibernation (aka 'suspend to disk')"
>> - depends on SWAP && ARCH_HIBERNATION_POSSIBLE
>> + def_bool n
>
> You don't need that. Simply use "bool"
>
>> + depends on ARCH_HIBERNATION_POSSIBLE
>
> That need not depend on it, HIBERNATION_INTERFACE should instead.
>
Does that mean the pm_ops interface does not depend on the ARCH_HIB.. ?
>> select LZO_COMPRESS
>> select LZO_DECOMPRESS
>
> These two selects may be moved to HIBERNATION_INTERFACE too.
>
>> +
>> +config HIBERNATION_INTERFACE
>> + bool "Hibernation (aka 'suspend to disk')"
>> + depends on SWAP
>> + select HIBERNATION
>> ---help---
>> Enable the suspend to disk (STD) functionality, which is usually
>> called "hibernation" in user interfaces. STD checkpoints the
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index 1832bd2..13bcf69 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -592,6 +592,7 @@ static int prepare_processes(void)
>> * hibernate - The granpappy of the built-in hibernation management
>> */
>>
>> +#ifdef CONFIG_HIBERNATION_INTERFACE
>
> Please don't do that. Please make the whole hibernate.c depend on
> CONFIG_HIBERNATION_INTERFACE.
>
This #if only wraps the hibernate() function. There is a whole lot of code
(global functions too) before the hibernate() function.
As i mentioned in the first email 0/5, simply making
hibernate.c and user.c depend on HIBERNATION_INTERFACE does not work.
I got the following linker errors.
kernel/built-in.o: In function `sys_reboot':
kernel/sys.c:440: undefined reference to `hibernate'
kernel/built-in.o: In function `state_store':
kernel/power/main.c:185: undefined reference to `hibernate'
kernel/built-in.o: In function `hibernate_preallocate_memory':
kernel/power/snapshot.c:1411: undefined reference to `swsusp_show_speed'
kernel/built-in.o: In function `swsusp_check':
kernel/power/swap.c:933: undefined reference to `swsusp_resume_device'
kernel/power/swap.c:938: undefined reference to `swsusp_resume_block'
kernel/power/swap.c:946: undefined reference to `swsusp_resume_block'
kernel/built-in.o: In function `load_image_lzo':
kernel/power/swap.c:878: undefined reference to `swsusp_show_speed'
kernel/built-in.o: In function `load_image':
kernel/power/swap.c:741: undefined reference to `swsusp_show_speed'
kernel/built-in.o: In function `save_image_lzo':
kernel/power/swap.c:502: undefined reference to `lzo1x_1_compress'
kernel/power/swap.c:543: undefined reference to `swsusp_show_speed'
kernel/built-in.o: In function `swsusp_swap_check':
kernel/power/swap.c:221: undefined reference to `swsusp_resume_block'
kernel/power/swap.c:221: undefined reference to `swsusp_resume_device'
kernel/built-in.o: In function `mark_swapfiles':
kernel/power/swap.c:195: undefined reference to `swsusp_resume_block'
kernel/power/swap.c:202: undefined reference to `swsusp_resume_block'
kernel/built-in.o: In function `save_image':
kernel/power/swap.c:418: undefined reference to `swsusp_show_speed'
drivers/built-in.o: In function `acpi_suspend':
drivers/acpi/sleep.c:584: undefined reference to `hibernate'
drivers/built-in.o: In function `ata_scsi_start_stop_xlat':
drivers/ata/libata-scsi.c:1331: undefined reference to
`system_entering_hibernation'
drivers/built-in.o: In function `acpi_sleep_init':
drivers/acpi/sleep.c:782: undefined reference to `hibernation_set_ops'
arch/x86/power/built-in.o: In function `restore_registers':
arch/x86/power/hibernate_asm_64.S:145: undefined reference to `in_suspend'
Seeing errors from arch/.. was also the reason why I made HIBERNATE depend on
ARCH_HIBERNATION_POSSIBLE
>> int hibernate(void)
>> {
>> int error;
>> @@ -667,6 +668,8 @@ int hibernate(void)
>> return error;
>> }
>>
>> +#else /* !CONFIG_HIBERNATION_INTERFACE */
>> +int hibernate(void) { return -ENOSYS; }
>
> And move that to a header.
was done for the same reason as above. linux/suspend.h
has a similar definition but it encompasses a whole set of
swsusp_* functions, register_nosave stuff. But defining alternate
versions there means hibernate.c, snapshot.c etc have to depend on
HIBERNATION_INTERFACE - which causes errors stated above.
>
>> /**
>> * software_resume - Resume from a saved image.
>> @@ -1029,3 +1032,4 @@ __setup("noresume", noresume_setup);
>> __setup("resume_offset=", resume_offset_setup);
>> __setup("resume=", resume_setup);
>> __setup("hibernate=", hibernate_setup);
>> +#endif /* !CONFIG_HIBERNATION_INTERFACE */
>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>> index 8eaba5f..686a130 100644
>> --- a/kernel/power/main.c
>> +++ b/kernel/power/main.c
>> @@ -156,7 +156,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>> s += sprintf(s,"%s ", pm_states[i]);
>> }
>> #endif
>> -#ifdef CONFIG_HIBERNATION
>> +#ifdef CONFIG_HIBERNATION_INTERFACE
>> s += sprintf(s, "%s\n", "disk");
>> #else
>> if (s != buf)
>> diff --git a/kernel/power/user.c b/kernel/power/user.c
>> index c36c3b9..5f36ee7 100644
>> --- a/kernel/power/user.c
>> +++ b/kernel/power/user.c
>> @@ -458,6 +458,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
>> return error;
>> }
>>
>> +#ifdef CONFIG_HIBERNATION_INTERFACE
>> static const struct file_operations snapshot_fops = {
>> .open = snapshot_open,
>> .release = snapshot_release,
>> @@ -479,3 +480,4 @@ static int __init snapshot_device_init(void)
>> };
>>
>> device_initcall(snapshot_device_init);
>> +#endif /* CONFIG_HIBERNATION_INTERFACE */
>>
>
> Again, please make the entire user.c depend on CONFIG_HIBERNATION_INTERFACE
> instead of adding #ifdefs to the file.
>
As above.
> Thanks,
> Rafael
>
>
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
