[PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Fu, Zhonghui]

>From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
Date: Tue, 9 Sep 2014 13:02:25 +0800
Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

LPSS devices must suspend/resume in fixed order. Or some LPSS devices
will hang during the transition to ACPI_STATE_D0 state.

Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index fddc1e8..54e5c97 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -419,7 +419,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 	adev->driver_data = pdata;
 	pdev = acpi_create_platform_device(adev);
 	if (!IS_ERR_OR_NULL(pdev)) {
-		device_enable_async_suspend(&pdev->dev);
+		device_disable_async_suspend(&pdev->dev);
 		return 1;
 	}
 
-- 1.7.1

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Tuesday, September 09, 2014 01:36:48 PM Fu, Zhonghui wrote:
> From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> Date: Tue, 9 Sep 2014 13:02:25 +0800
> Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices
> 
> LPSS devices must suspend/resume in fixed order. Or some LPSS devices
> will hang during the transition to ACPI_STATE_D0 state.
> 
> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> ---
>  drivers/acpi/acpi_lpss.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index fddc1e8..54e5c97 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -419,7 +419,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>  	adev->driver_data = pdata;
>  	pdev = acpi_create_platform_device(adev);
>  	if (!IS_ERR_OR_NULL(pdev)) {
> -		device_enable_async_suspend(&pdev->dev);
> +		device_disable_async_suspend(&pdev->dev);

Removing the "enable" line should be sufficient for that.  Isn't it?

>  		return 1;
>  	}
>  
> -- 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Mika Westerberg]

On Tue, Sep 09, 2014 at 01:36:48PM +0800, Fu, Zhonghui wrote:
> >From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> Date: Tue, 9 Sep 2014 13:02:25 +0800
> Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices
> 
> LPSS devices must suspend/resume in fixed order. Or some LPSS devices
> will hang during the transition to ACPI_STATE_D0 state.

In addition to the comment from Rafael, I would like to have more
details here why we must suspend/resume the LPSS devices in certain
order.

> 
> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> ---
>  drivers/acpi/acpi_lpss.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index fddc1e8..54e5c97 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -419,7 +419,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>  	adev->driver_data = pdata;
>  	pdev = acpi_create_platform_device(adev);
>  	if (!IS_ERR_OR_NULL(pdev)) {
> -		device_enable_async_suspend(&pdev->dev);
> +		device_disable_async_suspend(&pdev->dev);
>  		return 1;
>  	}
>  
> -- 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Fu, Zhonghui]

hi, Rafael

Sorry for late response.

Yes,Only removing the "enable" line is sufficient. I have verified this, and made a new patch as follows.

Thanks,
Zhonghui

>From 3c89c22c5b35ddbdcadecd391f2521a15ffc2f4f Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
Date: Sat, 13 Sep 2014 01:26:04 +0800
Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

LPSS devices must suspend/resume in fixed order. Or some LPSS devices
will hang during the transition to ACPI_STATE_D0 state.

Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index fddc1e8..b0ea767 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -419,7 +419,6 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 	adev->driver_data = pdata;
 	pdev = acpi_create_platform_device(adev);
 	if (!IS_ERR_OR_NULL(pdev)) {
-		device_enable_async_suspend(&pdev->dev);
 		return 1;
 	}
 
-- 1.7.1



On 2014/9/9 21:17, Rafael J. Wysocki wrote:
> On Tuesday, September 09, 2014 01:36:48 PM Fu, Zhonghui wrote:
>> From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
>> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> Date: Tue, 9 Sep 2014 13:02:25 +0800
>> Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices
>>
>> LPSS devices must suspend/resume in fixed order. Or some LPSS devices
>> will hang during the transition to ACPI_STATE_D0 state.
>>
>> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
>> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> ---
>>  drivers/acpi/acpi_lpss.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index fddc1e8..54e5c97 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -419,7 +419,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>>  	adev->driver_data = pdata;
>>  	pdev = acpi_create_platform_device(adev);
>>  	if (!IS_ERR_OR_NULL(pdev)) {
>> -		device_enable_async_suspend(&pdev->dev);
>> +		device_disable_async_suspend(&pdev->dev);
> Removing the "enable" line should be sufficient for that.  Isn't it?
>
>>  		return 1;
>>  	}
>>  
>> -- 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Fu, Zhonghui]

