On 02/27/2014 09:05 PM, Ulf Hansson wrote: > On 27 February 2014 12:26, Aaron Lu <aaron.lu@xxxxxxxxx> wrote: >> On 02/27/2014 06:18 PM, Ulf Hansson wrote: >>> On 27 February 2014 10:10, Aaron Lu <aaron.lu@xxxxxxxxx> wrote: >>>> Hi Ulf, >>>> >>>> I was tracking some SDIO suspend problem and came across this. As Neil >>>> mentioned here: >>>> http://lkml.org/lkml/2012/3/25/20 >>>> Quote: >>>> " >>>> SDIO (and possible MMC in general) has a protocol where the suspend >>>> method can return -ENOSYS and this means "There is no point in suspending, >>>> just turn me off". >>>> " >>>> >>>> It seems that the following commit: >>>> >>>> commit 810caddba42a54fe5db4e2664757a9a334ba359c >>>> Author: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >>>> Date: Mon Jun 10 17:03:37 2013 +0200 >>>> >>>> mmc: core: Validate suspend prerequisites for SDIO at SUSPEND_PREPARE >>>> >>>> Changed this behaviour? >>> >>> I realized I changed the behaviour to not cover for sdio func drivers, >>> that actually implements the pm callbacks - but do return -ENOSYS in >>> them. That wasn't obvious when looking at the code back then, sorry! >> >> Never mind, this behaviour change doesn't cause my problems but knowing >> whether this behaviour should be kept affects how I'm going to solve my >> problem. > > So let's aim for the a proper solution instead of a quick hack. OK. > > Although apparently mwifiex, libertas and btmrvl_sdio (bluetooth) may > return -ENOSYS from the respective system supend callbacks, thus > expecting the card to be removed. Currently this won't happen, but > instead the suspend will be aborted, which is really bad. > > I believe those driver's suspend callback should be fixed to not > return -ENOSYS. Returning 0 will, when MMC_PM_KEEP_POWER is not set, > power off the card similar what's done when removing the card - that > should be perfectly fine. Do note that the sdio func driver then > should expect the resume callback to invoked, instead of being > _probed_ at the next system resume. Good to know this. > >> >> My problem is that, after the following commit: >> >> commit eed222aca8d077af3600b651176f6fd04d95cce1 >> Author: Aaron Lu <aaron.lu@xxxxxxxxx> >> Date: Tue Mar 5 11:24:52 2013 +0800 >> >> mmc: sdio: bind acpi with sdio function device >> >> The SDIO function that has an ACPI node associated with will have the >> pm_domain assigned, which breaks the intention that during SDIO function >> device suspend phase nothing should be done by having a dummy BUS layer >> callback. The existence of the pm_domain for the SDIO function device >> will make its function driver's suspend callback gets called now. The >> end result is the function driver's suspend callback is called twice. > > Why is the sdio bus ignored? In __device_suspend, if pm_domain is set, the suspend callback is fetched there, no matter if there is a suspend callback defined in the pm_domain or not. In ACPI PM domain's case, the suspend callback is NULL, so the driver's suspend callback is used then. I consulted Rafael whether adding a NULL check before goto Run is OK like the following shows: diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 1b41fca3d65a..506583d84ed4 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1160,7 +1160,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) if (dev->pm_domain) { info = "power domain "; callback = pm_op(&dev->pm_domain->ops, state); - goto Run; + if (callback) + goto Run; } if (dev->type && dev->type->pm) { And Rafael said that would introduce regressions elsewhere, so it's a no go. > > Are you saying the power domain is using the pm_generic_suspend for or > from it's ->suspend callback? If so, that could be the problem!? > >> >> To solve this problem, I was wondering why SDIO function device has this >> 'special' requirement that nothing should be done at its own device >> suspend phase but instead, relies on its suspend callback gets called in >> its parent device's suspend callback. And then I realized the reason is >> for the special handling of -ENOSYS. > > That's to my understanding not the only reason. > > I think it's more a matter of having a controlled suspend sequence. > The mmc core are not able to serve any new SDIO requests while it is > suspended, therefore it tells the sdio func driver about when it safe > to send request - using it's PM callbacks. Does this mean after the function device is suspended from PM core's pespective, the mmc core will still send some requests to the function device? I did see one such request, disable_width, get sent after the function driver's suspend callback is invoked, don't know if there are others. >From the mmc_sdio_suspend function I can see two things are done: 1 function device driver's suspend callback is called; 2 optionally disable_width and power off the card according to some flags. So once we fixed the -ENOSYS problem, can we have the function device run its own suspend callback and have the mmc_sdio_suspend just did the disable_width and power off thing? > > You could debate whether that actually should be done though the PM > callbacks, or by some other SDIO specific callbacks. Haven't thought > it through completely yet. > >> >> So if we could get rid of the -ENOSYS, my problem could be easily solved >> by deleting some lines in current code(the calling of function driver's >> suspend callback in mmc_sdio_suspend and the dummy system suspend/resume >> callback for SDIO bus). Buf if the behaviour has to be kept, I'll >> probably need to remove the pm_domain for the device and do some >> additional ACPI handing in mmc_sdio_suspend/resume for the function >> device. >> > > So we have two different issues to address here. > > Removing the option for sdio func drivers to return -ENOSYS from their > suspend callback - has already be done, though by my mistake. Anyway, > this won't solve your problem. > > Additionally, I don't like putting this option back, since it's not > possible to remove the card in that path. Still we could handle > -ENOSYS and OK error and decide to power off the card. On the other > hand, we could instead fix the sdio func drivers, that should be quite > easy I think. > >>> >>> There are no solution to this problem in the mmc core right now, since >>> we can't remove the card while we have reach the state when the >>> suspend callback is being invoked. >>> >>> Instead, the sdio func driver shall not implement the PM callbacks at >>> all. That behaviour means the mmc core will remove the card, but now >>> it's done a in an earlier phase of the system suspend when we are >>> still able to remove it. >> >> The libertas suspend callback is doing different things depending on >> different conditions - sometime it will enable wakeup capability and >> sometime it will want the mmc core to remove the device entirely by >> retuning -ENOSYS, so it may not be that easy by just deleting the >> callback, but I don't know for sure. > > I had a look, the callback must remains. > > As, stated - fix the suspend callback to not return -ENOSYS. OK, I see, I'll try to come up with a patch for this, thanks for the suggestion! -Aaron > > Kind regards > Uffe > > >> >> Thanks, >> Aaron -- 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
