linux-kernel - RE: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <36DF59CE26D8EE47B0655C516E9CE6405E60F6DD@shsmsx102.ccr.corp.intel.com>
Date:   Tue, 20 Sep 2016 00:18:17 +0000
From:   "Chen, Yu C" <yu.c.chen@...el.com>
To:     "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        Lee Jones <lee.jones@...aro.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: RE: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
 lpss unnecessarily

Hi,
> -----Original Message-----
> From: Wysocki, Rafael J
> Sent: Tuesday, September 13, 2016 8:16 PM
> To: Lee Jones; Chen, Yu C
> Cc: linux-kernel@...r.kernel.org; Andy Shevchenko; Mika Westerberg
> Subject: Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended
> lpss unnecessarily
> 
> On 9/13/2016 10:24 AM, Lee Jones wrote:
> > On Mon, 12 Sep 2016, Chen Yu wrote:
> >> On Mon, Sep 12, 2016 at 04:17:04PM +0100, Lee Jones wrote:
> >>> On Sun, 04 Sep 2016, Chen Yu wrote:
> >>>
> >>>> We have report that the intel_lpss_prepare() takes too much time
> >>>> during suspend, and this is because we first resume the devices
> >>>> from runtime suspend by resume_lpss_device(), to make sure they are
> >>>> in proper state before system suspend, which takes 100ms for each
> >>>> LPSS devices(PCI power state from D3_cold to D0). And since
> >>>> resume_lpss_device() resumes the devices synchronously, we might
> >>>> get huge latency if we have many LPSS devices.
> >>>>
> >>>> So first try is to use pm_request_resume() instead, to make the
> >>>> runtime resume process asynchronously. Unfortunately the
> >>>> asynchronous runtime resume relies on pm_wq, which is freezed at
> >>>> early stage. So we choose another method, that is to avoid resuming
> >>>> runtime-suspended devices, if they are already runtime suspended.
> >>>> This is safe because for LPSS driver, the runtime suspend and
> >>>> system suspend are of the same hook - i.e., intel_lpss_suspend().
> >>>> And moreover, this device is neither runtime wakeup source nor system
> wakeup source.
> >>>>
> >>>> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> >>>> Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>
> >>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >>>> Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> >>>> ---
> >>>>   drivers/mfd/intel-lpss.c | 9 +++++++++
> >>>>   1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> >>>> index 41b1138..6dcc9a0 100644
> >>>> --- a/drivers/mfd/intel-lpss.c
> >>>> +++ b/drivers/mfd/intel-lpss.c
> >>>> @@ -485,6 +485,15 @@ static int resume_lpss_device(struct device *dev,
> void *data)
> >>>>   int intel_lpss_prepare(struct device *dev)
> >>>>   {
> >>>>   	/*
> >>>> +	 * This is safe because:
> >>>> +	 * 1. The runtime suspend and system suspend
> >>>> +	 * are of the same hook.
> >>>> +	 * 2. This device is neither runtime wakeup source
> >>>> +	 * nor system wakeup source.
> >>>> +	 */
> >>>> +	if (pm_runtime_status_suspended(dev))
> >>>> +		return 1;
> >>> What's '1'?
> >>>
> >> According to the comment in device_prepare():
> >>
> >> A positive return value from ->prepare() means "this device appears
> >> to be runtime-suspended and its state is fine, so if it really is
> >> runtime-suspended, you can leave it in that state provided that you
> >> will do the same thing with all of its descendants".
> > Are there no defines for this?
> >
> 
> Not at the moment, but I guess they can be added if really necessary. :-)
> 
> But that said it doesn't have to be 1 or any specific value. Any positive number
> will have the same effect.
Thanks for point it out,  Hi Lee, should I  repost a patch set to define the return value
and make this one based on that? 

Thanks,
Yu