Re: [PATCH v3 4/5] PM: Add visible HIBERNATION_INTERFACE and hide HIBERNATION

[Shriram Rajagopalan <rshriram@cs.ubc.ca>]

2011/3/15 Rafael J. Wysocki <rjw@sisk.pl>:
> 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@cs.ubc.ca>
>> ---
>>  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
>
>