On 2014/9/10 15:50, Mika Westerberg wrote:
> On Tue, Sep 09, 2014 at 01:36:48PM +0800, Fu, Zhonghui wrote:
>> >From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
>> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> Date: Tue, 9 Sep 2014 13:02:25 +0800
>> Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices
>>
>> LPSS devices must suspend/resume in fixed order. Or some LPSS devices
>> will hang during the transition to ACPI_STATE_D0 state.
> In addition to the comment from Rafael, I would like to have more
> details here why we must suspend/resume the LPSS devices in certain
> order.
Sorry for late response.

After the patch "ACPI / platform / LPSS: Enable async suspend/resume of LPSS devices(commit ID: 8ce62f85a81f57e86bc120ab690facc612223188)" was merged into upstream kernel, some LPSS devices(UART, I2C) frequently hang during resume. So, my patch is actually reverting that patch.


>
>> Signed-off-by: Li Aubrey <aubrey.li@linux.intel.com>
>> Signed-off-by: Fu Zhonghui <zhonghui.fu@linux.intel.com>
>> ---
>>  drivers/acpi/acpi_lpss.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index fddc1e8..54e5c97 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -419,7 +419,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>>  	adev->driver_data = pdata;
>>  	pdev = acpi_create_platform_device(adev);
>>  	if (!IS_ERR_OR_NULL(pdev)) {
>> -		device_enable_async_suspend(&pdev->dev);
>> +		device_disable_async_suspend(&pdev->dev);
>>  		return 1;
>>  	}
>>  
>> -- 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Saturday, September 13, 2014 01:53:55 AM Fu, Zhonghui wrote:
> 
> On 2014/9/10 15:50, Mika Westerberg wrote:
> > On Tue, Sep 09, 2014 at 01:36:48PM +0800, Fu, Zhonghui wrote:
> >> >From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
> >> From: Fu Zhonghui <zhonghui.fu@linux.intel.com>
> >> Date: Tue, 9 Sep 2014 13:02:25 +0800
> >> Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices
> >>
> >> LPSS devices must suspend/resume in fixed order. Or some LPSS devices
> >> will hang during the transition to ACPI_STATE_D0 state.
> > In addition to the comment from Rafael, I would like to have more
> > details here why we must suspend/resume the LPSS devices in certain
> > order.
> Sorry for late response.
> 
> After the patch "ACPI / platform / LPSS: Enable async suspend/resume of LPSS devices(commit ID: 8ce62f85a81f57e86bc120ab690facc612223188)" was merged into upstream kernel, some LPSS devices(UART, I2C) frequently hang during resume. So, my patch is actually reverting that patch.

I suppose the Mika's question was more about whether or not you were able
to identify the exact reason why the LPSS devices in question stopped
resuming correctly after the above change.

Also, your patch is not actually revering the whole commit mentioned above, as
it did a couple of things more.  You're simply restoring the previous behavior,
which doesn't mean "reverting".

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
> 

[cut]

> >>>
> >>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
> >> I don't know the exact reason why this particular read hangs, but this means
> >> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
> >> altogether, perhaps we can serialize their acpi_dev_resume_early()?
> >>
> >> Rafael
> > Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
> >
> > Thanks,
> > Zhonghui
> Hi, Rafael
> 
> Could you please confirm my understanding?

This is not what I meant.

Since we have a PM domain for the LPSS devices already, why don't we add an
internal lock to that PM domain and acquire it over executing either
acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
resume) for all of them?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Wednesday, September 24, 2014 11:19:22 PM Fu, Zhonghui wrote:
> This is a multi-part message in MIME format.
> --------------040808000309050202010005
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 7bit
> 
> 
> On 2014/9/23 7:17, Rafael J. Wysocki wrote:
> > On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
> > [cut]
> >
> >>>>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
> >>>> I don't know the exact reason why this particular read hangs, but this means
> >>>> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
> >>>> altogether, perhaps we can serialize their acpi_dev_resume_early()?
> >>>>
> >>>> Rafael
> >>> Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
> >>>
> >>> Thanks,
> >>> Zhonghui
> >> Hi, Rafael
> >>
> >> Could you please confirm my understanding?
> > This is not what I meant.
> >
> > Since we have a PM domain for the LPSS devices already, why don't we add an
> > internal lock to that PM domain and acquire it over executing either
> > acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
> > resume) for all of them?
> I seem find the root cause of this issue. Because this "hang" issue is occurred on ASUS T100(Baytrail-T platform), so I checked its DSDT and found that URT and I2C controllers depend on(_DEP) PEPD device(description in Windows is "power engine plug-in"). That is, URT and I2C controllers can not transition to ACPI_STATE_D0 state until PEPD device has completed this transition during resuming. But, the ACPI subsystem in the 3.16 kernel doesn't support "_DEP" feature. So, if enabling async suspend/resume for LPSS devices, their "_DEP" relationship with PEPD device will be broken and incur "hang" during the transition to ACPI_STATE_D0, please see the following code, it is from dpm_resume_early function in drivers/base/power/main.c file:
> 
> list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
>                 reinit_completion(&dev->power.completion);
>                 if (is_async(dev)) {
>                         get_device(dev);
>                         async_schedule(async_resume_early, dev);
>                 }
>         }
> 
>         while (!list_empty(&dpm_late_early_list)) {
>                 dev = to_device(dpm_late_early_list.next);
>                 get_device(dev);
>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
>                 mutex_unlock(&dpm_list_mtx);
> 
>                 if (!is_async(dev)) {    // PEPD is not configured as async device now.
>                         int error;
> 
>                         error = device_resume_early(dev, state, false);
>                         if (error) {
>                                 suspend_stats.failed_resume_early++;
>                                 dpm_save_failed_step(SUSPEND_RESUME_EARLY);
>                                 dpm_save_failed_dev(dev_name(dev));
>                                 pm_dev_err(dev, state, " early", error);
>                         }
>                 }
>                 mutex_lock(&dpm_list_mtx);
>                 put_device(dev);
>         }
> 
> 
> Based on the above analysis,I move the resume_early operation of PEPD device to head of dpm_resume_early function and "hang" did not occur any more during resuming(I tested this 10 times).
> 
> If disabling async suspend/resume for LPSS devices, PEPD device will be prior to UART and I2C controllers in dpm_late_early_list list and the "_DEP" relationship can be kept. Maybe,the "_DEP" ACPI feature will be supported in future kernel, so, I think simply disabling async suspend/resume for LPSS devices is a acceptable workaround now, and need not add new mechanism to deal with this issue.
> 
> BTW, I will take two week's leave and can't reply email during this time. Sorry.

OK, thanks for the analysis.  In that case we really may be better off by
disabling the runtime PM of LPSS devices for now until we figure out how this
can be addressed properly.

Rafael

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Li, Aubrey]

On 2014/9/25 4:32, Rafael J. Wysocki wrote:
> On Wednesday, September 24, 2014 11:19:22 PM Fu, Zhonghui wrote:
>> This is a multi-part message in MIME format.
>> --------------040808000309050202010005
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 7bit
>>
>>
>> On 2014/9/23 7:17, Rafael J. Wysocki wrote:
>>> On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
>>> [cut]
>>>
>>>>>>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
>>>>>> I don't know the exact reason why this particular read hangs, but this means
>>>>>> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
>>>>>> altogether, perhaps we can serialize their acpi_dev_resume_early()?
>>>>>>
>>>>>> Rafael
>>>>> Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
>>>>>
>>>>> Thanks,
>>>>> Zhonghui
>>>> Hi, Rafael
>>>>
>>>> Could you please confirm my understanding?
>>> This is not what I meant.
>>>
>>> Since we have a PM domain for the LPSS devices already, why don't we add an
>>> internal lock to that PM domain and acquire it over executing either
>>> acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
>>> resume) for all of them?
>> I seem find the root cause of this issue. Because this "hang" issue is occurred on ASUS T100(Baytrail-T platform), so I checked its DSDT and found that URT and I2C controllers depend on(_DEP) PEPD device(description in Windows is "power engine plug-in"). That is, URT and I2C controllers can not transition to ACPI_STATE_D0 state until PEPD device has completed this transition during resuming. But, the ACPI subsystem in the 3.16 kernel doesn't support "_DEP" feature. So, if enabling async suspend/resume for LPSS devices, their "_DEP" relationship with PEPD device will be broken and incur "hang" during the transition to ACPI_STATE_D0, please see the following code, it is from dpm_resume_early function in drivers/base/power/main.c file:
>>
>> list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
>>                 reinit_completion(&dev->power.completion);
>>                 if (is_async(dev)) {
>>                         get_device(dev);
>>                         async_schedule(async_resume_early, dev);
>>                 }
>>         }
>>
>>         while (!list_empty(&dpm_late_early_list)) {
>>                 dev = to_device(dpm_late_early_list.next);
>>                 get_device(dev);
>>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
>>                 mutex_unlock(&dpm_list_mtx);
>>
>>                 if (!is_async(dev)) {    // PEPD is not configured as async device now.
>>                         int error;
>>
>>                         error = device_resume_early(dev, state, false);
>>                         if (error) {
>>                                 suspend_stats.failed_resume_early++;
>>                                 dpm_save_failed_step(SUSPEND_RESUME_EARLY);
>>                                 dpm_save_failed_dev(dev_name(dev));
>>                                 pm_dev_err(dev, state, " early", error);
>>                         }
>>                 }
>>                 mutex_lock(&dpm_list_mtx);
>>                 put_device(dev);
>>         }
>>
>>
>> Based on the above analysis,I move the resume_early operation of PEPD device to head of dpm_resume_early function and "hang" did not occur any more during resuming(I tested this 10 times).
>>
>> If disabling async suspend/resume for LPSS devices, PEPD device will be prior to UART and I2C controllers in dpm_late_early_list list and the "_DEP" relationship can be kept. Maybe,the "_DEP" ACPI feature will be supported in future kernel, so, I think simply disabling async suspend/resume for LPSS devices is a acceptable workaround now, and need not add new mechanism to deal with this issue.
>>
>> BTW, I will take two week's leave and can't reply email during this time. Sorry.
> 
> OK, thanks for the analysis.  In that case we really may be better off by
> disabling the runtime PM of LPSS devices for now until we figure out how this
> can be addressed properly.

