From: Kai Heng Feng <kai.heng.feng@canonical.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Rafael Wysocki <rafael.j.wysocki@intel.com>,
linux-pci@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
Mathias Nyman <mathias.nyman@intel.com>,
linux-usb@vger.kernel.org
Subject: Re: [PATCH] PCI / PM: Don't runtime suspend when device only supports wakeup from D0
Date: Wed, 22 May 2019 11:42:14 +0800 [thread overview]
Message-ID: <AE3B21D0-5BD9-40A5-B5A1-3E9A8B8E1327@canonical.com> (raw)
In-Reply-To: <20190521222300.GG57618@google.com>
at 6:23 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Mathias, linux-usb]
>
> On Wed, May 22, 2019 at 12:31:04AM +0800, Kai-Heng Feng wrote:
>> There's an xHC device that doesn't wake when a USB device gets plugged
>> to its USB port. The driver's own runtime suspend callback was called,
>> PME signaling was enabled, but it stays at PCI D0.
>
> s/xHC/xHCI/ ?
>
> This looks like it's fixing a bug? If so, please include a link to
> the bug report, and make sure the bug report has "lspci -vv" output
> attached to it.
Ok, I’ll update this in V2.
>
>> A PCI device can be runtime suspended to D0 when it supports D0 PME and
>> its _S0W reports D0. Theoratically this should work, but as [1]
>> specifies, D0 doesn't have wakeup capability.
>
> s/Theoratically/Theoretically/
Ok.
>
> What does "runtime suspended to D0" mean?
It’s runtime suspended by PCI core, but stays at D0.
> Is that different from the regular "device is fully operational" sort of D0?
Yes it's different to that.
Because of _S0W reports D0 and the device has D0 PME support, so it’s
“suspended” to D0:
00:10.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] FCH USB
XHCI Controller [1022:7914] (rev 20) (prog-if 30 [XHCI])
Subsystem: Dell FCH USB XHCI Controller [1028:096c]
Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Interrupt: pin A routed to IRQ 18
Region 0: Memory at f0668000 (64-bit, non-prefetchable) [size=8K]
Capabilities: [50] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable+ DSel=0 DScale=0 PME-
Capabilities: [70] MSI: Enable- Count=1/8 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [90] MSI-X: Enable+ Count=8 Masked-
Vector table: BAR=0 offset=00001000
PBA: BAR=0 offset=00001080
Capabilities: [a0] Express (v2) Root Complex Integrated Endpoint, MSI 00
DevCap: MaxPayload 128 bytes, PhantFunc 0
ExtTag- RBE+
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 128 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend-
DevCap2: Completion Timeout: Not Supported, TimeoutDis+, LTR+, OBFF Not Supported
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled
Capabilities: [100 v1] Latency Tolerance Reporting
Max snoop latency: 0ns
Max no snoop latency: 0ns
Kernel driver in use: xhci_hcd
PME signaling is correctly enabled:
Status: D0 NoSoftRst+ PME-Enable+ DSel=0 DScale=0 PME-
> If so, what
> distinguishes "runtime suspended D0" from "normal fully operational
> D0”?
The xHC’s own runtime suspend routine is called, but PCI core’s runtime
suspend routine decides it should stay at D0.
So it’s technically runtime suspended to D0.
Kai-Heng
>
>> To avoid this problematic situation, we should avoid runtime suspend if
>> D0 is the only state that can wake up the device.
>>
>> [1]
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/device-working-state-d0
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/pci/pci-driver.c | 5 +++++
>> drivers/pci/pci.c | 2 +-
>> include/linux/pci.h | 3 +++
>> 3 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index cae630fe6387..15a6310c5d7b 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1251,6 +1251,11 @@ static int pci_pm_runtime_suspend(struct device
>> *dev)
>> return 0;
>> }
>>
>> + if (pci_target_state(pci_dev, device_can_wakeup(dev)) == PCI_D0) {
>> + dev_dbg(dev, "D0 doesn't have wakeup capability\n");
>> + return -EBUSY;
>> + }
>> +
>> pci_dev->state_saved = false;
>> if (pm && pm->runtime_suspend) {
>> error = pm->runtime_suspend(dev);
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8abc843b1615..ceee6efbbcfe 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2294,7 +2294,7 @@ EXPORT_SYMBOL(pci_wake_from_d3);
>> * If the platform can't manage @dev, return the deepest state from which it
>> * can generate wake events, based on any available PME info.
>> */
>> -static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>> +pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>> {
>> pci_power_t target_state = PCI_D3hot;
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 4a5a84d7bdd4..91e8dc4d04aa 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1188,6 +1188,7 @@ bool pci_pme_capable(struct pci_dev *dev,
>> pci_power_t state);
>> void pci_pme_active(struct pci_dev *dev, bool enable);
>> int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable);
>> int pci_wake_from_d3(struct pci_dev *dev, bool enable);
>> +pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup);
>> int pci_prepare_to_sleep(struct pci_dev *dev);
>> int pci_back_from_sleep(struct pci_dev *dev);
>> bool pci_dev_run_wake(struct pci_dev *dev);
>> @@ -1672,6 +1673,8 @@ static inline int pci_set_power_state(struct
>> pci_dev *dev, pci_power_t state)
>> { return 0; }
>> static inline int pci_wake_from_d3(struct pci_dev *dev, bool enable)
>> { return 0; }
>> +pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
>> +{ return PCI_D0; }
>> static inline pci_power_t pci_choose_state(struct pci_dev *dev,
>> pm_message_t state)
>> { return PCI_D0; }
>> --
>> 2.17.1
next prev parent reply other threads:[~2019-05-22 3:42 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-21 16:31 [PATCH] PCI / PM: Don't runtime suspend when device only supports wakeup from D0 Kai-Heng Feng
2019-05-21 22:23 ` Bjorn Helgaas
2019-05-22 3:42 ` Kai Heng Feng [this message]
2019-05-22 13:48 ` Bjorn Helgaas
2019-05-22 15:46 ` Kai Heng Feng
2019-05-22 18:11 ` Bjorn Helgaas
2019-05-22 18:39 ` Alan Stern
2019-05-22 18:53 ` Lukas Wunner
2019-05-22 19:05 ` Kai Heng Feng
2019-05-22 20:52 ` Bjorn Helgaas
2019-05-23 4:39 ` Kai-Heng Feng
2019-05-27 16:57 ` Bjorn Helgaas
2019-06-05 11:57 ` Bjorn Helgaas
2019-07-05 7:02 ` Kai-Heng Feng
2019-07-05 9:39 ` Rafael J. Wysocki
2019-07-05 13:51 ` Kai-Heng Feng
2019-07-09 13:45 ` Bjorn Helgaas
2019-09-02 13:47 ` Kai-Heng Feng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AE3B21D0-5BD9-40A5-B5A1-3E9A8B8E1327@canonical.com \
--to=kai.heng.feng@canonical.com \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@intel.com \
--cc=rafael.j.wysocki@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.