Re: [RFC PATCH] mmc: core: Invoke sdio func driver's PM callbacks from the sdio bus

[Aaron Lu <aaron.lu@intel.com>]

On 03/12/2014 11:44 AM, Dong, Chuanxiao wrote:
> Hi Aaron,
> 
> This patch is tested on Intel platform, and SDIO function driver's suspend/resume callback will only be called once, which fixed this issue. Previously, they can be called twice.
> 
> Here is the tested-by:
> 
> Tested-by: xiaoming wang <xiaoming.wang@intel.com>
> Tested-by: Chuanxiao Dong <chuanxiao.dong@intel.com>

Thanks a lot for the test!

-Aaron

> 
> Thanks
> Chuanxiao
> 
>> -----Original Message-----
>> From: Lu, Aaron
>> Sent: Wednesday, March 12, 2014 10:36 AM
>> To: Ulf Hansson; linux-mmc@vger.kernel.org; Chris Ball; Liu, Chuansheng; Dong,
>> Chuanxiao
>> Cc: linux-kernel@vger.kernel.org; NeilBrown; Rafael J. Wysocki
>> Subject: Re: [RFC PATCH] mmc: core: Invoke sdio func driver's PM callbacks from the
>> sdio bus
>>
>> Hi Chuansheng & Chuanxiao,
>>
>> Can you please help us testing this patch on your platform and let us know the test
>> result? Thanks.
>>
>> -Aaron
>>
>> On 02/28/2014 07:49 PM, Ulf Hansson wrote:
>>> The sdio func device is added to the driver model after the card
>>> device.
>>>
>>> This means the sdio func device will be suspend before the card device
>>> and thus resumed after. The consequence are the mmc core don't
>>> explicity need to protect itself from receiving sdio requests in
>>> suspended state. Instead that can be handled from the sdio bus, which
>>> is thus invokes the PM callbacks instead of old dummy function.
>>>
>>> In the case were the sdio func driver don't implement the PM callbacks
>>> the mmc core will in the early phase of system suspend, remove the
>>> card from the driver model and thus power off it.
>>>
>>> Cc: Aaron Lu <aaron.lu@intel.com>
>>> Cc: NeilBrown <neilb@suse.de>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>
>>> Note, this patch has only been compile tested. Would appreciate if
>>> some with SDIO and a sdio func driver could help out to test this.
>>> Especially the libertas driver would be nice.
>>>
>>> ---
>>>  drivers/mmc/core/sdio.c     |   45 ++++---------------------------------------
>>>  drivers/mmc/core/sdio_bus.c |   14 +-------------
>>>  2 files changed, 5 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index
>>> 4d721c6..9933e42 100644
>>> --- a/drivers/mmc/core/sdio.c
>>> +++ b/drivers/mmc/core/sdio.c
>>> @@ -943,40 +943,21 @@ static int mmc_sdio_pre_suspend(struct mmc_host
>> *host)
>>>   */
>>>  static int mmc_sdio_suspend(struct mmc_host *host)  {
>>> -	int i, err = 0;
>>> -
>>> -	for (i = 0; i < host->card->sdio_funcs; i++) {
>>> -		struct sdio_func *func = host->card->sdio_func[i];
>>> -		if (func && sdio_func_present(func) && func->dev.driver) {
>>> -			const struct dev_pm_ops *pmops = func->dev.driver->pm;
>>> -			err = pmops->suspend(&func->dev);
>>> -			if (err)
>>> -				break;
>>> -		}
>>> -	}
>>> -	while (err && --i >= 0) {
>>> -		struct sdio_func *func = host->card->sdio_func[i];
>>> -		if (func && sdio_func_present(func) && func->dev.driver) {
>>> -			const struct dev_pm_ops *pmops = func->dev.driver->pm;
>>> -			pmops->resume(&func->dev);
>>> -		}
>>> -	}
>>> -
>>> -	if (!err && mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host))
>> {
>>> +	if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
>>>  		mmc_claim_host(host);
>>>  		sdio_disable_wide(host->card);
>>>  		mmc_release_host(host);
>>>  	}
>>>
>>> -	if (!err && !mmc_card_keep_power(host))
>>> +	if (!mmc_card_keep_power(host))
>>>  		mmc_power_off(host);
>>>
>>> -	return err;
>>> +	return 0;
>>>  }
>>>
>>>  static int mmc_sdio_resume(struct mmc_host *host)  {
>>> -	int i, err = 0;
>>> +	int err = 0;
>>>
>>>  	BUG_ON(!host);
>>>  	BUG_ON(!host->card);
>>> @@ -1019,24 +1000,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
>>>  		wake_up_process(host->sdio_irq_thread);
>>>  	mmc_release_host(host);
>>>
>>> -	/*
>>> -	 * If the card looked to be the same as before suspending, then
>>> -	 * we proceed to resume all card functions.  If one of them returns
>>> -	 * an error then we simply return that error to the core and the
>>> -	 * card will be redetected as new.  It is the responsibility of
>>> -	 * the function driver to perform further tests with the extra
>>> -	 * knowledge it has of the card to confirm the card is indeed the
>>> -	 * same as before suspending (same MAC address for network cards,
>>> -	 * etc.) and return an error otherwise.
>>> -	 */
>>> -	for (i = 0; !err && i < host->card->sdio_funcs; i++) {
>>> -		struct sdio_func *func = host->card->sdio_func[i];
>>> -		if (func && sdio_func_present(func) && func->dev.driver) {
>>> -			const struct dev_pm_ops *pmops = func->dev.driver->pm;
>>> -			err = pmops->resume(&func->dev);
>>> -		}
>>> -	}
>>> -
>>>  	host->pm_flags &= ~MMC_PM_KEEP_POWER;
>>>  	return err;
>>>  }
>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>>> index 92d1ba8..4fa8fef9 100644
>>> --- a/drivers/mmc/core/sdio_bus.c
>>> +++ b/drivers/mmc/core/sdio_bus.c
>>> @@ -197,20 +197,8 @@ static int sdio_bus_remove(struct device *dev)
>>>
>>>  #ifdef CONFIG_PM
>>>
>>> -#ifdef CONFIG_PM_SLEEP
>>> -static int pm_no_operation(struct device *dev) -{
>>> -	/*
>>> -	 * Prevent the PM core from calling SDIO device drivers' suspend
>>> -	 * callback routines, which it is not supposed to do, by using this
>>> -	 * empty function as the bus type suspend callaback for SDIO.
>>> -	 */
>>> -	return 0;
>>> -}
>>> -#endif
>>> -
>>>  static const struct dev_pm_ops sdio_bus_pm_ops = {
>>> -	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
>>> +	SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume)
>>>  	SET_RUNTIME_PM_OPS(
>>>  		pm_generic_runtime_suspend,
>>>  		pm_generic_runtime_resume,
>>>
>