[GIT PULL] Power management Kconfig modification for 2.6.39

[GIT PULL] Power management Kconfig modification for 2.6.39

[Rafael J. Wysocki]

[Forgot to CC the lists and interested people, sorry.]

Hi Linus,

Please pull a power management Kconfig modification for 2.6.39 from:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git for-linus

It modifies power management core Kconfig options so that Xen save/restore
can select CONFIG_HIBERNATION without including the whole image creating and
saving code and the hibernate user space interfaces into the kernel.


 arch/x86/power/Makefile |    3 ++-
 include/linux/suspend.h |   15 +++++----------
 kernel/power/Kconfig    |    4 ++++
 kernel/power/Makefile   |    4 ++--
 kernel/power/main.c     |    2 +-
 kernel/power/power.h    |    6 +++---
 6 files changed, 17 insertions(+), 17 deletions(-)

---------------

Rafael J. Wysocki (1):
      PM / Hibernate: Introduce CONFIG_HIBERNATE_INTERFACE

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Linus Torvalds]

On Tue, Mar 29, 2011 at 12:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> Please pull a power management Kconfig modification for 2.6.39 from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git for-linus

I absolutely detest this.

The way you changed the Kconfig file makes no sense what-so-ever, and
now the Kconfig options are exactly the wrong way around.

We should NOT ask users "do you want the hibernation interface?".
That's just a totally idiotic question to ask, doubly so when it's not
what we ask for the STR "interfaces" either.

>From a user standpoint, the fact that Xen may want to use the
low-level code for hibernation should NOT mean that the user should be
asked about some interface. That's not what they care about, and it's
not what they _should_ care about.

They want hibernation support. You shouldn't ask whether they want the
"interface". That's just crazy.

Now, I realize that you did it the way you did to make the diff
smaller, but I really don't think that's a good reason to make the
config options be the wrong way around, and ask questions (and show a
config option name) of users that are illogical and doesn't match the
other related config options they see. That's the wrong choice to make
just to keep a patch smaller.

I could imagine using just "CONFIG_HIBERNATE" instead of your
suggested "CONFIG_HIBERNATION_INTERFACE", as that would at least match
the current CONFIG_SUSPEND that we ask users about - ie at least the
config option would not be visibly strange to the user. Still, I think
that would be prone to confusion at a code level ("What's the
difference between CONFIG_HIBERNATE and CONFIG_HIBERNATION?"), so I
think that's a bad idea too.

So I'd suggest just making the patch bigger, and make the new
(non-asked) config option be called CONFIG_HIBERNATION_SUPPORT or
something like that. Make Xen select that option instead.

(I also think that your patch was actually wrong. Why does the x86
code to actually save/restore registers depend on that "INTERFACE"
thing, but not on any other architecture? In general, I just think the
thing is confusing. Maybe it's confusing exactly because the config
option name doesn't actually describe _what_ part of the hibernation
code it is all about?)

                     Linus

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Rafael J. Wysocki]

On Wednesday, March 30, 2011, Linus Torvalds wrote:
> On Tue, Mar 29, 2011 at 12:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > Please pull a power management Kconfig modification for 2.6.39 from:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git for-linus
> 
> I absolutely detest this.
> 
> The way you changed the Kconfig file makes no sense what-so-ever, and
> now the Kconfig options are exactly the wrong way around.
> 
> We should NOT ask users "do you want the hibernation interface?".
> That's just a totally idiotic question to ask, doubly so when it's not
> what we ask for the STR "interfaces" either.
> 
> From a user standpoint, the fact that Xen may want to use the
> low-level code for hibernation should NOT mean that the user should be
> asked about some interface. That's not what they care about, and it's
> not what they _should_ care about.
> 
> They want hibernation support. You shouldn't ask whether they want the
> "interface". That's just crazy.
> 
> Now, I realize that you did it the way you did to make the diff
> smaller, but I really don't think that's a good reason to make the
> config options be the wrong way around, and ask questions (and show a
> config option name) of users that are illogical and doesn't match the
> other related config options they see. That's the wrong choice to make
> just to keep a patch smaller.

Well, otherwise I'd have to change CONFIG_HIBERNATION to something else
throughout the tree and then add CONFIG_HIBERNATION as a user-visible option.
Perhaps that would be better from the user's viewpoint, but the confusion would
be still there.

> I could imagine using just "CONFIG_HIBERNATE" instead of your
> suggested "CONFIG_HIBERNATION_INTERFACE", as that would at least match
> the current CONFIG_SUSPEND that we ask users about - ie at least the
> config option would not be visibly strange to the user. Still, I think
> that would be prone to confusion at a code level ("What's the
> difference between CONFIG_HIBERNATE and CONFIG_HIBERNATION?"), so I
> think that's a bad idea too.

Exactly.

> So I'd suggest just making the patch bigger, and make the new
> (non-asked) config option be called CONFIG_HIBERNATION_SUPPORT or
> something like that. Make Xen select that option instead.
> 
> (I also think that your patch was actually wrong. Why does the x86
> code to actually save/restore registers depend on that "INTERFACE"
> thing, but not on any other architecture? In general, I just think the
> thing is confusing. Maybe it's confusing exactly because the config
> option name doesn't actually describe _what_ part of the hibernation
> code it is all about?)

It is confusing, because Xen wanted to use hibernate callbacks without
supporting hibernation itself and then realized it should rathern not tell
user space that hibernation was supported.  Kind of the other way around.

Now, I agree, to the users it would be _less_ confusing to rename
CONFIG_HIBERNATION everywhere to eg. CONFIG_HIBERNATE_CALLBACKS and then
change every instance of CONFIG_HIBERNATE_CALLBACKS that's not about callbacks
to CONFIG_HIBERNATE (or CONFIG_HIBERNATION again), but that's going to
be confusing to every maintainer of the affected code (which is the majority
of architectures in the tree).  I simply wasn't quite sure which one to
choose, so I took an easier path.  If you want the more complicated one, then
so be it.

Thanks,
Rafael

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Linus Torvalds]

On Wed, Mar 30, 2011 at 1:47 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> Well, otherwise I'd have to change CONFIG_HIBERNATION to something else
> throughout the tree and then add CONFIG_HIBERNATION as a user-visible option.
> Perhaps that would be better from the user's viewpoint, but the confusion would
> be still there.

Umm. It would be *less* confusing than the thing you sent me.

I would heartily support just renaming them all. Who cares about patch
size? What matters is the end result not being confusing.

Your current patch results in an end result that doesn't make sense.
That's what I'm complaining about. The #ifdef's don't make sense, and
the question to the user doesn't make sense.

It is in all ways _inferior_ to the current situation. So I simply
won't pull it.

The thing it tries to fix (some crazy Xen detail) is not worth messing
up the main code for.

So I'd be happy to fix up whatever issues Xen has, but not at the cost
of making all the _sane_ configurations more confusing.

> Now, I agree, to the users it would be _less_ confusing to rename
> CONFIG_HIBERNATION everywhere to eg. CONFIG_HIBERNATE_CALLBACKS and then
> change every instance of CONFIG_HIBERNATE_CALLBACKS that's not about callbacks
> to CONFIG_HIBERNATE (or CONFIG_HIBERNATION again), but that's going to
> be confusing to every maintainer of the affected code (which is the majority
> of architectures in the tree).  I simply wasn't quite sure which one to
> choose, so I took an easier path.  If you want the more complicated one, then
> so be it.

Well, so having looked at the current #ifdef'fery, I actually think
the Xen people would need to do more than that to make sense.

For example, just renaming CONFIG_HIBERNATION to
CONFIG_HIBERNATE_CALLBACKS is insane too. It makes some of the
#ifdef's look just completely insane. Looking at the current state of
<linux/mm.h>, for example, we have things like this:

#ifdef CONFIG_HIBERNATION
extern bool kernel_page_present(struct page *page);
#endif /* CONFIG_HIBERNATION */
#else
static inline void
kernel_map_pages(struct page *page, int numpages, int enable) {}
static inline void enable_debug_pagealloc(void)
{
}
#ifdef CONFIG_HIBERNATION
static inline bool kernel_page_present(struct page *page) { return true; }
#endif /* CONFIG_HIBERNATION */
#endif

