From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752805AbaBTKhk (ORCPT ); Thu, 20 Feb 2014 05:37:40 -0500 Received: from service87.mimecast.com ([91.220.42.44]:34359 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751830AbaBTKhi convert rfc822-to-8bit (ORCPT ); Thu, 20 Feb 2014 05:37:38 -0500 Date: Thu, 20 Feb 2014 10:37:31 +0000 From: Lorenzo Pieralisi To: Russ Dill Cc: Sebastian Capella , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linaro-kernel@lists.linaro.org" , "linux-arm-kernel@lists.infradead.org" , "Rafael J. Wysocki" , Russell King , Len Brown , Pavel Machek , Nicolas Pitre , Santosh Shilimkar , Will Deacon , Jonathan Austin , Catalin Marinas , Uwe Kleine-K?nig , Stephen Boyd Subject: Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk Message-ID: <20140220103731.GB15994@e102568-lin.cambridge.arm.com> References: <1392774729-3235-1-git-send-email-sebastian.capella@linaro.org> <1392774729-3235-4-git-send-email-sebastian.capella@linaro.org> <20140219161254.GB19343@e102568-lin.cambridge.arm.com> <530501A7.2030703@ti.com> MIME-Version: 1.0 In-Reply-To: <530501A7.2030703@ti.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-OriginalArrivalTime: 20 Feb 2014 10:37:35.0957 (UTC) FILETIME=[C4EE0850:01CF2E27] X-MC-Unique: 114022010373503601 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote: > > + * https://patchwork.kernel.org/patch/96442/ > I am guessing the snippets of code your comments refer to. > I think the idea here is to get the CPU into a state so that later > when we resume from the resume kernel, the actual CPU state matches > the state we have in kernel. The main thing flush_thread does is clear > out any and all FP state. Which has already been saved through syscore_suspend().... > The may be part of the patchset that is OBE. It has to be updated then. > cpu_resume makes many assumptions about the state of the state of the > CPU, the primary being that the MMU is disabled, but also that all > caches and IRQs are disabled. soft_restart does all this for us. > > > > ah, you are saying just return from __swsusp_arch_save_image and allow > cpu_suspend_abort to be called, placing the result of swsusp_save > somewhere else. This may work and would reduce the complexity of the > code slightly. Yes. Basically you are doing a soft reboot just to return 0. > This is taken from the previous iteration of the patchset, I think the > comment is OBE. Updated it please then. > But this is still required to select the right mapping for our copying. /me confused. Please describe what switching to idmap is meant to achieve. In the patch above the copied swapper pgdir is not even used, I would like to understand why this is done. > I don't remember why I needed to prevent gcc from manipulating the > stack here. That's not a good reason to mark a function with attr __naked. If it is needed we leave it there, if it is not it has to go. > This is another holdover from previous patch versions that may be OBE. See above. Lorenzo From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk Date: Thu, 20 Feb 2014 10:37:31 +0000 Message-ID: <20140220103731.GB15994@e102568-lin.cambridge.arm.com> References: <1392774729-3235-1-git-send-email-sebastian.capella@linaro.org> <1392774729-3235-4-git-send-email-sebastian.capella@linaro.org> <20140219161254.GB19343@e102568-lin.cambridge.arm.com> <530501A7.2030703@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Return-path: Received: from service87.mimecast.com ([91.220.42.44]:34342 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751445AbaBTKhi convert rfc822-to-8bit (ORCPT ); Thu, 20 Feb 2014 05:37:38 -0500 In-Reply-To: <530501A7.2030703@ti.com> Content-Disposition: inline Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Russ Dill Cc: Sebastian Capella , "linux-kernel@vger.kernel.org" , "linux-pm@vger.kernel.org" , "linaro-kernel@lists.linaro.org" , "linux-arm-kernel@lists.infradead.org" , "Rafael J. Wysocki" , Russell King , Len Brown , Pavel Machek , Nicolas Pitre , Santosh Shilimkar , Will Deacon , Jonathan Austin , Catalin Marinas , Uwe Kleine-K?nig , Stephen Boyd On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote: > > + * https://patchwork.kernel.org/patch/96442/ > I am guessing the snippets of code your comments refer to. > I think the idea here is to get the CPU into a state so that later > when we resume from the resume kernel, the actual CPU state matches > the state we have in kernel. The main thing flush_thread does is clear > out any and all FP state. Which has already been saved through syscore_suspend().... > The may be part of the patchset that is OBE. It has to be updated then. > cpu_resume makes many assumptions about the state of the state of the > CPU, the primary being that the MMU is disabled, but also that all > caches and IRQs are disabled. soft_restart does all this for us. > > > > ah, you are saying just return from __swsusp_arch_save_image and allow > cpu_suspend_abort to be called, placing the result of swsusp_save > somewhere else. This may work and would reduce the complexity of the > code slightly. Yes. Basically you are doing a soft reboot just to return 0. > This is taken from the previous iteration of the patchset, I think the > comment is OBE. Updated it please then. > But this is still required to select the right mapping for our copying. /me confused. Please describe what switching to idmap is meant to achieve. In the patch above the copied swapper pgdir is not even used, I would like to understand why this is done. > I don't remember why I needed to prevent gcc from manipulating the > stack here. That's not a good reason to mark a function with attr __naked. If it is needed we leave it there, if it is not it has to go. > This is another holdover from previous patch versions that may be OBE. See above. Lorenzo From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Thu, 20 Feb 2014 10:37:31 +0000 Subject: [PATCH RFC v1 3/3] ARM hibernation / suspend-to-disk In-Reply-To: <530501A7.2030703@ti.com> References: <1392774729-3235-1-git-send-email-sebastian.capella@linaro.org> <1392774729-3235-4-git-send-email-sebastian.capella@linaro.org> <20140219161254.GB19343@e102568-lin.cambridge.arm.com> <530501A7.2030703@ti.com> Message-ID: <20140220103731.GB15994@e102568-lin.cambridge.arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Feb 19, 2014 at 07:10:31PM +0000, Russ Dill wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/19/2014 08:12 AM, Lorenzo Pieralisi wrote: > > + * https://patchwork.kernel.org/patch/96442/ > I am guessing the snippets of code your comments refer to. > I think the idea here is to get the CPU into a state so that later > when we resume from the resume kernel, the actual CPU state matches > the state we have in kernel. The main thing flush_thread does is clear > out any and all FP state. Which has already been saved through syscore_suspend().... > The may be part of the patchset that is OBE. It has to be updated then. > cpu_resume makes many assumptions about the state of the state of the > CPU, the primary being that the MMU is disabled, but also that all > caches and IRQs are disabled. soft_restart does all this for us. > > > > ah, you are saying just return from __swsusp_arch_save_image and allow > cpu_suspend_abort to be called, placing the result of swsusp_save > somewhere else. This may work and would reduce the complexity of the > code slightly. Yes. Basically you are doing a soft reboot just to return 0. > This is taken from the previous iteration of the patchset, I think the > comment is OBE. Updated it please then. > But this is still required to select the right mapping for our copying. /me confused. Please describe what switching to idmap is meant to achieve. In the patch above the copied swapper pgdir is not even used, I would like to understand why this is done. > I don't remember why I needed to prevent gcc from manipulating the > stack here. That's not a good reason to mark a function with attr __naked. If it is needed we leave it there, if it is not it has to go. > This is another holdover from previous patch versions that may be OBE. See above. Lorenzo