On Wed, Oct 17, 2007 at 10:46:12AM +0800, Qi Yong wrote: > On 12/05/2007, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On Fri, 11 May 2007, Rafael J. Wysocki wrote: > > > > > > We're working on fixing the breakage, but currently it's difficult, because > > > none of my testboxes has problems with the 'platform' hibernation and I > > > cannot reproduce the reported issues. > > > > The rule for anything ACPI-related has been: no regressions. > > > > It doesn't matter if something fixes 10 boxes, if it breaks a single one, > > it's going to get reverted. > > > > We had much too much of the "two steps forward, one step back" dance with > > ACPI a few years ago, which is the reason that rule got installed (and > > which is why it's ACPI-only: in some other subsystems we accept the fact > > that sometimes we don't know how to fix some hardware issue, but the new > > situation is at least better than the old one). > > > > I agree that it can be aggravating to know that you can fix a problem for > > some people, but then being limited by the fact that it breaks for others. > > But beign able to *rely* on something that used to work is just too > > important, and with ACPI, you can never make a good judgement of which way > > works better (since it really just depends on some random firmware issues > > that we have zero visibility into). > > > > Also, quite often, it may *seem* like something fixes more boxes than it > > breaks, but it's because people report *breakage* only, and then a few > > months later it turns out that it's exactly the other way around: now it's > > a hundred people who report breakage with the *new* code, and the reason > > people thought it fixed more than it broke was that the people for whom > > the old code worked fine obviously never reported it! > > > > So this is why "a single regression is considered more important than ten > > fixes" - because a single regressionr report tends to actually be just the > > first indication of a lot of people who simply haven't tested the new code > > yet! People for whom the old code is broken are more likely to test new > > things. > > > > So I'd just suggest changing the default back to PM_DISK_SHUTDOWN (but > > leave the "pm_ops->enter" testing in place - ie not reverting the other > > commits in the series). > > > > The patch would look something like this. Coywolf, does this fix it for > > you? > > > > Yes, it fixes my problem. > > (Sorry for this long delayed report. I didn't get the chance to test > and reboot my box.) > > This quick test explains me the problem that we should not set > hibernation_mode to HIBERNATION_PLATFORM if it is not !ops". I will > post a formal patch later. > > diff --git a/kernel/power/disk.c b/kernel/power/disk.c > index eb72255..8e52553 100644 > --- a/kernel/power/disk.c > +++ b/kernel/power/disk.c > @@ -62,7 +62,7 @@ void hibernation_set_ops(struct hibernation_ops *ops) > mutex_lock(&pm_mutex); > hibernation_ops = ops; > if (ops) > - hibernation_mode = HIBERNATION_PLATFORM; > + ; > else if (hibernation_mode == HIBERNATION_PLATFORM) > hibernation_mode = HIBERNATION_SHUTDOWN; please apply. Signed-off-by: Qi Yong <qiyong@xxxxxxxxx> --- diff --git a/kernel/power/disk.c b/kernel/power/disk.c index eb72255..95b66ee 100644 --- a/kernel/power/disk.c +++ b/kernel/power/disk.c @@ -61,9 +61,11 @@ void hibernation_set_ops(struct hibernation_ops *ops) } mutex_lock(&pm_mutex); hibernation_ops = ops; - if (ops) - hibernation_mode = HIBERNATION_PLATFORM; - else if (hibernation_mode == HIBERNATION_PLATFORM) + + /* + * Turn off HIBERNATION_PLATFORM if we no longer have any platform ops. + */ + if (!ops && hibernation_mode == HIBERNATION_PLATFORM) hibernation_mode = HIBERNATION_SHUTDOWN; mutex_unlock(&pm_mutex); > > > Linus > > > > --- > > kernel/power/disk.c | 6 +++--- > > 1 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/power/disk.c b/kernel/power/disk.c > > index b5f0543..f6aa06e 100644 > > --- a/kernel/power/disk.c > > +++ b/kernel/power/disk.c > > @@ -60,9 +60,9 @@ void hibernation_set_ops(struct hibernation_ops *ops) > > } > > mutex_lock(&pm_mutex); > > hibernation_ops = ops; > > - if (ops) > > - hibernation_mode = HIBERNATION_PLATFORM; > > - else if (hibernation_mode == HIBERNATION_PLATFORM) > > + > > + /* Turn off HIBERNATION_PLATFORM if we no longer have any platform ops */ > > + if (!ops && hibernation_mode == HIBERNATION_PLATFORM) > > hibernation_mode = HIBERNATION_SHUTDOWN; > > > > mutex_unlock(&pm_mutex); > > > > > -- > Qi Yong > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ - To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