which is just nasty, but at least reading it makes sense ("ok,
kernel_page_present() is about HIBERNATION"). But would that be a
"callback" case? That's what your old patch implies (because it wasn't
changed to an "interface" case in your patch). But that makes no sense
either. What does that function have to do with the callbacks?

So once you split up CONFIG_HIBERNATION into multiple cases (and that
is true both for the HIBERNATION_INTERFACE and HIBERNATION_CALLBACKS
case), what is the rule for something like kernel_page_present()?
Something that sounds logical? Taking into account that the config
option needs to be logical for the person who actually sees the
_question_ too (which was the problem with the "INTERFACE" approach).

That mm.h thing - it sure doesn't look like some callback thing. Or if
it is, I'd like an explanation about it.

And why was the low-level x86 register save code under "interface"?
That made no logical sense either. To me, the _interface_ is about
things like /sys/power/disk etc, not about some low-level x86 code.

So I'd expect that

 (a) the main question to the user - and config variable) should be
CONFIG_HINBERNATE, matching CONFIG_SUSPEND. We're already a bit
illogical ("HIBERNATION" vs "SUSPEND" - if you google for those two
words, you'll notice that the top answers actually talk about
"hibernate" vs "suspend"), we shouldn't make it worse.

     This one is just a no-brainer. We shouldn't ask users about
"interfaces" in the first place, but more importantly, to the user the
HIBERNATE vs SUSPEND question are about the exact same kind of thing,
so if we _did_ ask about interfaces, then we should be consistent, and
change it to SUSPEND_INTERFACE too.  And I don't think anybody really
would want that.

 (b) when writing code, the #ifdef's you need should not surprise you. Doing a

    git grep '#.*if.*CONFIG_HIBERNAT'

  shouldn't result in some WTF moment. I still don't understand why
CONFIG_HIBERNATE_INTERFACE was the one that picked the x86
hibernate_[asm]_$(BITS).o files, for example. That was a very WTF
moment to me in the patch.

What parts is it that Xen actually wants? It doesn't seem to be much
of the core hibernate code at all - a lot of the core hibernate code
seemed to go under "interface" in your patch. But what about
kernel_page_present()? That's the kind of WTF? question that made me
go "no, this makes things worse".

                       Linus

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Rafael J. Wysocki]

On Wednesday, March 30, 2011, Linus Torvalds wrote:
> On Wed, Mar 30, 2011 at 1:47 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > Well, otherwise I'd have to change CONFIG_HIBERNATION to something else
> > throughout the tree and then add CONFIG_HIBERNATION as a user-visible option.
> > Perhaps that would be better from the user's viewpoint, but the confusion would
> > be still there.
> 
> Umm. It would be *less* confusing than the thing you sent me.
> 
> I would heartily support just renaming them all. Who cares about patch
> size? What matters is the end result not being confusing.

That, in fact, is not necessary.  Moreover, it turns out that the patch size
is not much different, because only a few subsystems use CONFIG_HIBERNATION
directly.

The patch below has only been slightly tested, in particular I'd like the Xen
people to check if it works for them.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Hibernate: Introduce CONFIG_HIBERNATE_CALLBACKS

Xen save/restore is going to use hibernate device callbacks for
quiescing devices and putting them back to normal operations and it
would need to select CONFIG_HIBERNATION for this purpose.  However,
that also would cause the hibernate interfaces for user space to be
enabled, which might confuse user space, because the Xen kernels
don't support hibernation.  Moreover, it would be wasteful, as it
would make the Xen kernels include a substantial amount of code that
they would never use.

