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

[Chen, Yu C]

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