Please let me know if the patch need to be refined, I can do it before
October 1st, then one-week Chinese National holiday.

Besides this patch, we leave the non-LPSS devices as async
suspend/resume, the risk is unknown. I wonder if we need to make
pm_async parameter configurable thru kernel command line to make android
userspace happy?

Thanks,
-Aubrey

> 
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Thursday, September 25, 2014 10:07:44 AM Li, Aubrey wrote:
> On 2014/9/25 4:32, Rafael J. Wysocki wrote:
> > On Wednesday, September 24, 2014 11:19:22 PM Fu, Zhonghui wrote:
> >> This is a multi-part message in MIME format.
> >> --------------040808000309050202010005
> >> Content-Type: text/plain; charset=UTF-8
> >> Content-Transfer-Encoding: 7bit
> >>
> >>
> >> On 2014/9/23 7:17, Rafael J. Wysocki wrote:
> >>> On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
> >>> [cut]
> >>>
> >>>>>>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
> >>>>>> I don't know the exact reason why this particular read hangs, but this means
> >>>>>> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
> >>>>>> altogether, perhaps we can serialize their acpi_dev_resume_early()?
> >>>>>>
> >>>>>> Rafael
> >>>>> Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
> >>>>>
> >>>>> Thanks,
> >>>>> Zhonghui
> >>>> Hi, Rafael
> >>>>
> >>>> Could you please confirm my understanding?
> >>> This is not what I meant.
> >>>
> >>> Since we have a PM domain for the LPSS devices already, why don't we add an
> >>> internal lock to that PM domain and acquire it over executing either
> >>> acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
> >>> resume) for all of them?
> >> I seem find the root cause of this issue. Because this "hang" issue is occurred on ASUS T100(Baytrail-T platform), so I checked its DSDT and found that URT and I2C controllers depend on(_DEP) PEPD device(description in Windows is "power engine plug-in"). That is, URT and I2C controllers can not transition to ACPI_STATE_D0 state until PEPD device has completed this transition during resuming. But, the ACPI subsystem in the 3.16 kernel doesn't support "_DEP" feature. So, if enabling async suspend/resume for LPSS devices, their "_DEP" relationship with PEPD device will be broken and incur "hang" during the transition to ACPI_STATE_D0, please see the following code, it is from dpm_resume_early function in drivers/base/power/main.c file:
> >>
> >> list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
> >>                 reinit_completion(&dev->power.completion);
> >>                 if (is_async(dev)) {
> >>                         get_device(dev);
> >>                         async_schedule(async_resume_early, dev);
> >>                 }
> >>         }
> >>
> >>         while (!list_empty(&dpm_late_early_list)) {
> >>                 dev = to_device(dpm_late_early_list.next);
> >>                 get_device(dev);
> >>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
> >>                 mutex_unlock(&dpm_list_mtx);
> >>
> >>                 if (!is_async(dev)) {    // PEPD is not configured as async device now.
> >>                         int error;
> >>
> >>                         error = device_resume_early(dev, state, false);
> >>                         if (error) {
> >>                                 suspend_stats.failed_resume_early++;
> >>                                 dpm_save_failed_step(SUSPEND_RESUME_EARLY);
> >>                                 dpm_save_failed_dev(dev_name(dev));
> >>                                 pm_dev_err(dev, state, " early", error);
> >>                         }
> >>                 }
> >>                 mutex_lock(&dpm_list_mtx);
> >>                 put_device(dev);
> >>         }
> >>
> >>
> >> Based on the above analysis,I move the resume_early operation of PEPD device to head of dpm_resume_early function and "hang" did not occur any more during resuming(I tested this 10 times).
> >>
> >> If disabling async suspend/resume for LPSS devices, PEPD device will be prior to UART and I2C controllers in dpm_late_early_list list and the "_DEP" relationship can be kept. Maybe,the "_DEP" ACPI feature will be supported in future kernel, so, I think simply disabling async suspend/resume for LPSS devices is a acceptable workaround now, and need not add new mechanism to deal with this issue.
> >>
> >> BTW, I will take two week's leave and can't reply email during this time. Sorry.
> > 
> > OK, thanks for the analysis.  In that case we really may be better off by
> > disabling the runtime PM of LPSS devices for now until we figure out how this
> > can be addressed properly.
> 
> Please let me know if the patch need to be refined, I can do it before
> October 1st, then one-week Chinese National holiday.

