ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

PATCH In review Fu, Zhonghui 4 participants 12 messages Sep 9, 2014 – Sep 26, 2014 3 messages not in the archive ACPI

Share & tools

Rafael J. Wysocki

2014-09-09 12:57 UTC #1 lore raw

Quoted 19 lines · Zhonghui

From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
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 <[email protected]>
Signed-off-by Fu Zhonghui <[email protected]>
---
 drivers/acpi/acpi_lpss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/acpi_lpss.c +0 -0
index fddc1e8..54e5c97 100644

drivers/acpi/acpi_lpss.c static int acpi_lpss_create_device(struct acpi_device *adev, · line 422

Quoted 5 lines

drivers/acpi/acpi_lpss.c +1 -1

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

Quoted 9 lines

drivers/acpi/acpi_lpss.c +0 -2
 		return 1;
 	}
 
-- 1.7.1
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Fu, Zhonghui

author

2014-09-12 17:40 UTC #2 lore raw

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

Quoted 1 line

From 3c89c22c5b35ddbdcadecd391f2521a15ffc2f4f Mon Sep 17 00:00:00 2001

From: Fu Zhonghui <[email protected]>
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 <[email protected]>

Signed-off-by Fu Zhonghui <[email protected]>

---
 drivers/acpi/acpi_lpss.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/acpi/acpi_lpss.c +0 -1

index fddc1e8..b0ea767 100644
@@ -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

Quoted 36 lines · Rafael J. Wysocki

Quoted 19 lines · Zhonghui
From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
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 <[email protected]>
Signed-off-by Fu Zhonghui <[email protected]>
---
 drivers/acpi/acpi_lpss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/acpi_lpss.c +0 -0
index fddc1e8..54e5c97 100644
drivers/acpi/acpi_lpss.c static int acpi_lpss_create_device(struct acpi_device *adev, · line 422
Quoted 5 lines
drivers/acpi/acpi_lpss.c +1 -1
@@ -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?
Quoted 9 lines
drivers/acpi/acpi_lpss.c +0 -2
 		return 1;
 	}
 
-- 1.7.1
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mika Westerberg

2014-09-10 07:50 UTC #3 lore raw

In reply to Fu, Zhonghui

Quoted 7 lines · Zhonghui

Quoted 1 line
From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
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.

Quoted 26 lines

Signed-off-by Li Aubrey <[email protected]>
Signed-off-by Fu Zhonghui <[email protected]>
---
 drivers/acpi/acpi_lpss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/acpi_lpss.c +1 -3
index fddc1e8..54e5c97 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fu, Zhonghui

author

2014-09-12 17:54 UTC #4 lore raw

Quoted 11 lines · Mika Westerberg

Quoted 7 lines · Zhonghui
Quoted 1 line
From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
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.

Quoted 31 lines

Quoted 23 lines
Signed-off-by Li Aubrey <[email protected]>
Signed-off-by Fu Zhonghui <[email protected]>
---
 drivers/acpi/acpi_lpss.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/acpi/acpi_lpss.c +1 -3
index fddc1e8..54e5c97 100644
@@ -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
 
--
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rafael J. Wysocki

2014-09-14 16:33 UTC #5 lore raw

Quoted 8 lines · Zhonghui

Quoted 6 lines · Mika Westerberg
Quoted 5 lines · Zhonghui
Quoted 1 line
From 6deb00230f5df68da3ca7490402a0c537bf386bb Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
Date: Tue, 9 Sep 2014 13:02:25 +0800
Subject: [PATCH] ACPI / platform / LPSS: disable async suspend/resume of LPSS devices

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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".

Signature · 2 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rafael J. Wysocki

2014-09-22 22:57 UTC #8 lore raw

Quoted 1 line · Zhonghui

[cut]

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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()?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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?

Signature · 2 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Rafael J. Wysocki

2014-09-24 20:13 UTC #10 lore raw

Quoted 61 lines · Zhonghui

This is a multi-part message in MIME format.
--------------040808000309050202010005
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Quoted 15 lines · Rafael J. Wysocki
On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
[cut]
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
Quoted 1 line
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);
        }

Quoted 5 lines

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

Li, Aubrey

2014-09-25 02:07 UTC #11 lore raw

Quoted 64 lines · Rafael J. Wysocki

Quoted 63 lines · Zhonghui
This is a multi-part message in MIME format.
--------------040808000309050202010005
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Quoted 15 lines · Rafael J. Wysocki
On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
[cut]
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
Quoted 1 line
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).

Quoted 7 lines

Quoted 3 lines
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

Quoted 10 lines

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

Rafael J. Wysocki

2014-09-25 19:48 UTC #12 lore raw

Quoted 67 lines · Aubrey

Quoted 66 lines · Rafael J. Wysocki
Quoted 65 lines · Zhonghui
This is a multi-part message in MIME format.
--------------040808000309050202010005
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Quoted 15 lines · Rafael J. Wysocki
On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
[cut]
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Signature · 2 lines

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

Li, Aubrey

2014-09-26 03:54 UTC #13 lore raw

Quoted 82 lines · Rafael J. Wysocki

Quoted 67 lines · Aubrey
Quoted 66 lines · Rafael J. Wysocki
Quoted 65 lines · Zhonghui
This is a multi-part message in MIME format.
--------------040808000309050202010005
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Quoted 15 lines · Rafael J. Wysocki
On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
[cut]
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
Quoted 2 lines
Besides this patch, we leave the non-LPSS devices as async suspend/resume, the risk is unknown.

Quoted 9 lines

No, we don't in general. That is an opt-in, usually on a per-subsystem basis.
Quoted 3 lines
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

Rafael J. Wysocki

2014-09-26 13:46 UTC #14 lore raw

Quoted 85 lines · Aubrey

Quoted 84 lines · Rafael J. Wysocki
Quoted 67 lines · Aubrey
Quoted 66 lines · Rafael J. Wysocki
Quoted 65 lines · Zhonghui
This is a multi-part message in MIME format.
--------------040808000309050202010005
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Quoted 15 lines · Rafael J. Wysocki
On Monday, September 22, 2014 10:45:42 PM Fu, Zhonghui wrote:
[cut]
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 7 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
Quoted 2 lines
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.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
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.

Quoted 4 lines

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