Re: [PATCH][RFC] mfd: intel-lpss: Avoid resuming runtime-suspended lpss unnecessarily

[Lee Jones <lee.jones@linaro.org>]

On Tue, 20 Sep 2016, Chen, Yu C wrote:
> > -----Original Message-----
> > From: Wysocki, Rafael J
> > Sent: Tuesday, September 13, 2016 8:16 PM
> > To: Lee Jones; Chen, Yu C
> > Cc: linux-kernel@vger.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@linux.intel.com>
> > >>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >>>> Signed-off-by: Chen Yu <yu.c.chen@intel.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? 

I think that would be a great way to move forward.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog