+ linux-pm, Rafael, Geert
On 21 December 2017 at 15:25, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 21/12/17 16:15, Ulf Hansson wrote:
>> On 20 December 2017 at 09:18, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>>> sdhci_enable_irq_wakeups() is already called by sdhci_suspend_host() so
>>> sdhci-pci should not need to call it. However sdhci_suspend_host() only
>>> calls it if wakeups are enabled, and sdhci-pci does not enable them until
>>> after calling sdhci_suspend_host(). So move the calls to
>>> sdhci_pci_init_wakeup() before calling sdhci_suspend_host(), and
>>> stop calling sdhci_enable_irq_wakeups(). That results in some
>>> simplification because sdhci_pci_suspend_host() and
>>> __sdhci_pci_suspend_host() no longer need to be separate functions.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>>> ---
>>> drivers/mmc/host/sdhci-pci-core.c | 58 ++++++++++++++-------------------------
>>> 1 file changed, 21 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>> index 110c634cfb43..2ffc09f088ee 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -39,10 +39,29 @@
>>> static void sdhci_pci_hw_reset(struct sdhci_host *host);
>>>
>>> #ifdef CONFIG_PM_SLEEP
>>> -static int __sdhci_pci_suspend_host(struct sdhci_pci_chip *chip)
>>> +static int sdhci_pci_init_wakeup(struct sdhci_pci_chip *chip)
>>> +{
>>> + mmc_pm_flag_t pm_flags = 0;
>>> + int i;
>>> +
>>> + for (i = 0; i < chip->num_slots; i++) {
>>> + struct sdhci_pci_slot *slot = chip->slots[i];
>>> +
>>> + if (slot)
>>> + pm_flags |= slot->host->mmc->pm_flags;
>>> + }
>>> +
>>> + return device_init_wakeup(&chip->pdev->dev,
>>> + (pm_flags & MMC_PM_KEEP_POWER) &&
>>> + (pm_flags & MMC_PM_WAKE_SDIO_IRQ));
>>
>> A couple of comments here.
>>
>> Wakeup settings shouldn't be changed during system suspend. That means
>> we shouldn't call device_init_wakeup() (or device_set_wakeup_enable()
>> for that matter) here.
>>
>> Instead, device_init_wakeup() should be called during ->probe(), while
>> device_set_wakeup_enable() should normally *not* have to be called at
>> all by drivers. There are a exceptions for device_set_wakeup_enable(),
>> however it should not have to be called during system suspend, at
>> least.
>>
>> Of course, I realize that you are not changing the behavior in this
>> regards in this patch, so I am fine this anyway.
>>
>> My point is, that the end result while doing this improvements in this
>> series, should strive to that device_init_wakeup() and
>> device_set_wakeup_enable() becomes used correctly. I don't think that
>> is the case, or is it?
>
> Unfortunately we don't find out what the SDIO driver wants to do (i.e
> pm_flags & MMC_PM_WAKE_SDIO_IRQ) until suspend.
That's true! Of course, we need to be able to act on this, somehow.
>
> I considered enabling wakeups by default but wasn't sure that would not
> increase power consumption in devices not expecting it.
I understand the problem, believe me. However, I would rather like to
try to find a generic solution to these problems, else we will keep
abusing existing wakeups APIs.
For your information, I have been working on several issues on how to
handle the "wakeup path" correctly, which is linked to this problem.
See a few quite small series for this below [1], [2].
I think the general problems can be described liked this:
1) The dev->power.wakeup_path flag doesn't get set for the device by
the PM core, in case device_set_wakeup_enable() is called from a
->suspend() callback. That also means, that the parent device don't
get its >power.wakeup_path flag set in __device_suspend() by the PM
core, while this may be expected by upper layers (PM domains, middle
layers).
2) device_may_wakeup() doesn't tell us the hole story about the
wakeup. Only that is *may* be configured. :-)
So in cases when device_may_wakeup() returns true, we still have these
three conditions to consider, which we currently can't distinguish
between:
*) The wakeup is configured and enabled, so the device should be
enabled in the wakeup path.
**) The wakeup is configured and enabled, but as an out-band-wakeup
signal (like a GPIO IRQ). This means the device shouldn't be enabled
in the wakeup path.
***) The wakeup isn't configured, thus disabled, because there are no
consumers of the wakeup IRQ registered (as may be the case for SDIO
IRQ). This also means the device shouldn't be enabled in the wakeup
path.
Potentially, one could consider **) and ***) being treated in the same
way, via using an opt-out method, avoiding the wakeup path to be
enabled. Currently I have been thinking of adding an "OUT_BAND_WAKEUP"
driver PM flag, to deal with this, however there may be other
preferred options.
Kind regards
Uffe
[1]
https://www.spinics.net/lists/linux-renesas-soc/msg21421.html
[2]
https://www.spinics.net/lists/linux-renesas-soc/msg20122.html
(Actually the discussions there concluded that we may need an
"OUT_BAND_WAKEUP" flag instead)
--
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
