LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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 23:46:25 +0800	[thread overview]
Message-ID: <D5DC20F1-6B8F-466F-BAE7-65F0C8FB3D1D@canonical.com> (raw)
In-Reply-To: <20190522134854.GA79339@google.com>



> On May 22, 2019, at 9:48 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> On Wed, May 22, 2019 at 11:42:14AM +0800, Kai Heng Feng wrote:
>> at 6:23 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> 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.
>>> 
>>> 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.
> 
> I see your bug report (https://bugzilla.kernel.org/show_bug.cgi?id=203673)
> but it doesn't say anything about what this looks like to a user.
> Presumably somebody complained about something not working.  The bug
> report should include that information about what isn't working.
> Ideally, a user experiencing a problem should be able to google for
> the symptoms and find the bug report.

Sorry about that. I’ve added a comment to describe the symptom.

> 
>>>> 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.
>>> 
>>> What does "runtime suspended to D0" mean?
> 
> If I understand correctly, Linux normally clears PME-Enable while
> devices are in D0, but sets PME-Enable if a device is "runtime
> suspended to D0”.

Yes, this is what happens here.

> 
> I'm not sure I'd describe that as "suspended", since the power
> management state is exactly D0 and the only difference is that a PME
> interrupt is enabled.  It sounds to me like the xHCI controller is
> basically using PME as a hotplug interrupt to say "something happened
> on my secondary (USB) interface".  But that's more a question for
> Rafael.

Runtime suspend routines are called successfully, so I think it’s still logically suspended.

> 
> And I guess this patch basically means we wouldn't call the driver's
> suspend callback if we're merely going to stay at D0, so the driver
> would have no idea anything happened.  That might match
> Documentation/power/pci.txt better, because it suggests that the
> suspend callback is related to putting a device in a low-power state,
> and D0 is not a low-power state.

Yes, the patch is to let the device stay at D0 and don’t run driver’s own runtime suspend routine.

I guess I’ll just proceed to send a V2 with updated commit message?

Kai-Heng

> 
> Maybe we should also update Documentation/power/pci.txt to say "PCI
> devices ... can be programmed to generate PMEs while in any state
> (D0-D3)" instead of "a low-power state (D1-D3)”.
> 
> Anyway, this is all Rafael's area, so I'll defer to him.
> 
>>>> 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
>> 
>> 


  reply	other threads:[~2019-05-22 15:46 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 16:31 Kai-Heng Feng
2019-05-21 22:23 ` Bjorn Helgaas
2019-05-22  3:42   ` Kai Heng Feng
2019-05-22 13:48     ` Bjorn Helgaas
2019-05-22 15:46       ` Kai Heng Feng [this message]
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=D5DC20F1-6B8F-466F-BAE7-65F0C8FB3D1D@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 \
    --subject='Re: [PATCH] PCI / PM: Don'\''t runtime suspend when device only supports wakeup from D0' \
    /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

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).