MMC/SDIO: enable SDIO device to suspend/resume asynchronously

PATCH In review Fu, Zhonghui 2 participants 7 messages Jul 30, 2015 – Nov 15, 2015 1 message not in the archive MMC, SD and SDIO

lore

Share & tools

lore

Fu, Zhonghui

author

2015-08-17 03:26 UTC #1 lore raw

Hi,

Any comments are welcome.

Thanks,
Zhonghui

Quoted 32 lines · Zhonghui

Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.

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

---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/mmc/core/sdio.c +4 -1

index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.

@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
 	/*
-- 1.7.1

Adrian Hunter

2015-08-17 06:51 UTC #2 lore raw

Quoted 12 lines · Zhonghui

Hi,
Any comments are welcome.
Thanks,
Zhonghui
Quoted 2 lines · Zhonghui
Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.

For me, it needs more explanation.

For example, why is this worth doing? Can you give an example where it does significantly improve suspend/resume speed? Are there any cases where it could be worse?

Why is it safe? Presumably it is safe if there are no dependencies on the device outside the device hierarchy. Is that so? Are there any other potential pitfalls to enabling async_suspend?

Quoted 33 lines

Quoted 30 lines

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

---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/mmc/core/sdio.c +4 -1

index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.

@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
 	/*
-- 1.7.1

Fu, Zhonghui

author

2015-08-24 07:07 UTC #3 lore raw

Quoted 14 lines · Adrian Hunter

Quoted 11 lines · Zhonghui
Hi,
Any comments are welcome.
Thanks,
Zhonghui
Quoted 2 lines · Zhonghui
Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.
For me, it needs more explanation.

Quoted 7 lines

For example, why is this worth doing? Can you give an example where it does significantly improve suspend/resume speed? Are there any cases where it could be worse?
Why is it safe? Presumably it is safe if there are no dependencies on the device outside the device hierarchy. Is that so? Are there any other potential pitfalls to enabling async_suspend?

Now, PM core support asynchronous device suspend/resume mode. If one device has been set to support asynchronous PM mode, it's suspend/resume operation can be performed in a separate kernel thread and take advantage of multicore feature to improve overall system suspend/resume speed. The worst case is that all device suspend/resume threads will be scheduled to the same CPU, it hardly occur.

PM core ensure all the suspend/resume dependency related to one device. Actually, async suspend/resume mode is one feature of PM core, every device subsystem may use it or not use it. Once one device subsystem choose to use this feature, its safety is up to PM core as long as device subsystem has initialized fully this device.

Thanks,
Zhonghui

Quoted 37 lines

Quoted 27 lines
Quoted 27 lines
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/mmc/core/sdio.c +4 -0
index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
Quoted 5 lines
drivers/mmc/core/sdio.c int mmc_attach_sdio(struct mmc_host *host)
Quoted 3 lines
drivers/mmc/core/sdio.c +0 -1
 	/*
-- 1.7.1
--
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/

Adrian Hunter

2015-09-21 08:15 UTC #5 lore raw

In reply to a message not in the archive

Quoted 42 lines · Zhonghui

Quoted 20 lines · Zhonghui
Quoted 18 lines · Adrian Hunter
Quoted 11 lines · Zhonghui
Hi,
Any comments are welcome.
Thanks,
Zhonghui
Quoted 2 lines · Zhonghui
Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.
For me, it needs more explanation.
For example, why is this worth doing? Can you give an example where it does significantly improve suspend/resume speed? Are there any cases where it could be worse?
Quoted 6 lines
Quoted 3 lines
Why is it safe? Presumably it is safe if there are no dependencies on the device outside the device hierarchy. Is that so? Are there any other potential pitfalls to enabling async_suspend?
Now, PM core support asynchronous device suspend/resume mode. If one device has been set to support asynchronous PM mode, it's suspend/resume operation can be performed in a separate kernel thread and take advantage of multicore feature to improve overall system suspend/resume speed. The worst case is that all device suspend/resume threads will be scheduled to the same CPU, it hardly occur.
PM core ensure all the suspend/resume dependency related to one device. Actually, async suspend/resume mode is one feature of PM core, every device subsystem may use it or not use it. Once one device subsystem choose to use this feature, its safety is up to PM core as long as device subsystem has initialized fully this device.
The original suspend time is 1645ms and resume time is 940ms on ASUS T100TA machine. After enabling "wiphy device", "SDIO device", "mmc host" and "sdhci-acpi device" to suspend/resume asynchronously, the suspend time is 1096ms and resume time is 908ms. The test environment is listed as follows:
OS: Ubuntu 14.04
Kernel: mainline v4.1
Machine: ASUS T100TA(Baytrail-T platform)
Tool: analyze_suspend
“analyze_suspend.py –f –m freeze” to suspend system Press power button to resume system

Quoted 2 lines

I have resent this patch with updated commit message - "[PATCH v2] MMC/SDIO:
enable SDIO device to suspend/resume asynchronously".

It is really cool that you tested this. Thank you!

I am a bit baffled and bemused that you didn't put a summary of the testing and results in the commit message. Can you do that.

As I wrote, we are assuming that there are no dependencies on the device outside the device hierarchy. That is a reasonable assumption for an SDIO controller because it doesn't provide resources for other devices to use, except for the card itself which is a child device, and therefore a dependency that PM core knows about.

Does that make sense? If it does then shouldn't that explanation be added to the commit message too. i.e. this is why we think it is always going to work?

Quoted 44 lines

Thanks,
Zhonghui
Quoted 39 lines
Thanks,
Zhonghui
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/mmc/core/sdio.c +4 -0
index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
Quoted 4 lines
drivers/mmc/core/sdio.c int mmc_attach_sdio(struct mmc_host *host)
Quoted 3 lines
drivers/mmc/core/sdio.c +0 -1
 	/*
-- 1.7.1
--
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/

Adrian Hunter

2015-09-21 08:44 UTC #6 lore raw

Quoted 57 lines · Adrian Hunter

Quoted 42 lines · Zhonghui
Quoted 20 lines · Zhonghui
Quoted 18 lines · Adrian Hunter
Quoted 11 lines · Zhonghui
Hi,
Any comments are welcome.
Thanks,
Zhonghui
Quoted 2 lines · Zhonghui
Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.
For me, it needs more explanation.
For example, why is this worth doing? Can you give an example where it does significantly improve suspend/resume speed? Are there any cases where it could be worse?
Quoted 6 lines
Quoted 3 lines
Why is it safe? Presumably it is safe if there are no dependencies on the device outside the device hierarchy. Is that so? Are there any other potential pitfalls to enabling async_suspend?
Now, PM core support asynchronous device suspend/resume mode. If one device has been set to support asynchronous PM mode, it's suspend/resume operation can be performed in a separate kernel thread and take advantage of multicore feature to improve overall system suspend/resume speed. The worst case is that all device suspend/resume threads will be scheduled to the same CPU, it hardly occur.
PM core ensure all the suspend/resume dependency related to one device. Actually, async suspend/resume mode is one feature of PM core, every device subsystem may use it or not use it. Once one device subsystem choose to use this feature, its safety is up to PM core as long as device subsystem has initialized fully this device.
The original suspend time is 1645ms and resume time is 940ms on ASUS T100TA machine. After enabling "wiphy device", "SDIO device", "mmc host" and "sdhci-acpi device" to suspend/resume asynchronously, the suspend time is 1096ms and resume time is 908ms. The test environment is listed as follows:
OS: Ubuntu 14.04
Kernel: mainline v4.1
Machine: ASUS T100TA(Baytrail-T platform)
Tool: analyze_suspend
“analyze_suspend.py –f –m freeze” to suspend system Press power button to resume system
Quoted 2 lines
I have resent this patch with updated commit message - "[PATCH v2] MMC/SDIO:
enable SDIO device to suspend/resume asynchronously".
It is really cool that you tested this. Thank you!
I am a bit baffled and bemused that you didn't put a summary of the testing and results in the commit message. Can you do that.
As I wrote, we are assuming that there are no dependencies on the device outside the device hierarchy. That is a reasonable assumption for an SDIO controller because it doesn't provide resources for other devices to use, except for the card itself which is a child device, and therefore a dependency that PM core knows about.

Quoted 3 lines

Does that make sense? If it does then shouldn't that explanation be added to the commit message too. i.e. this is why we think it is always going to work?

Just realized this patch is for the card, not the host controller, so those last 2 paragraphs don't apply.

Quoted 47 lines

Quoted 45 lines
Thanks,
Zhonghui
Quoted 39 lines
Thanks,
Zhonghui
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/mmc/core/sdio.c +4 -0
index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
Quoted 4 lines
drivers/mmc/core/sdio.c int mmc_attach_sdio(struct mmc_host *host)
Quoted 3 lines
drivers/mmc/core/sdio.c +0 -1
 	/*
-- 1.7.1
--
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/

Fu, Zhonghui

author

2015-11-15 14:03 UTC #7 lore raw

Quoted 54 lines · Adrian Hunter

Quoted 53 lines · Adrian Hunter
Quoted 39 lines · Zhonghui
Quoted 19 lines · Zhonghui
Quoted 18 lines · Adrian Hunter
Quoted 11 lines · Zhonghui
Hi,
Any comments are welcome.
Thanks,
Zhonghui
Quoted 2 lines · Zhonghui
Enable SDIO card and function device to suspend/resume asynchronously. This can improve system suspend/resume speed.
For me, it needs more explanation.
For example, why is this worth doing? Can you give an example where it does significantly improve suspend/resume speed? Are there any cases where it could be worse?
Quoted 6 lines
Quoted 3 lines
Why is it safe? Presumably it is safe if there are no dependencies on the device outside the device hierarchy. Is that so? Are there any other potential pitfalls to enabling async_suspend?
Now, PM core support asynchronous device suspend/resume mode. If one device has been set to support asynchronous PM mode, it's suspend/resume operation can be performed in a separate kernel thread and take advantage of multicore feature to improve overall system suspend/resume speed. The worst case is that all device suspend/resume threads will be scheduled to the same CPU, it hardly occur.
PM core ensure all the suspend/resume dependency related to one device. Actually, async suspend/resume mode is one feature of PM core, every device subsystem may use it or not use it. Once one device subsystem choose to use this feature, its safety is up to PM core as long as device subsystem has initialized fully this device.
The original suspend time is 1645ms and resume time is 940ms on ASUS T100TA machine. After enabling "wiphy device", "SDIO device", "mmc host" and "sdhci-acpi device" to suspend/resume asynchronously, the suspend time is 1096ms and resume time is 908ms. The test environment is listed as follows:
OS: Ubuntu 14.04
Kernel: mainline v4.1
Machine: ASUS T100TA(Baytrail-T platform)
Tool: analyze_suspend
“analyze_suspend.py –f –m freeze” to suspend system Press power button to resume system
Quoted 2 lines
I have resent this patch with updated commit message - "[PATCH v2] MMC/SDIO:
enable SDIO device to suspend/resume asynchronously".
It is really cool that you tested this. Thank you!
I am a bit baffled and bemused that you didn't put a summary of the testing and results in the commit message. Can you do that.
As I wrote, we are assuming that there are no dependencies on the device outside the device hierarchy. That is a reasonable assumption for an SDIO controller because it doesn't provide resources for other devices to use, except for the card itself which is a child device, and therefore a dependency that PM core knows about.

Quoted 5 lines

Quoted 3 lines
Does that make sense? If it does then shouldn't that explanation be added to the commit message too. i.e. this is why we think it is always going to work?
Just realized this patch is for the card, not the host controller, so those last 2 paragraphs don't apply.

Sorry for replying this mail after long leave.

Thanks for your comments. I have added the test result into commit message and sent the latest version of this patch: "[PATCH v3] MMC/SDIO: enable SDIO device to suspend/resume asynchronously"

Thanks,
Zhonghui

Quoted 47 lines

Quoted 33 lines
Quoted 33 lines
Thanks,
Zhonghui
Quoted 29 lines
Thanks,
Zhonghui
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Signed-off-by Zhonghui Fu <[email protected]>
---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/mmc/core/sdio.c +4 -0
index b91abed..6719b77 100644
@@ -1106,6 +1106,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		pm_runtime_enable(&card->dev);
 	}
 
+	device_enable_async_suspend(&card->dev);
+
 	/*
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
@@ -1126,6 +1128,8 @@ int mmc_attach_sdio(struct mmc_host *host)
 		 */
 		if (host->caps & MMC_CAP_POWER_OFF_CARD)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
+
+		device_enable_async_suspend(&card->sdio_func[i]->dev);
 	}
 
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
drivers/mmc/core/sdio.c int mmc_attach_sdio(struct mmc_host *host)
Quoted 3 lines
drivers/mmc/core/sdio.c +0 -1
 	/*
-- 1.7.1
--
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/
--
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/