To address this issue introduce new power management Kconfig option
CONFIG_HIBERNATE_CALLBACKS, such that it will only select the code
that is necessary for the hibernate device callbacks to work and make
CONFIG_HIBERNATION select it.  Then, Xen save/restore will be able to
select CONFIG_HIBERNATE_CALLBACKS without dragging the entire
hibernate code along with it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/powerpc/kernel/ibmebus.c |    6 +++---
 drivers/amba/bus.c            |    6 +++---
 drivers/base/platform.c       |    6 +++---
 drivers/base/power/main.c     |    8 ++++----
 drivers/pci/pci-driver.c      |    6 +++---
 drivers/xen/manage.c          |    6 +++---
 include/linux/suspend.h       |   11 +++--------
 kernel/power/Kconfig          |    6 +++++-
 8 files changed, 27 insertions(+), 28 deletions(-)

Index: linux-2.6/kernel/power/Kconfig
===================================================================
--- linux-2.6.orig/kernel/power/Kconfig
+++ linux-2.6/kernel/power/Kconfig
@@ -18,9 +18,13 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config HIBERNATE_CALLBACKS
+	bool
+
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on SWAP && ARCH_HIBERNATION_POSSIBLE
+	select HIBERNATE_CALLBACKS
 	select LZO_COMPRESS
 	select LZO_DECOMPRESS
 	---help---
@@ -85,7 +89,7 @@ config PM_STD_PARTITION
 
 config PM_SLEEP
 	def_bool y
