* Re: SDIO driver return -ENOSYS behaviour change? [not found] ` <CAPDyKFpcJ5T9QXbFzZnu4fdgfjsTntfrtS=mo=5Wb_uJa7iMbw@mail.gmail.com> @ 2014-02-28 2:37 ` Aaron Lu 2014-02-28 8:30 ` Ulf Hansson 0 siblings, 1 reply; 3+ messages in thread From: Aaron Lu @ 2014-02-28 2:37 UTC (permalink / raw) To: Ulf Hansson Cc: NeilBrown, linux-mmc@vger.kernel.org, Rafael J. Wysocki, Linux-pm mailing list On 02/27/2014 09:05 PM, Ulf Hansson wrote: > On 27 February 2014 12:26, Aaron Lu <aaron.lu@intel.com> wrote: >> On 02/27/2014 06:18 PM, Ulf Hansson wrote: >>> On 27 February 2014 10:10, Aaron Lu <aaron.lu@intel.com> 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@linaro.org> >>>> 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@intel.com> >> 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: SDIO driver return -ENOSYS behaviour change? 2014-02-28 2:37 ` SDIO driver return -ENOSYS behaviour change? Aaron Lu @ 2014-02-28 8:30 ` Ulf Hansson 2014-02-28 8:49 ` Aaron Lu 0 siblings, 1 reply; 3+ messages in thread From: Ulf Hansson @ 2014-02-28 8:30 UTC (permalink / raw) To: Aaron Lu Cc: NeilBrown, linux-mmc@vger.kernel.org, Rafael J. Wysocki, Linux-pm mailing list On 28 February 2014 03:37, Aaron Lu <aaron.lu@intel.com> wrote: > On 02/27/2014 09:05 PM, Ulf Hansson wrote: >> On 27 February 2014 12:26, Aaron Lu <aaron.lu@intel.com> wrote: >>> On 02/27/2014 06:18 PM, Ulf Hansson wrote: >>>> On 27 February 2014 10:10, Aaron Lu <aaron.lu@intel.com> 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@linaro.org> >>>>> 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@intel.com> >>> 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. Will it be possible to add a suspend callback in the ACPI PM domain, which then respects susbystems callbacks before it uses pm_generic_suspend? > > 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. Nope. The mmc core will send request to the "card device" during suspend. Those are part's of a sequence to put the card in a proper "suspend mode". > > 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? That requires that all sdio func devices get suspended before the card devices. (And resumed after, but that will of course be implicit.) I guess the guarantee for that is already present, since we have added the func device to the driver model after we have added the card device at mmc_sdio_attach(). So, I suppose this could work. Do you want me to create a patch that you can test? Kind regards Ulf Hansson > >> >> 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: SDIO driver return -ENOSYS behaviour change? 2014-02-28 8:30 ` Ulf Hansson @ 2014-02-28 8:49 ` Aaron Lu 0 siblings, 0 replies; 3+ messages in thread From: Aaron Lu @ 2014-02-28 8:49 UTC (permalink / raw) To: Ulf Hansson Cc: NeilBrown, linux-mmc@vger.kernel.org, Rafael J. Wysocki, Linux-pm mailing list On 02/28/2014 04:30 PM, Ulf Hansson wrote: > On 28 February 2014 03:37, Aaron Lu <aaron.lu@intel.com> wrote: >> On 02/27/2014 09:05 PM, Ulf Hansson wrote: >>> 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. > > Nope. The mmc core will send request to the "card device" during > suspend. Those are part's of a sequence to put the card in a proper > "suspend mode". I suppose this request is sent after calling function driver's suspend callback? BTW, what request is it? > >> >> 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? > > That requires that all sdio func devices get suspended before the card > devices. (And resumed after, but that will of course be implicit.) Yes, and this is guaranteed by PM core as the function device is the child of the card device. > > I guess the guarantee for that is already present, since we have added > the func device to the driver model after we have added the card > device at mmc_sdio_attach(). So, I suppose this could work. Exactly. > > Do you want me to create a patch that you can test? That would be good. I do not have any SDIO card at hand, but I would be glad to review the patch. Thanks, Aaron ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-28 8:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <530F0118.2010501@intel.com>
[not found] ` <CAPDyKFrZAAVfFcaFx7o3TWMikWx7TvSPW_AuV1VsiuQF656uXg@mail.gmail.com>
[not found] ` <530F20E5.9000107@intel.com>
[not found] ` <CAPDyKFpcJ5T9QXbFzZnu4fdgfjsTntfrtS=mo=5Wb_uJa7iMbw@mail.gmail.com>
2014-02-28 2:37 ` SDIO driver return -ENOSYS behaviour change? Aaron Lu
2014-02-28 8:30 ` Ulf Hansson
2014-02-28 8:49 ` Aaron Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).