The patch is fine.  In fact, I'm going to push it to Linus shortly.

> Besides this patch, we leave the non-LPSS devices as async
> suspend/resume, the risk is unknown.

No, we don't in general.  That is an opt-in, usually on a per-subsystem basis.

> I wonder if we need to make
> pm_async parameter configurable thru kernel command line to make android
> userspace happy?

There is a sysfs switch for disabling async suspend/resume (/sys/power/pm_async).
That has to suffice.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Li, Aubrey]

On 2014/9/26 4:08, Rafael J. Wysocki wrote:
> On Thursday, September 25, 2014 10:07:44 AM Li, Aubrey wrote:
>> On 2014/9/25 4:32, Rafael J. Wysocki wrote:
>>> On Wednesday, September 24, 2014 11:19:22 PM Fu, Zhonghui wrote:
>>>> This is a multi-part message in MIME format.
>>>> --------------040808000309050202010005
>>>> Content-Type: text/plain; charset=UTF-8
>>>> Content-Transfer-Encoding: 7bit
>>>>
>>>>
>>>> On 2014/9/23 7:17, Rafael J. Wysocki wrote:
>>>>> On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
>>>>> [cut]
>>>>>
>>>>>>>>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
>>>>>>>> I don't know the exact reason why this particular read hangs, but this means
>>>>>>>> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
>>>>>>>> altogether, perhaps we can serialize their acpi_dev_resume_early()?
>>>>>>>>
>>>>>>>> Rafael
>>>>>>> Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Zhonghui
>>>>>> Hi, Rafael
>>>>>>
>>>>>> Could you please confirm my understanding?
>>>>> This is not what I meant.
>>>>>
>>>>> Since we have a PM domain for the LPSS devices already, why don't we add an
>>>>> internal lock to that PM domain and acquire it over executing either
>>>>> acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
>>>>> resume) for all of them?
>>>> I seem find the root cause of this issue. Because this "hang" issue is occurred on ASUS T100(Baytrail-T platform), so I checked its DSDT and found that URT and I2C controllers depend on(_DEP) PEPD device(description in Windows is "power engine plug-in"). That is, URT and I2C controllers can not transition to ACPI_STATE_D0 state until PEPD device has completed this transition during resuming. But, the ACPI subsystem in the 3.16 kernel doesn't support "_DEP" feature. So, if enabling async suspend/resume for LPSS devices, their "_DEP" relationship with PEPD device will be broken and incur "hang" during the transition to ACPI_STATE_D0, please see the following code, it is from dpm_resume_early function in drivers/base/power/main.c file:
>>>>
>>>> list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
>>>>                 reinit_completion(&dev->power.completion);
>>>>                 if (is_async(dev)) {
>>>>                         get_device(dev);
>>>>                         async_schedule(async_resume_early, dev);
>>>>                 }
>>>>         }
>>>>
>>>>         while (!list_empty(&dpm_late_early_list)) {
>>>>                 dev = to_device(dpm_late_early_list.next);
>>>>                 get_device(dev);
>>>>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
>>>>                 mutex_unlock(&dpm_list_mtx);
>>>>
>>>>                 if (!is_async(dev)) {    // PEPD is not configured as async device now.
>>>>                         int error;
>>>>
>>>>                         error = device_resume_early(dev, state, false);
>>>>                         if (error) {
>>>>                                 suspend_stats.failed_resume_early++;
>>>>                                 dpm_save_failed_step(SUSPEND_RESUME_EARLY);
>>>>                                 dpm_save_failed_dev(dev_name(dev));
>>>>                                 pm_dev_err(dev, state, " early", error);
>>>>                         }
>>>>                 }
>>>>                 mutex_lock(&dpm_list_mtx);
>>>>                 put_device(dev);
>>>>         }
>>>>
>>>>
>>>> Based on the above analysis,I move the resume_early operation of PEPD device to head of dpm_resume_early function and "hang" did not occur any more during resuming(I tested this 10 times).
>>>>
>>>> If disabling async suspend/resume for LPSS devices, PEPD device will be prior to UART and I2C controllers in dpm_late_early_list list and the "_DEP" relationship can be kept. Maybe,the "_DEP" ACPI feature will be supported in future kernel, so, I think simply disabling async suspend/resume for LPSS devices is a acceptable workaround now, and need not add new mechanism to deal with this issue.
>>>>
>>>> BTW, I will take two week's leave and can't reply email during this time. Sorry.
>>>
>>> OK, thanks for the analysis.  In that case we really may be better off by
>>> disabling the runtime PM of LPSS devices for now until we figure out how this
>>> can be addressed properly.
>>
>> Please let me know if the patch need to be refined, I can do it before
>> October 1st, then one-week Chinese National holiday.
> 
> The patch is fine.  In fact, I'm going to push it to Linus shortly.
> 
>> Besides this patch, we leave the non-LPSS devices as async
>> suspend/resume, the risk is unknown.
> 
> No, we don't in general.  That is an opt-in, usually on a per-subsystem basis.
> 
>> I wonder if we need to make
>> pm_async parameter configurable thru kernel command line to make android
>> userspace happy?
> 
> There is a sysfs switch for disabling async suspend/resume (/sys/power/pm_async).
> That has to suffice.
> 
Like what you did to pretend echo mem > /sys/power/state, it's hard to
visit sysfs switch from android UI, we want to disable async
suspend/resume from kernel command line, so that we can bypass this
feature after boot.