-	depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
+	depends on SUSPEND || HIBERNATE_CALLBACKS || XEN_SAVE_RESTORE
 
 config PM_SLEEP_SMP
 	def_bool y
Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -249,6 +249,8 @@ extern void hibernation_set_ops(const st
 extern int hibernate(void);
 extern bool system_entering_hibernation(void);
 #else /* CONFIG_HIBERNATION */
+static inline void register_nosave_region(unsigned long b, unsigned long e) {}
+static inline void register_nosave_region_late(unsigned long b, unsigned long e) {}
 static inline int swsusp_page_is_forbidden(struct page *p) { return 0; }
 static inline void swsusp_set_page_free(struct page *p) {}
 static inline void swsusp_unset_page_free(struct page *p) {}
@@ -297,14 +299,7 @@ static inline bool pm_wakeup_pending(voi
 
 extern struct mutex pm_mutex;
 
-#ifndef CONFIG_HIBERNATION
-static inline void register_nosave_region(unsigned long b, unsigned long e)
-{
-}
-static inline void register_nosave_region_late(unsigned long b, unsigned long e)
-{
-}
-
+#ifndef CONFIG_HIBERNATE_CALLBACKS
 static inline void lock_system_sleep(void) {}
 static inline void unlock_system_sleep(void) {}
 
Index: linux-2.6/arch/powerpc/kernel/ibmebus.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/ibmebus.c
+++ linux-2.6/arch/powerpc/kernel/ibmebus.c
@@ -527,7 +527,7 @@ static int ibmebus_bus_pm_resume_noirq(s
 
 #endif /* !CONFIG_SUSPEND */
 
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 
 static int ibmebus_bus_pm_freeze(struct device *dev)
 {
@@ -665,7 +665,7 @@ static int ibmebus_bus_pm_restore_noirq(
 	return ret;
 }
 
-#else /* !CONFIG_HIBERNATION */
+#else /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #define ibmebus_bus_pm_freeze		NULL
 #define ibmebus_bus_pm_thaw		NULL
@@ -676,7 +676,7 @@ static int ibmebus_bus_pm_restore_noirq(
 #define ibmebus_bus_pm_poweroff_noirq	NULL
 #define ibmebus_bus_pm_restore_noirq	NULL
 
-#endif /* !CONFIG_HIBERNATION */
+#endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
 static struct dev_pm_ops ibmebus_bus_dev_pm_ops = {
 	.prepare = ibmebus_bus_pm_prepare,
Index: linux-2.6/drivers/amba/bus.c
===================================================================
--- linux-2.6.orig/drivers/amba/bus.c
+++ linux-2.6/drivers/amba/bus.c
@@ -214,7 +214,7 @@ static int amba_pm_resume_noirq(struct d
 
 #endif /* !CONFIG_SUSPEND */
 
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 
 static int amba_pm_freeze(struct device *dev)
 {
@@ -352,7 +352,7 @@ static int amba_pm_restore_noirq(struct
 	return ret;
 }
 
-#else /* !CONFIG_HIBERNATION */
+#else /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #define amba_pm_freeze		NULL
 #define amba_pm_thaw		NULL
@@ -363,7 +363,7 @@ static int amba_pm_restore_noirq(struct
 #define amba_pm_poweroff_noirq	NULL
 #define amba_pm_restore_noirq	NULL
 
-#endif /* !CONFIG_HIBERNATION */
+#endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #ifdef CONFIG_PM
 
Index: linux-2.6/drivers/base/platform.c
===================================================================
--- linux-2.6.orig/drivers/base/platform.c
+++ linux-2.6/drivers/base/platform.c
@@ -771,7 +771,7 @@ int __weak platform_pm_resume_noirq(stru
 
 #endif /* !CONFIG_SUSPEND */
 
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 
 static int platform_pm_freeze(struct device *dev)
 {
@@ -909,7 +909,7 @@ static int platform_pm_restore_noirq(str
 	return ret;
 }
 
-#else /* !CONFIG_HIBERNATION */
+#else /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #define platform_pm_freeze		NULL
 #define platform_pm_thaw		NULL
@@ -920,7 +920,7 @@ static int platform_pm_restore_noirq(str
 #define platform_pm_poweroff_noirq	NULL
 #define platform_pm_restore_noirq	NULL
 
-#endif /* !CONFIG_HIBERNATION */
+#endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #ifdef CONFIG_PM_RUNTIME
 
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -233,7 +233,7 @@ static int pm_op(struct device *dev,
 		}
 		break;
 #endif /* CONFIG_SUSPEND */
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		if (ops->freeze) {
@@ -260,7 +260,7 @@ static int pm_op(struct device *dev,
 			suspend_report_result(ops->restore, error);
 		}
 		break;
-#endif /* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATE_CALLBACKS */
 	default:
 		error = -EINVAL;
 	}
@@ -308,7 +308,7 @@ static int pm_noirq_op(struct device *de
 		}
 		break;
 #endif /* CONFIG_SUSPEND */
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 	case PM_EVENT_FREEZE:
 	case PM_EVENT_QUIESCE:
 		if (ops->freeze_noirq) {
@@ -335,7 +335,7 @@ static int pm_noirq_op(struct device *de
 			suspend_report_result(ops->restore_noirq, error);
 		}
 		break;
-#endif /* CONFIG_HIBERNATION */
+#endif /* CONFIG_HIBERNATE_CALLBACKS */
 	default:
 		error = -EINVAL;
 	}
Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -781,7 +781,7 @@ static int pci_pm_resume(struct device *
 
 #endif /* !CONFIG_SUSPEND */
 
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 
 static int pci_pm_freeze(struct device *dev)
 {
@@ -970,7 +970,7 @@ static int pci_pm_restore(struct device
 	return error;
 }
 
-#else /* !CONFIG_HIBERNATION */
+#else /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #define pci_pm_freeze		NULL
 #define pci_pm_freeze_noirq	NULL
@@ -981,7 +981,7 @@ static int pci_pm_restore(struct device
 #define pci_pm_restore		NULL
 #define pci_pm_restore_noirq	NULL
 
-#endif /* !CONFIG_HIBERNATION */
+#endif /* !CONFIG_HIBERNATE_CALLBACKS */
 
 #ifdef CONFIG_PM_RUNTIME
 
Index: linux-2.6/drivers/xen/manage.c
===================================================================
--- linux-2.6.orig/drivers/xen/manage.c
+++ linux-2.6/drivers/xen/manage.c
@@ -61,7 +61,7 @@ static void xen_post_suspend(int cancell
 	xen_mm_unpin_all();
 }
 
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 static int xen_suspend(void *data)
 {
 	struct suspend_info *si = data;
@@ -173,7 +173,7 @@ out:
 #endif
 	shutting_down = SHUTDOWN_INVALID;
 }
-#endif	/* CONFIG_HIBERNATION */
+#endif	/* CONFIG_HIBERNATE_CALLBACKS */
 
 struct shutdown_handler {
 	const char *command;
@@ -202,7 +202,7 @@ static void shutdown_handler(struct xenb
 		{ "poweroff",	do_poweroff },
 		{ "halt",	do_poweroff },
 		{ "reboot",	do_reboot   },
-#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_CALLBACKS
 		{ "suspend",	do_suspend  },
 #endif
 		{NULL, NULL},

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Linus Torvalds]

On Wed, Mar 30, 2011 at 2:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> That, in fact, is not necessary.  Moreover, it turns out that the patch size
> is not much different, because only a few subsystems use CONFIG_HIBERNATION
> directly.
>
> The patch below has only been slightly tested, in particular I'd like the Xen
> people to check if it works for them.

Ok, this seems to make much more sense. Just from a quick scan, the
places where you changed things to

  #ifdef CONFIG_HIBERNATE_CALLBACKS

really do seem to be about the freeze/thaw callbacks. So now the patch
looks more logical to me. And it obviously doesn't change the
user-visible questions.

Thank you,

            Linus

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Rafael J. Wysocki]

On Thursday, March 31, 2011, Linus Torvalds wrote:
> On Wed, Mar 30, 2011 at 2:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > That, in fact, is not necessary.  Moreover, it turns out that the patch size
> > is not much different, because only a few subsystems use CONFIG_HIBERNATION
> > directly.
> >
> > The patch below has only been slightly tested, in particular I'd like the Xen
> > people to check if it works for them.
> 
> Ok, this seems to make much more sense. Just from a quick scan, the
> places where you changed things to
> 
>   #ifdef CONFIG_HIBERNATE_CALLBACKS
> 
> really do seem to be about the freeze/thaw callbacks. So now the patch
> looks more logical to me. And it obviously doesn't change the
> user-visible questions.
> 
> Thank you,

Thanks for the review!

@Shriram: Can you please test if the new patch works with your other Xen
patches?

Rafael

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Shriram Rajagopalan]

Shriram

On 2011-03-30, at 6:39 PM, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, March 31, 2011, Linus Torvalds wrote:
>> On Wed, Mar 30, 2011 at 2:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> That, in fact, is not necessary.  Moreover, it turns out that the patch size
>>> is not much different, because only a few subsystems use CONFIG_HIBERNATION
>>> directly.
>>> 
>>> The patch below has only been slightly tested, in particular I'd like the Xen
>>> people to check if it works for them.
>> 
>> Ok, this seems to make much more sense. Just from a quick scan, the
>> places where you changed things to
>> 
>>  #ifdef CONFIG_HIBERNATE_CALLBACKS
>> 
>> really do seem to be about the freeze/thaw callbacks. So now the patch
>> looks more logical to me. And it obviously doesn't change the
>> user-visible questions.
>> 
>> Thank you,
> 
> Thanks for the review!
> 
> @Shriram: Can you please test if the new patch works with your other Xen
> patches?
> 
> Rafael
> 

Re: [GIT PULL] Power management Kconfig modification for 2.6.39

[Shriram Rajagopalan]

On 2011-03-30, at 6:39 PM, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Thursday, March 31, 2011, Linus Torvalds wrote:
>> On Wed, Mar 30, 2011 at 2:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> That, in fact, is not necessary.  Moreover, it turns out that the patch size
>>> is not much different, because only a few subsystems use CONFIG_HIBERNATION
>>> directly.
>>> 
>>> The patch below has only been slightly tested, in particular I'd like the Xen
>>> people to check if it works for them.
>> 
>> Ok, this seems to make much more sense. Just from a quick scan, the
>> places where you changed things to
>> 
>>  #ifdef CONFIG_HIBERNATE_CALLBACKS
>> 
>> really do seem to be about the freeze/thaw callbacks. So now the patch
>> looks more logical to me. And it obviously doesn't change the
>> user-visible questions.
>> 
>> Thank you,
> 
> Thanks for the review!
> 
> @Shriram: Can you please test if the new patch works with your other Xen
> patches?
> 
> Rafael
> 
Yep. Surely. Will get back to you in a couple of days. 

Shriram