[PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Fu, Zhonghui]

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

Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
---
 drivers/mmc/core/sdio.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index b91abed..6719b77 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -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

Re: [PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Fu, Zhonghui]

Hi,

Any comments are welcome.


Thanks,
Zhonghui

On 2015/7/30 15:40, Fu, Zhonghui wrote:
> Enable SDIO card and function device to suspend/resume asynchronously.
> This can improve system suspend/resume speed.
>
> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
> ---
>  drivers/mmc/core/sdio.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index b91abed..6719b77 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -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
>

Re: [PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Adrian Hunter]

On 17/08/15 06:26, Fu, Zhonghui wrote:
> 
> Hi,
> 
> Any comments are welcome.
> 
> 
> Thanks,
> Zhonghui
> 
> On 2015/7/30 15:40, Fu, Zhonghui wrote:
>> 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?

>>
>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>> ---
>>  drivers/mmc/core/sdio.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index b91abed..6719b77 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -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
>>
> 
> 
> 

Re: [PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Fu, Zhonghui]

On 2015/8/17 14:48, Adrian Hunter wrote:
> On 17/08/15 06:26, Fu, Zhonghui wrote:
>> Hi,
>>
>> Any comments are welcome.
>>
>>
>> Thanks,
>> Zhonghui
>>
>> On 2015/7/30 15:40, Fu, Zhonghui wrote:
>>> 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?

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  
>
>>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>>> ---
>>>  drivers/mmc/core/sdio.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>> index b91abed..6719b77 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -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
>>>
>>
>>
> --
> 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] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Adrian Hunter]

On 21/09/15 08:29, Fu, Zhonghui wrote:
> 
> 
> On 2015/8/24 15:07, Fu, Zhonghui wrote:
>>
>> On 2015/8/17 14:48, Adrian Hunter wrote:
>>> On 17/08/15 06:26, Fu, Zhonghui wrote:
>>>> Hi,
>>>>
>>>> Any comments are welcome.
>>>>
>>>>
>>>> Thanks,
>>>> Zhonghui
>>>>
>>>> On 2015/7/30 15:40, Fu, Zhonghui wrote:
>>>>> 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?
>> 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
> 
> 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?

> 
> 
> Thanks,
> Zhonghui
>>
>>
>> Thanks,
>> Zhonghui  
>>>>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>>>>> ---
>>>>>  drivers/mmc/core/sdio.c |    4 ++++
>>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>> index b91abed..6719b77 100644
>>>>> --- a/drivers/mmc/core/sdio.c
>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>> @@ -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
>>>>>
>>>>
>>> --
>>> 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] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Adrian Hunter]

On 21/09/15 11:15, Adrian Hunter wrote:
> On 21/09/15 08:29, Fu, Zhonghui wrote:
>>
>>
>> On 2015/8/24 15:07, Fu, Zhonghui wrote:
>>>
>>> On 2015/8/17 14:48, Adrian Hunter wrote:
>>>> On 17/08/15 06:26, Fu, Zhonghui wrote:
>>>>> Hi,
>>>>>
>>>>> Any comments are welcome.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Zhonghui
>>>>>
>>>>> On 2015/7/30 15:40, Fu, Zhonghui wrote:
>>>>>> 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?
>>> 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
>>
>> 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?

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

> 
>>
>>
>> Thanks,
>> Zhonghui
>>>
>>>
>>> Thanks,
>>> Zhonghui  
>>>>>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/mmc/core/sdio.c |    4 ++++
>>>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>>> index b91abed..6719b77 100644
>>>>>> --- a/drivers/mmc/core/sdio.c
>>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>>> @@ -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
>>>>>>
>>>>>
>>>> --
>>>> 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: Re: [PATCH] MMC/SDIO: enable SDIO device to suspend/resume asynchronously

[Fu, Zhonghui]

On 9/21/2015 4:41 PM, Adrian Hunter wrote:
> On 21/09/15 11:15, Adrian Hunter wrote:
>> On 21/09/15 08:29, Fu, Zhonghui wrote:
>>>
>>> On 2015/8/24 15:07, Fu, Zhonghui wrote:
>>>> On 2015/8/17 14:48, Adrian Hunter wrote:
>>>>> On 17/08/15 06:26, Fu, Zhonghui wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Any comments are welcome.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Zhonghui
>>>>>>
>>>>>> On 2015/7/30 15:40, Fu, Zhonghui wrote:
>>>>>>> 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?
>>>> 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
>>>
>>> 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?
> 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
>
>>>
>>> Thanks,
>>> Zhonghui
>>>>
>>>> Thanks,
>>>> Zhonghui  
>>>>>>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>>>>>>> ---
>>>>>>>  drivers/mmc/core/sdio.c |    4 ++++
>>>>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>>>>>>> index b91abed..6719b77 100644
>>>>>>> --- a/drivers/mmc/core/sdio.c
>>>>>>> +++ b/drivers/mmc/core/sdio.c
>>>>>>> @@ -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
>>>>>>>
>>>>> --
>>>>> 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/
>>
> --
> 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/