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

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

[Chen Yu]

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;
+	/*
 	 * Resume both child devices before entering system sleep. This
 	 * ensures that they are in proper state before they get suspended.
 	 */
-- 
2.7.4

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

[Lee Jones]

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'?

> +	/*
>  	 * Resume both child devices before entering system sleep. This
>  	 * ensures that they are in proper state before they get suspended.
>  	 */

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

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

[Chen Yu]

Hi,
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". 

Thanks,
Yu

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

[Lee Jones]

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?

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

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

[Rafael J. Wysocki]

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,

Rafael

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@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? 

Thanks,
Yu

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

[Lee Jones]

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