Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()

From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: Tejun Heo <tj@kernel.org>, "Rafael J. Wysocki" <rjw@sisk.pl>,
	Linux PM list <linux-pm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	horms@verge.net.au, Len Brown <lenb@kernel.org>
Subject: Re: [Update][PATCH] PM / Hibernate: Fix s2disk regression related to unlock_system_sleep()
Date: Thu, 19 Jan 2012 23:40:34 +0530	[thread overview]
Message-ID: <4F185C9A.9060408@linux.vnet.ibm.com> (raw)
In-Reply-To: <20120119174038.GA25397@atrey.karlin.mff.cuni.cz>

On 01/19/2012 11:10 PM, Pavel Machek wrote:

> Hi!
> 
>>>> To give an example,
>>>>
>>>> 	/*
>>>> 	 * XXX: Open code SKIP clearing for now to avoid invoking
>>>> 	 * try_to_freeze().  This isn't correct but this function is
>>>> 	 * called from deep inside hibernation path and calling
>>>> 	 * try_to_freeze() leads to hang during hibernation.  This
>>>> 	 * will be properly fixed soon.  See commit message for
>>>> 	 * more details.
>>>> 	 */
>>>
>>> Which paths are affected?
>>>
>>> With uswsusp, we have userland in control of hibernation process; I'd
>>> say almost anything can be called...
>>
>>
>> Hi Pavel,
>> I am afraid you are looking at a stale comment. We finalized on a
>> different comment altogether.
> 
> I know. But the underlying problem (uswsusp code is userland, and can
> do anything) is still there, right? I guess limitations for
> applications using uswsusp interface should be documented somewhere.

Sorry, but I think you missed the conclusion of the discussion in this
thread: The comment mentioned above is completely wrong! Rewriting
unlock_system_sleep() by open coding the SKIP clearing and omitting the
try_to_freeze() part is not a hack - instead, it is a clean and sane
design. Hence, with the latest version of my patch applied, it would
no longer be "hey, please watch out for this, this and this for now;
we know things are messy but we will fix it up later".
Instead it is like "Don't worry, we have fixed up the buggy
unlock_system_sleep() function. Go ahead and do what you want!"

On a more serious note, here is a (hopefully) convincing explanation
as to why the patch makes perfect sense:

http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1240892
and
http://thread.gmane.org/gmane.linux.kernel/1240493/focus=1240940

Moreover, we have already advised people to use the lock_system_sleep()
and unlock_system_sleep() APIs instead of directly using mutex_lock and
mutex_unlock on pm_mutex, in Documentation/power/freezing-of-tasks.txt

But unfortunately, unlock_system_sleep() was actually not quite correct
and hence this patch fixes it. So things should be fine now...

But, in case you were hinting at some totally different problem, please
let me know..

Regards,
Srivatsa S. Bhat