Thanks,
-Aubrey

Re: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

[Rafael J. Wysocki]

On Friday, September 26, 2014 11:54:42 AM Li, Aubrey wrote:
> On 2014/9/26 4:08, Rafael J. Wysocki wrote:
> > On Thursday, September 25, 2014 10:07:44 AM Li, Aubrey wrote:
> >> On 2014/9/25 4:32, Rafael J. Wysocki wrote:
> >>> On Wednesday, September 24, 2014 11:19:22 PM Fu, Zhonghui wrote:
> >>>> This is a multi-part message in MIME format.
> >>>> --------------040808000309050202010005
> >>>> Content-Type: text/plain; charset=UTF-8
> >>>> Content-Transfer-Encoding: 7bit
> >>>>
> >>>>
> >>>> On 2014/9/23 7:17, Rafael J. Wysocki wrote:
> >>>>> On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
> >>>>> [cut]
> >>>>>
> >>>>>>>>> This operation is reading data from Operation Region of one operand object in name space. I don't know the reason of hang at this point. Could you please give out some explanation about this?
> >>>>>>>> I don't know the exact reason why this particular read hangs, but this means
> >>>>>>>> that, perhaps, instead of disabling async suspend/resume for all LPSS devices
> >>>>>>>> altogether, perhaps we can serialize their acpi_dev_resume_early()?
> >>>>>>>>
> >>>>>>>> Rafael
> >>>>>>> Do you mean keeping other phases(prepare, suspend, suspend_late, suspend_noirq, resume_noirq, resume, complete) of suspend/resume asynchronous, and only serializing "resume_early" phase for all LPSS devices?
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Zhonghui
> >>>>>> Hi, Rafael
> >>>>>>
> >>>>>> Could you please confirm my understanding?
> >>>>> This is not what I meant.
> >>>>>
> >>>>> Since we have a PM domain for the LPSS devices already, why don't we add an
> >>>>> internal lock to that PM domain and acquire it over executing either
> >>>>> acpi_dev_suspend_late() (during suspend) or acpi_dev_resume_early() (during
> >>>>> resume) for all of them?
> >>>> I seem find the root cause of this issue. Because this "hang" issue is occurred on ASUS T100(Baytrail-T platform), so I checked its DSDT and found that URT and I2C controllers depend on(_DEP) PEPD device(description in Windows is "power engine plug-in"). That is, URT and I2C controllers can not transition to ACPI_STATE_D0 state until PEPD device has completed this transition during resuming. But, the ACPI subsystem in the 3.16 kernel doesn't support "_DEP" feature. So, if enabling async suspend/resume for LPSS devices, their "_DEP" relationship with PEPD device will be broken and incur "hang" during the transition to ACPI_STATE_D0, please see the following code, it is from dpm_resume_early function in drivers/base/power/main.c file:
> >>>>
> >>>> list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
> >>>>                 reinit_completion(&dev->power.completion);
> >>>>                 if (is_async(dev)) {
> >>>>                         get_device(dev);
> >>>>                         async_schedule(async_resume_early, dev);
> >>>>                 }
> >>>>         }
> >>>>
> >>>>         while (!list_empty(&dpm_late_early_list)) {
> >>>>                 dev = to_device(dpm_late_early_list.next);
> >>>>                 get_device(dev);
> >>>>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
> >>>>                 mutex_unlock(&dpm_list_mtx);
> >>>>
> >>>>                 if (!is_async(dev)) {    // PEPD is not configured as async device now.
> >>>>                         int error;
> >>>>
> >>>>                         error = device_resume_early(dev, state, false);
> >>>>                         if (error) {
> >>>>                                 suspend_stats.failed_resume_early++;
> >>>>                                 dpm_save_failed_step(SUSPEND_RESUME_EARLY);
> >>>>                                 dpm_save_failed_dev(dev_name(dev));
> >>>>                                 pm_dev_err(dev, state, " early", error);
> >>>>                         }
> >>>>                 }
> >>>>                 mutex_lock(&dpm_list_mtx);
> >>>>                 put_device(dev);
> >>>>         }
> >>>>
> >>>>
> >>>> Based on the above analysis,I move the resume_early operation of PEPD device to head of dpm_resume_early function and "hang" did not occur any more during resuming(I tested this 10 times).
> >>>>
> >>>> If disabling async suspend/resume for LPSS devices, PEPD device will be prior to UART and I2C controllers in dpm_late_early_list list and the "_DEP" relationship can be kept. Maybe,the "_DEP" ACPI feature will be supported in future kernel, so, I think simply disabling async suspend/resume for LPSS devices is a acceptable workaround now, and need not add new mechanism to deal with this issue.
> >>>>
> >>>> BTW, I will take two week's leave and can't reply email during this time. Sorry.
> >>>
> >>> OK, thanks for the analysis.  In that case we really may be better off by
> >>> disabling the runtime PM of LPSS devices for now until we figure out how this
> >>> can be addressed properly.
> >>
> >> Please let me know if the patch need to be refined, I can do it before
> >> October 1st, then one-week Chinese National holiday.
> > 
> > The patch is fine.  In fact, I'm going to push it to Linus shortly.
> > 
> >> Besides this patch, we leave the non-LPSS devices as async
> >> suspend/resume, the risk is unknown.
> > 
> > No, we don't in general.  That is an opt-in, usually on a per-subsystem basis.
> > 
> >> I wonder if we need to make
> >> pm_async parameter configurable thru kernel command line to make android
> >> userspace happy?
> > 
> > There is a sysfs switch for disabling async suspend/resume (/sys/power/pm_async).
> > That has to suffice.
> > 
> Like what you did to pretend echo mem > /sys/power/state,

That was supposed to be an exception.

> it's hard to
> visit sysfs switch from android UI, we want to disable async
> suspend/resume from kernel command line, so that we can bypass this
> feature after boot.

Please feel free to submit a patch adding a command line switch to set the
initial value of /sys/power/pm_async.  Maybe people won't complain about it.

Rafael