Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 30 Mar 2011 14:36:03 -0700 | Subject | Re: [GIT PULL] Power management Kconfig modification for 2.6.39 |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |