> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx] > Sent: Wednesday, March 12, 2014 8:41 PM > To: Chris Ball; Lu, Aaron; Wang, Xiaoming; Dong, Chuanxiao > Cc: linux-mmc@xxxxxxxxxxxxxxx; Liu, Chuansheng; linux-kernel@xxxxxxxxxxxxxxx; > NeilBrown; Rafael J. Wysocki > Subject: Re: [RFC PATCH] mmc: core: Invoke sdio func driver's PM callbacks from the > sdio bus > > On 12 March 2014 07:26, Aaron Lu <aaron.lu@xxxxxxxxx> wrote: > > 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@xxxxxxxxx> > >> Tested-by: Chuanxiao Dong <chuanxiao.dong@xxxxxxxxx> > > > > Thanks a lot for the test! > > > > -Aaron > > > > Thanks for helping out testing! > > Just out of curiosity, which sdio func driver did you use (or maybe it hasn't been > upstreamed yet)? > > Anyway, I suppose it's ->suspend callback don't return -ENOSYS with the > expectation of the card to be removed? > > So I assume you want this to go to stable as well, right? Hi Uffe, We are testing based on BRCM WiFi driver. It is not in upstream. Actually no matter which kind of SDIO function driver it is, if this SDIO host device is an ACPI device which contains SDIO device as its children, the SDIO function driver's suspend will be called twice. And you are right, there is no -ENOSYS returned if the card is removed. It's better to have this patch go to stable as well. Thanks Chuanxiao > > Kind regards > Uffe > > >> > >> Thanks > >> Chuanxiao > >> > >>> -----Original Message----- > >>> From: Lu, Aaron > >>> Sent: Wednesday, March 12, 2014 10:36 AM > >>> To: Ulf Hansson; linux-mmc@xxxxxxxxxxxxxxx; Chris Ball; Liu, > >>> Chuansheng; Dong, Chuanxiao > >>> Cc: linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx> > >>>> Cc: NeilBrown <neilb@xxxxxxx> > >>>> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> > >>>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > >>>> --- > >>>> > >>>> 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, > >>>> > >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
