* [PATCH v1 0/2]: PCI: PM: Improvements related to D3cold -> D0 transitions @ 2022-04-04 15:38 Rafael J. Wysocki 2022-04-04 15:41 ` [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() Rafael J. Wysocki 2022-04-04 15:42 ` [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely Rafael J. Wysocki 0 siblings, 2 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2022-04-04 15:38 UTC (permalink / raw) To: Linux PCI; +Cc: Linux PM, LKML, Bjorn Helgaas, Mika Westerberg Hi All, This series of two patches addresses potential issues related to transitions of PCI devices from D3cold to D0. The first one is that devices initially in D3cold may be left in D0-uninitialized by pci_power_up() in some cases and the second one is that attempting to resume PCI devices under a bridge that fails to go from D3cold to D0 is pointless. Please refer to the patch changelogs for details. Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() 2022-04-04 15:38 [PATCH v1 0/2]: PCI: PM: Improvements related to D3cold -> D0 transitions Rafael J. Wysocki @ 2022-04-04 15:41 ` Rafael J. Wysocki 2022-04-05 9:53 ` Mika Westerberg 2022-04-04 15:42 ` [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely Rafael J. Wysocki 1 sibling, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2022-04-04 15:41 UTC (permalink / raw) To: Linux PCI; +Cc: Linux PM, LKML, Bjorn Helgaas, Mika Westerberg From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> In theory, pci_power_up() may leave a device in D0-uninitialized during a transition from D3cold to D0. Say, a PCIe device depending on some ACPI power resources is put into D3cold, so the power resources in question are all turned off. Then, pci_power_up() is called to put it into D0. It first calls pci_platform_power_transition() which invokes platform_pci_set_power_state() to turn on the ACPI power resources depended on by the device and, if that is successful, it calls pci_update_current_state() to update the current_state field of the PCI device object. If the device's configuration space is accessible at this point, which is the case if platform_pci_set_power_state() leaves it in D0-uninitialized (and there's nothing to prevent it from doing so), current_state will be set to PCI_D0 and the pci_raw_set_power_state() called subsequently will notice that the device is in D0 already and do nothing. However, that is not correct, because it may be still necessary to restore the device's BARs at this point. To address this issue, set current_state temporarily to PCI_D3hot in the cases in which pci_raw_set_power_state() may need to do more than just changing the power state of the device. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/pci/pci.c | 10 ++++++++++ 1 file changed, 10 insertions(+) Index: linux-pm/drivers/pci/pci.c =================================================================== --- linux-pm.orig/drivers/pci/pci.c +++ linux-pm/drivers/pci/pci.c @@ -1309,6 +1309,8 @@ static int pci_dev_wait(struct pci_dev * */ int pci_power_up(struct pci_dev *dev) { + pci_power_t old_state = dev->current_state; + pci_platform_power_transition(dev, PCI_D0); /* @@ -1325,6 +1327,14 @@ int pci_power_up(struct pci_dev *dev) pci_resume_bus(dev->subordinate); } + /* + * For transitions from D3hot or deeper and initial power-up, force + * PCI_PM_CTRL register write, D3*->D0 transition delay and BARS + * restoration. + */ + if (old_state >= PCI_D3hot) + dev->current_state = PCI_D3hot; + return pci_raw_set_power_state(dev, PCI_D0); } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() 2022-04-04 15:41 ` [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() Rafael J. Wysocki @ 2022-04-05 9:53 ` Mika Westerberg 2022-04-07 19:01 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Mika Westerberg @ 2022-04-05 9:53 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PCI, Linux PM, LKML, Bjorn Helgaas On Mon, Apr 04, 2022 at 05:41:13PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > In theory, pci_power_up() may leave a device in D0-uninitialized > during a transition from D3cold to D0. > > Say, a PCIe device depending on some ACPI power resources is put into > D3cold, so the power resources in question are all turned off. Then, > pci_power_up() is called to put it into D0. > > It first calls pci_platform_power_transition() which invokes > platform_pci_set_power_state() to turn on the ACPI power resources > depended on by the device and, if that is successful, it calls > pci_update_current_state() to update the current_state field of > the PCI device object. If the device's configuration space is > accessible at this point, which is the case if > platform_pci_set_power_state() leaves it in D0-uninitialized (and > there's nothing to prevent it from doing so), current_state will be > set to PCI_D0 and the pci_raw_set_power_state() called subsequently > will notice that the device is in D0 already and do nothing. > However, that is not correct, because it may be still necessary to > restore the device's BARs at this point. > > To address this issue, set current_state temporarily to PCI_D3hot > in the cases in which pci_raw_set_power_state() may need to do more > than just changing the power state of the device. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() 2022-04-05 9:53 ` Mika Westerberg @ 2022-04-07 19:01 ` Rafael J. Wysocki 2022-04-08 12:43 ` Mika Westerberg 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2022-04-07 19:01 UTC (permalink / raw) To: Mika Westerberg Cc: Rafael J. Wysocki, Linux PCI, Linux PM, LKML, Bjorn Helgaas On Tue, Apr 5, 2022 at 1:45 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Mon, Apr 04, 2022 at 05:41:13PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > In theory, pci_power_up() may leave a device in D0-uninitialized > > during a transition from D3cold to D0. > > > > Say, a PCIe device depending on some ACPI power resources is put into > > D3cold, so the power resources in question are all turned off. Then, > > pci_power_up() is called to put it into D0. > > > > It first calls pci_platform_power_transition() which invokes > > platform_pci_set_power_state() to turn on the ACPI power resources > > depended on by the device and, if that is successful, it calls > > pci_update_current_state() to update the current_state field of > > the PCI device object. If the device's configuration space is > > accessible at this point, which is the case if > > platform_pci_set_power_state() leaves it in D0-uninitialized (and > > there's nothing to prevent it from doing so), current_state will be > > set to PCI_D0 and the pci_raw_set_power_state() called subsequently > > will notice that the device is in D0 already and do nothing. > > However, that is not correct, because it may be still necessary to > > restore the device's BARs at this point. > > > > To address this issue, set current_state temporarily to PCI_D3hot > > in the cases in which pci_raw_set_power_state() may need to do more > > than just changing the power state of the device. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> Thanks, but on second thought, I'm not sure if this is the best way to address the issue. Basically, pci_power_up() is called in two places, in pci_set_power_state() (for the transitions to D0) and in pci_pm_default_resume_early(). In the latter case, pci_restore_state() is called right after it and that covers BARs restoration, so nothing more needs to be done in that case. This means that pci_set_power_state() is the only place needing to restore the BARs when going into D0 from D3hot or deeper and it is better to move BARs restoration directly into it. I'll update the series accordingly and resend. I also think that the mandatory delay is not needed at all when pci_raw_set_power_state() is called for transitions D3cold -> D0, because in that case either the device has been powered up via platform_pci_set_power_state(), or via the bridge resume which takes the delay into account. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() 2022-04-07 19:01 ` Rafael J. Wysocki @ 2022-04-08 12:43 ` Mika Westerberg 0 siblings, 0 replies; 7+ messages in thread From: Mika Westerberg @ 2022-04-08 12:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PCI, Linux PM, LKML, Bjorn Helgaas Hi Rafael, On Thu, Apr 07, 2022 at 09:01:59PM +0200, Rafael J. Wysocki wrote: > On Tue, Apr 5, 2022 at 1:45 PM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > On Mon, Apr 04, 2022 at 05:41:13PM +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > In theory, pci_power_up() may leave a device in D0-uninitialized > > > during a transition from D3cold to D0. > > > > > > Say, a PCIe device depending on some ACPI power resources is put into > > > D3cold, so the power resources in question are all turned off. Then, > > > pci_power_up() is called to put it into D0. > > > > > > It first calls pci_platform_power_transition() which invokes > > > platform_pci_set_power_state() to turn on the ACPI power resources > > > depended on by the device and, if that is successful, it calls > > > pci_update_current_state() to update the current_state field of > > > the PCI device object. If the device's configuration space is > > > accessible at this point, which is the case if > > > platform_pci_set_power_state() leaves it in D0-uninitialized (and > > > there's nothing to prevent it from doing so), current_state will be > > > set to PCI_D0 and the pci_raw_set_power_state() called subsequently > > > will notice that the device is in D0 already and do nothing. > > > However, that is not correct, because it may be still necessary to > > > restore the device's BARs at this point. > > > > > > To address this issue, set current_state temporarily to PCI_D3hot > > > in the cases in which pci_raw_set_power_state() may need to do more > > > than just changing the power state of the device. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Thanks, but on second thought, I'm not sure if this is the best way to > address the issue. > > Basically, pci_power_up() is called in two places, in > pci_set_power_state() (for the transitions to D0) and in > pci_pm_default_resume_early(). In the latter case, > pci_restore_state() is called right after it and that covers BARs > restoration, so nothing more needs to be done in that case. I see. > This means that pci_set_power_state() is the only place needing to > restore the BARs when going into D0 from D3hot or deeper and it is > better to move BARs restoration directly into it. I'll update the > series accordingly and resend. Okay sounds good. > I also think that the mandatory delay is not needed at all when > pci_raw_set_power_state() is called for transitions D3cold -> D0, > because in that case either the device has been powered up via > platform_pci_set_power_state(), or via the bridge resume which takes > the delay into account. I agree. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely 2022-04-04 15:38 [PATCH v1 0/2]: PCI: PM: Improvements related to D3cold -> D0 transitions Rafael J. Wysocki 2022-04-04 15:41 ` [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() Rafael J. Wysocki @ 2022-04-04 15:42 ` Rafael J. Wysocki 2022-04-05 9:54 ` Mika Westerberg 1 sibling, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2022-04-04 15:42 UTC (permalink / raw) To: Linux PCI; +Cc: Linux PM, LKML, Bjorn Helgaas, Mika Westerberg From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> It is rather counter-intuitive to attempt to resume devices on a bus segment before completing a transition of their parent bridge into D0, so do that when the transition is complete. This matters especially when the transition in question is not successful, in which case it doesn't make sense to even try to resume the child devices at all. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/pci/pci.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) Index: linux-pm/drivers/pci/pci.c =================================================================== --- linux-pm.orig/drivers/pci/pci.c +++ linux-pm/drivers/pci/pci.c @@ -1310,15 +1310,24 @@ static int pci_dev_wait(struct pci_dev * int pci_power_up(struct pci_dev *dev) { pci_power_t old_state = dev->current_state; + int ret; pci_platform_power_transition(dev, PCI_D0); + /* + * For transitions from D3hot or deeper (including unknown), force + * PCI_PM_CTRL register write, D3*->D0 transition delay and BARS + * restoration. + */ + if (old_state >= PCI_D3hot) + dev->current_state = PCI_D3hot; + ret = pci_raw_set_power_state(dev, PCI_D0); /* * Mandatory power management transition delays are handled in * pci_pm_resume_noirq() and pci_pm_runtime_resume() of the * corresponding bridge. */ - if (dev->runtime_d3cold) { + if (!ret && dev->runtime_d3cold) { /* * When powering on a bridge from D3cold, the whole hierarchy * may be powered on into D0uninitialized state, resume them to @@ -1327,15 +1336,7 @@ int pci_power_up(struct pci_dev *dev) pci_resume_bus(dev->subordinate); } - /* - * For transitions from D3hot or deeper and initial power-up, force - * PCI_PM_CTRL register write, D3*->D0 transition delay and BARS - * restoration. - */ - if (old_state >= PCI_D3hot) - dev->current_state = PCI_D3hot; - - return pci_raw_set_power_state(dev, PCI_D0); + return ret; } /** ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely 2022-04-04 15:42 ` [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely Rafael J. Wysocki @ 2022-04-05 9:54 ` Mika Westerberg 0 siblings, 0 replies; 7+ messages in thread From: Mika Westerberg @ 2022-04-05 9:54 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PCI, Linux PM, LKML, Bjorn Helgaas On Mon, Apr 04, 2022 at 05:42:27PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > It is rather counter-intuitive to attempt to resume devices on a bus > segment before completing a transition of their parent bridge into > D0, so do that when the transition is complete. > > This matters especially when the transition in question is not > successful, in which case it doesn't make sense to even try to > resume the child devices at all. Good point. > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-08 12:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-04 15:38 [PATCH v1 0/2]: PCI: PM: Improvements related to D3cold -> D0 transitions Rafael J. Wysocki 2022-04-04 15:41 ` [PATCH v1 1/2] PCI: PM: Avoid leaving devices in D0-uninitialized in pci_power_up() Rafael J. Wysocki 2022-04-05 9:53 ` Mika Westerberg 2022-04-07 19:01 ` Rafael J. Wysocki 2022-04-08 12:43 ` Mika Westerberg 2022-04-04 15:42 ` [PATCH v1 2/2] PCI: PM: Resume bus after putting the bridge into D0 entirely Rafael J. Wysocki 2022-04-05 9:54 ` Mika Westerberg
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).