* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
@ 2022-04-25 18:32 ` David E. Box
0 siblings, 0 replies; 17+ messages in thread
From: David E. Box @ 2022-04-25 18:32 UTC (permalink / raw)
To: Bjorn Helgaas, Jingar, Rajvi
Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pm, linux-pci,
Wysocki, Rafael J, linux-kernel, koba.ko, Kai-Heng Feng,
Oliver O'Halloran, bhelgaas, mika.westerberg, baolu.lu
On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > For the PCIe devices (like nvme) that do not go into D3 state still
> > > > > need to
> > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > power PM
> > > > > state and the SoC to reach a lower-power idle state as a whole. Move
> > > > > the
> > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
> > > > > not
> > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > issue
> > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
> > > > > Coffee
> > > > > Lake CPU platforms to get improved residency in low power idle states.
> > > > >
> > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > ---
> > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > drivers/pci/pci.c | 10 ----------
> > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > --- a/drivers/pci/pci-driver.c
> > > > > +++ b/drivers/pci/pci-driver.c
> > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > *dev)
> > > > > if (!pci_dev->state_saved) {
> > > > > pci_save_state(pci_dev);
> > > > > + /*
> > > > > + * There are systems (for example, Intel mobile chips
> > > > > since
> > > Coffee
> > > > > + * Lake) where the power drawn while suspended can be
> > > significantly
> > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > allows the
> > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > reach a
> > > > > + * lower-power idle state as a whole.
> > > > > + */
> > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > + pci_disable_ptm(pci_dev);
> > >
> > > Why is disabling PTM dependent on pci_dev->state_saved? The point of
> > > this is to change the behavior of the device, and it seems like we
> > > want to do that regardless of whether the driver has used
> > > pci_save_state().
> >
> > Because we use the saved state to restore PTM on the root port.
> > And it's under this condition that the root port state gets saved.
>
> Yes, I understand that pci_restore_ptm_state() depends on a previous
> call to pci_save_ptm_state().
>
> The point I'm trying to make is that pci_disable_ptm() changes the
> state of the device, and that state change should not depend on
> whether the driver has used pci_save_state().
We do it here because D3 depends on whether the device state was saved by the
driver.
if (!pci_dev->state_saved) {
pci_save_state(pci_dev);
/* disable PTM here */
if (pci_power_manageable(pci_dev))
pci_prepare_to_sleep(pci_dev);
}
If we disable PTM before the check, we will have saved "PTM disabled" as the
restore state. And we can't do it after the check as the device will be in D3.
As to disabling PTM on all devices, I see no problem with this, but the
reasoning is different. We disabled the root port PTM for power savings.
David
>
> When we're putting a device into a low-power state, I think we want to
> disable PTM *always*, no matter what the driver did. And I think we
> want to do it for all devices, not just Root Ports.
>
> Bjorn
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
2022-04-25 18:32 ` David E. Box
@ 2022-04-25 18:39 ` Rafael J. Wysocki
-1 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-04-25 18:39 UTC (permalink / raw)
To: David Box, Bjorn Helgaas
Cc: Jingar, Rajvi, bhelgaas, linux-pci, linux-kernel, linux-pm,
Wysocki, Rafael J, Kai-Heng Feng, mika.westerberg, koba.ko,
baolu.lu, sathyanarayanan.kuppuswamy, Russell Currey,
Oliver O'Halloran, linuxppc-dev
On Mon, Apr 25, 2022 at 8:33 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > For the PCIe devices (like nvme) that do not go into D3 state still
> > > > > > need to
> > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > power PM
> > > > > > state and the SoC to reach a lower-power idle state as a whole. Move
> > > > > > the
> > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
> > > > > > not
> > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > issue
> > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
> > > > > > Coffee
> > > > > > Lake CPU platforms to get improved residency in low power idle states.
> > > > > >
> > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > ---
> > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > *dev)
> > > > > > if (!pci_dev->state_saved) {
> > > > > > pci_save_state(pci_dev);
> > > > > > + /*
> > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > since
> > > > Coffee
> > > > > > + * Lake) where the power drawn while suspended can be
> > > > significantly
> > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > allows the
> > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > reach a
> > > > > > + * lower-power idle state as a whole.
> > > > > > + */
> > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > + pci_disable_ptm(pci_dev);
> > > >
> > > > Why is disabling PTM dependent on pci_dev->state_saved? The point of
> > > > this is to change the behavior of the device, and it seems like we
> > > > want to do that regardless of whether the driver has used
> > > > pci_save_state().
> > >
> > > Because we use the saved state to restore PTM on the root port.
> > > And it's under this condition that the root port state gets saved.
> >
> > Yes, I understand that pci_restore_ptm_state() depends on a previous
> > call to pci_save_ptm_state().
> >
> > The point I'm trying to make is that pci_disable_ptm() changes the
> > state of the device, and that state change should not depend on
> > whether the driver has used pci_save_state().
>
> We do it here because D3 depends on whether the device state was saved by the
> driver.
>
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
>
> /* disable PTM here */
>
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
>
>
> If we disable PTM before the check, we will have saved "PTM disabled" as the
> restore state. And we can't do it after the check as the device will be in D3.
>
> As to disabling PTM on all devices, I see no problem with this, but the
> reasoning is different. We disabled the root port PTM for power savings.
Right. As per the comment explaining why it is disabled.
> >
> > When we're putting a device into a low-power state, I think we want to
> > disable PTM *always*, no matter what the driver did. And I think we
> > want to do it for all devices, not just Root Ports.
> >
> > Bjorn
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
@ 2022-04-25 18:39 ` Rafael J. Wysocki
0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2022-04-25 18:39 UTC (permalink / raw)
To: David Box, Bjorn Helgaas
Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pm, linux-pci,
Wysocki, Rafael J, linux-kernel, koba.ko, Jingar, Rajvi,
Kai-Heng Feng, Oliver O'Halloran, bhelgaas, mika.westerberg,
baolu.lu
On Mon, Apr 25, 2022 at 8:33 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > For the PCIe devices (like nvme) that do not go into D3 state still
> > > > > > need to
> > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > power PM
> > > > > > state and the SoC to reach a lower-power idle state as a whole. Move
> > > > > > the
> > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
> > > > > > not
> > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > issue
> > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
> > > > > > Coffee
> > > > > > Lake CPU platforms to get improved residency in low power idle states.
> > > > > >
> > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > ---
> > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > *dev)
> > > > > > if (!pci_dev->state_saved) {
> > > > > > pci_save_state(pci_dev);
> > > > > > + /*
> > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > since
> > > > Coffee
> > > > > > + * Lake) where the power drawn while suspended can be
> > > > significantly
> > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > allows the
> > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > reach a
> > > > > > + * lower-power idle state as a whole.
> > > > > > + */
> > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > + pci_disable_ptm(pci_dev);
> > > >
> > > > Why is disabling PTM dependent on pci_dev->state_saved? The point of
> > > > this is to change the behavior of the device, and it seems like we
> > > > want to do that regardless of whether the driver has used
> > > > pci_save_state().
> > >
> > > Because we use the saved state to restore PTM on the root port.
> > > And it's under this condition that the root port state gets saved.
> >
> > Yes, I understand that pci_restore_ptm_state() depends on a previous
> > call to pci_save_ptm_state().
> >
> > The point I'm trying to make is that pci_disable_ptm() changes the
> > state of the device, and that state change should not depend on
> > whether the driver has used pci_save_state().
>
> We do it here because D3 depends on whether the device state was saved by the
> driver.
>
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
>
> /* disable PTM here */
>
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
>
>
> If we disable PTM before the check, we will have saved "PTM disabled" as the
> restore state. And we can't do it after the check as the device will be in D3.
>
> As to disabling PTM on all devices, I see no problem with this, but the
> reasoning is different. We disabled the root port PTM for power savings.
Right. As per the comment explaining why it is disabled.
> >
> > When we're putting a device into a low-power state, I think we want to
> > disable PTM *always*, no matter what the driver did. And I think we
> > want to do it for all devices, not just Root Ports.
> >
> > Bjorn
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
2022-04-25 18:32 ` David E. Box
@ 2022-04-26 16:50 ` Bjorn Helgaas
-1 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2022-04-26 16:50 UTC (permalink / raw)
To: David E. Box
Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pm, linux-pci,
Wysocki, Rafael J, linux-kernel, koba.ko, Jingar, Rajvi,
Kai-Heng Feng, Oliver O'Halloran, bhelgaas, mika.westerberg,
baolu.lu
On Mon, Apr 25, 2022 at 11:32:54AM -0700, David E. Box wrote:
> On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > For the PCIe devices (like nvme) that do not go into D3 state still
> > > > > > need to
> > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > power PM
> > > > > > state and the SoC to reach a lower-power idle state as a whole. Move
> > > > > > the
> > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
> > > > > > not
> > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > issue
> > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
> > > > > > Coffee
> > > > > > Lake CPU platforms to get improved residency in low power idle states.
> > > > > >
> > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > ---
> > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > *dev)
> > > > > > if (!pci_dev->state_saved) {
> > > > > > pci_save_state(pci_dev);
> > > > > > + /*
> > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > since
> > > > Coffee
> > > > > > + * Lake) where the power drawn while suspended can be
> > > > significantly
> > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > allows the
> > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > reach a
> > > > > > + * lower-power idle state as a whole.
> > > > > > + */
> > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > + pci_disable_ptm(pci_dev);
> > > >
> > > > Why is disabling PTM dependent on pci_dev->state_saved? The
> > > > point of this is to change the behavior of the device, and it
> > > > seems like we want to do that regardless of whether the driver
> > > > has used pci_save_state().
> > >
> > > Because we use the saved state to restore PTM on the root port.
> > > And it's under this condition that the root port state gets
> > > saved.
> >
> > Yes, I understand that pci_restore_ptm_state() depends on a
> > previous call to pci_save_ptm_state().
> >
> > The point I'm trying to make is that pci_disable_ptm() changes the
> > state of the device, and that state change should not depend on
> > whether the driver has used pci_save_state().
>
> We do it here because D3 depends on whether the device state was
> saved by the driver.
>
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
>
> /* disable PTM here */
>
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
>
>
> If we disable PTM before the check, we will have saved "PTM
> disabled" as the restore state. And we can't do it after the check
> as the device will be in D3.
Are you suggesting that PTM should be left enabled if the driver
called pci_save_state(), but disabled otherwise? I don't see the
rationale for that.
I don't understand all the paths through pci_pm_suspend_noirq() (e.g.,
skip_bus_pm), but for this one, I think we could do something like
this:
driver_saved = pci_dev->state_saved;
if (!driver_saved)
pci_save_state(pci_dev);
pci_disable_ptm(pci_dev);
if (!driver_saved) {
if (pci_power_manageable(pci_dev))
pci_prepare_to_sleep(pci_dev);
}
Or I guess one could argue that a driver calling pci_save_state() is
implicitly taking responsibility for all PCI-related suspend work, and
it should be disabling PTM itself. But that doesn't really seem
maintainable.
> As to disabling PTM on all devices, I see no problem with this, but the
> reasoning is different. We disabled the root port PTM for power savings.
The power saving is good. I'm trying to make the argument that we
need to disable PTM on all devices for correctness.
If we disable PTM on the root port, are we guaranteed that it will
never receive a PTM Request from a downstream device? Per PCIe r6.0,
sec 6.21.3, such a request would cause an Unsupported Request error.
I sort of expect that if we're putting a root port in a low-power
state, all downstream devices are already in the same or a lower-power
state (but I don't understand PM well enough to be confident).
And I don't really *expect* devices in a low-power state to generate
PTM Requests, but I haven't seen anything in the spec that prohibits
it.
This leads me to believe that if we disable PTM in a root port, we
must first disable PTM in any downstream devices. Otherwise, the root
port may log UR errors if the downstream device issues a PTM Request.
> > When we're putting a device into a low-power state, I think we want to
> > disable PTM *always*, no matter what the driver did. And I think we
> > want to do it for all devices, not just Root Ports.
> >
> > Bjorn
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
@ 2022-04-26 16:50 ` Bjorn Helgaas
0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2022-04-26 16:50 UTC (permalink / raw)
To: David E. Box
Cc: Jingar, Rajvi, bhelgaas, linux-pci, linux-kernel, linux-pm,
Wysocki, Rafael J, Kai-Heng Feng, mika.westerberg, koba.ko,
baolu.lu, sathyanarayanan.kuppuswamy, Russell Currey,
Oliver O'Halloran, linuxppc-dev
On Mon, Apr 25, 2022 at 11:32:54AM -0700, David E. Box wrote:
> On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > For the PCIe devices (like nvme) that do not go into D3 state still
> > > > > > need to
> > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > power PM
> > > > > > state and the SoC to reach a lower-power idle state as a whole. Move
> > > > > > the
> > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path is
> > > > > > not
> > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > issue
> > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530 with
> > > > > > Coffee
> > > > > > Lake CPU platforms to get improved residency in low power idle states.
> > > > > >
> > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save power")
> > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > ---
> > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > *dev)
> > > > > > if (!pci_dev->state_saved) {
> > > > > > pci_save_state(pci_dev);
> > > > > > + /*
> > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > since
> > > > Coffee
> > > > > > + * Lake) where the power drawn while suspended can be
> > > > significantly
> > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > allows the
> > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > reach a
> > > > > > + * lower-power idle state as a whole.
> > > > > > + */
> > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > + pci_disable_ptm(pci_dev);
> > > >
> > > > Why is disabling PTM dependent on pci_dev->state_saved? The
> > > > point of this is to change the behavior of the device, and it
> > > > seems like we want to do that regardless of whether the driver
> > > > has used pci_save_state().
> > >
> > > Because we use the saved state to restore PTM on the root port.
> > > And it's under this condition that the root port state gets
> > > saved.
> >
> > Yes, I understand that pci_restore_ptm_state() depends on a
> > previous call to pci_save_ptm_state().
> >
> > The point I'm trying to make is that pci_disable_ptm() changes the
> > state of the device, and that state change should not depend on
> > whether the driver has used pci_save_state().
>
> We do it here because D3 depends on whether the device state was
> saved by the driver.
>
> if (!pci_dev->state_saved) {
> pci_save_state(pci_dev);
>
> /* disable PTM here */
>
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
>
>
> If we disable PTM before the check, we will have saved "PTM
> disabled" as the restore state. And we can't do it after the check
> as the device will be in D3.
Are you suggesting that PTM should be left enabled if the driver
called pci_save_state(), but disabled otherwise? I don't see the
rationale for that.
I don't understand all the paths through pci_pm_suspend_noirq() (e.g.,
skip_bus_pm), but for this one, I think we could do something like
this:
driver_saved = pci_dev->state_saved;
if (!driver_saved)
pci_save_state(pci_dev);
pci_disable_ptm(pci_dev);
if (!driver_saved) {
if (pci_power_manageable(pci_dev))
pci_prepare_to_sleep(pci_dev);
}
Or I guess one could argue that a driver calling pci_save_state() is
implicitly taking responsibility for all PCI-related suspend work, and
it should be disabling PTM itself. But that doesn't really seem
maintainable.
> As to disabling PTM on all devices, I see no problem with this, but the
> reasoning is different. We disabled the root port PTM for power savings.
The power saving is good. I'm trying to make the argument that we
need to disable PTM on all devices for correctness.
If we disable PTM on the root port, are we guaranteed that it will
never receive a PTM Request from a downstream device? Per PCIe r6.0,
sec 6.21.3, such a request would cause an Unsupported Request error.
I sort of expect that if we're putting a root port in a low-power
state, all downstream devices are already in the same or a lower-power
state (but I don't understand PM well enough to be confident).
And I don't really *expect* devices in a low-power state to generate
PTM Requests, but I haven't seen anything in the spec that prohibits
it.
This leads me to believe that if we disable PTM in a root port, we
must first disable PTM in any downstream devices. Otherwise, the root
port may log UR errors if the downstream device issues a PTM Request.
> > When we're putting a device into a low-power state, I think we want to
> > disable PTM *always*, no matter what the driver did. And I think we
> > want to do it for all devices, not just Root Ports.
> >
> > Bjorn
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
2022-04-26 16:50 ` Bjorn Helgaas
@ 2022-04-27 4:22 ` David E. Box
-1 siblings, 0 replies; 17+ messages in thread
From: David E. Box @ 2022-04-27 4:22 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jingar, Rajvi, bhelgaas, linux-pci, linux-kernel, linux-pm,
Wysocki, Rafael J, Kai-Heng Feng, mika.westerberg, koba.ko,
baolu.lu, sathyanarayanan.kuppuswamy, Russell Currey,
Oliver O'Halloran, linuxppc-dev
On Tue, 2022-04-26 at 11:50 -0500, Bjorn Helgaas wrote:
> On Mon, Apr 25, 2022 at 11:32:54AM -0700, David E. Box wrote:
> > On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > > For the PCIe devices (like nvme) that do not go into D3 state
> > > > > > > still
> > > > > > > need to
> > > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > > power PM
> > > > > > > state and the SoC to reach a lower-power idle state as a whole.
> > > > > > > Move
> > > > > > > the
> > > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path
> > > > > > > is
> > > > > > > not
> > > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > > issue
> > > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530
> > > > > > > with
> > > > > > > Coffee
> > > > > > > Lake CPU platforms to get improved residency in low power idle
> > > > > > > states.
> > > > > > >
> > > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save
> > > > > > > power")
> > > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > > *dev)
> > > > > > > if (!pci_dev->state_saved) {
> > > > > > > pci_save_state(pci_dev);
> > > > > > > + /*
> > > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > > since
> > > > > Coffee
> > > > > > > + * Lake) where the power drawn while suspended can be
> > > > > significantly
> > > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > > allows the
> > > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > > reach a
> > > > > > > + * lower-power idle state as a whole.
> > > > > > > + */
> > > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > > + pci_disable_ptm(pci_dev);
> > > > >
> > > > > Why is disabling PTM dependent on pci_dev->state_saved? The
> > > > > point of this is to change the behavior of the device, and it
> > > > > seems like we want to do that regardless of whether the driver
> > > > > has used pci_save_state().
> > > >
> > > > Because we use the saved state to restore PTM on the root port.
> > > > And it's under this condition that the root port state gets
> > > > saved.
> > >
> > > Yes, I understand that pci_restore_ptm_state() depends on a
> > > previous call to pci_save_ptm_state().
> > >
> > > The point I'm trying to make is that pci_disable_ptm() changes the
> > > state of the device, and that state change should not depend on
> > > whether the driver has used pci_save_state().
> >
> > We do it here because D3 depends on whether the device state was
> > saved by the driver.
> >
> > if (!pci_dev->state_saved) {
> > pci_save_state(pci_dev);
> >
> > /* disable PTM here */
> >
> > if (pci_power_manageable(pci_dev))
> > pci_prepare_to_sleep(pci_dev);
> > }
> >
> >
> > If we disable PTM before the check, we will have saved "PTM
> > disabled" as the restore state. And we can't do it after the check
> > as the device will be in D3.
>
> Are you suggesting that PTM should be left enabled if the driver
> called pci_save_state(), but disabled otherwise? I don't see the
> rationale for that.
No. I was saying that because pci_power_manageable() depends on the state not
being saved, we depended on it too ...
>
> I don't understand all the paths through pci_pm_suspend_noirq() (e.g.,
> skip_bus_pm), but for this one, I think we could do something like
> this:
>
> driver_saved = pci_dev->state_saved;
> if (!driver_saved)
> pci_save_state(pci_dev);
>
> pci_disable_ptm(pci_dev);
>
> if (!driver_saved) {
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
... but this solution gets us away from dependency. We'll make this change.
>
> Or I guess one could argue that a driver calling pci_save_state() is
> implicitly taking responsibility for all PCI-related suspend work, and
> it should be disabling PTM itself. But that doesn't really seem
> maintainable.
>
> > As to disabling PTM on all devices, I see no problem with this, but the
> > reasoning is different. We disabled the root port PTM for power savings.
>
> The power saving is good. I'm trying to make the argument that we
> need to disable PTM on all devices for correctness.
>
> If we disable PTM on the root port, are we guaranteed that it will
> never receive a PTM Request from a downstream device? Per PCIe r6.0,
> sec 6.21.3, such a request would cause an Unsupported Request error.
>
> I sort of expect that if we're putting a root port in a low-power
> state, all downstream devices are already in the same or a lower-power
> state (but I don't understand PM well enough to be confident).
>
> And I don't really *expect* devices in a low-power state to generate
> PTM Requests, but I haven't seen anything in the spec that prohibits
> it.
>
> This leads me to believe that if we disable PTM in a root port, we
> must first disable PTM in any downstream devices. Otherwise, the root
> port may log UR errors if the downstream device issues a PTM Request.
I don't know that Kai-Heng's case is due to this, but it's a fair reading of the
spec that downstream devices should be disabled first. We'll change the patch to
disable PTM on all devices. Thanks.
David
>
> > > When we're putting a device into a low-power state, I think we want to
> > > disable PTM *always*, no matter what the driver did. And I think we
> > > want to do it for all devices, not just Root Ports.
> > >
> > > Bjorn
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] PCI/PM: Fix pci_pm_suspend_noirq() to disable PTM
@ 2022-04-27 4:22 ` David E. Box
0 siblings, 0 replies; 17+ messages in thread
From: David E. Box @ 2022-04-27 4:22 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: sathyanarayanan.kuppuswamy, linuxppc-dev, linux-pm, linux-pci,
Wysocki, Rafael J, linux-kernel, koba.ko, Jingar, Rajvi,
Kai-Heng Feng, Oliver O'Halloran, bhelgaas, mika.westerberg,
baolu.lu
On Tue, 2022-04-26 at 11:50 -0500, Bjorn Helgaas wrote:
> On Mon, Apr 25, 2022 at 11:32:54AM -0700, David E. Box wrote:
> > On Sat, 2022-04-23 at 10:01 -0500, Bjorn Helgaas wrote:
> > > On Sat, Apr 23, 2022 at 12:43:14AM +0000, Jingar, Rajvi wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > > On Thu, Apr 14, 2022 at 07:54:02PM +0200, Rafael J. Wysocki wrote:
> > > > > > On 3/25/2022 8:50 PM, Rajvi Jingar wrote:
> > > > > > > For the PCIe devices (like nvme) that do not go into D3 state
> > > > > > > still
> > > > > > > need to
> > > > > > > disable PTM on PCIe root ports to allow the port to enter a lower-
> > > > > > > power PM
> > > > > > > state and the SoC to reach a lower-power idle state as a whole.
> > > > > > > Move
> > > > > > > the
> > > > > > > pci_disable_ptm() out of pci_prepare_to_sleep() as this code path
> > > > > > > is
> > > > > > > not
> > > > > > > followed for devices that do not go into D3. This patch fixes the
> > > > > > > issue
> > > > > > > seen on Dell XPS 9300 with Ice Lake CPU and Dell Precision 5530
> > > > > > > with
> > > > > > > Coffee
> > > > > > > Lake CPU platforms to get improved residency in low power idle
> > > > > > > states.
> > > > > > >
> > > > > > > Fixes: a697f072f5da ("PCI: Disable PTM during suspend to save
> > > > > > > power")
> > > > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@intel.com>
> > > > > > > Suggested-by: David E. Box <david.e.box@linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/pci/pci-driver.c | 10 ++++++++++
> > > > > > > drivers/pci/pci.c | 10 ----------
> > > > > > > 2 files changed, 10 insertions(+), 10 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > > > > > index 8b55a90126a2..ab733374a260 100644
> > > > > > > --- a/drivers/pci/pci-driver.c
> > > > > > > +++ b/drivers/pci/pci-driver.c
> > > > > > > @@ -847,6 +847,16 @@ static int pci_pm_suspend_noirq(struct device
> > > > > > > *dev)
> > > > > > > if (!pci_dev->state_saved) {
> > > > > > > pci_save_state(pci_dev);
> > > > > > > + /*
> > > > > > > + * There are systems (for example, Intel mobile chips
> > > > > > > since
> > > > > Coffee
> > > > > > > + * Lake) where the power drawn while suspended can be
> > > > > significantly
> > > > > > > + * reduced by disabling PTM on PCIe root ports as this
> > > > > > > allows the
> > > > > > > + * port to enter a lower-power PM state and the SoC to
> > > > > > > reach a
> > > > > > > + * lower-power idle state as a whole.
> > > > > > > + */
> > > > > > > + if (pci_pcie_type(pci_dev) == PCI_EXP_TYPE_ROOT_PORT)
> > > > > > > + pci_disable_ptm(pci_dev);
> > > > >
> > > > > Why is disabling PTM dependent on pci_dev->state_saved? The
> > > > > point of this is to change the behavior of the device, and it
> > > > > seems like we want to do that regardless of whether the driver
> > > > > has used pci_save_state().
> > > >
> > > > Because we use the saved state to restore PTM on the root port.
> > > > And it's under this condition that the root port state gets
> > > > saved.
> > >
> > > Yes, I understand that pci_restore_ptm_state() depends on a
> > > previous call to pci_save_ptm_state().
> > >
> > > The point I'm trying to make is that pci_disable_ptm() changes the
> > > state of the device, and that state change should not depend on
> > > whether the driver has used pci_save_state().
> >
> > We do it here because D3 depends on whether the device state was
> > saved by the driver.
> >
> > if (!pci_dev->state_saved) {
> > pci_save_state(pci_dev);
> >
> > /* disable PTM here */
> >
> > if (pci_power_manageable(pci_dev))
> > pci_prepare_to_sleep(pci_dev);
> > }
> >
> >
> > If we disable PTM before the check, we will have saved "PTM
> > disabled" as the restore state. And we can't do it after the check
> > as the device will be in D3.
>
> Are you suggesting that PTM should be left enabled if the driver
> called pci_save_state(), but disabled otherwise? I don't see the
> rationale for that.
No. I was saying that because pci_power_manageable() depends on the state not
being saved, we depended on it too ...
>
> I don't understand all the paths through pci_pm_suspend_noirq() (e.g.,
> skip_bus_pm), but for this one, I think we could do something like
> this:
>
> driver_saved = pci_dev->state_saved;
> if (!driver_saved)
> pci_save_state(pci_dev);
>
> pci_disable_ptm(pci_dev);
>
> if (!driver_saved) {
> if (pci_power_manageable(pci_dev))
> pci_prepare_to_sleep(pci_dev);
> }
... but this solution gets us away from dependency. We'll make this change.
>
> Or I guess one could argue that a driver calling pci_save_state() is
> implicitly taking responsibility for all PCI-related suspend work, and
> it should be disabling PTM itself. But that doesn't really seem
> maintainable.
>
> > As to disabling PTM on all devices, I see no problem with this, but the
> > reasoning is different. We disabled the root port PTM for power savings.
>
> The power saving is good. I'm trying to make the argument that we
> need to disable PTM on all devices for correctness.
>
> If we disable PTM on the root port, are we guaranteed that it will
> never receive a PTM Request from a downstream device? Per PCIe r6.0,
> sec 6.21.3, such a request would cause an Unsupported Request error.
>
> I sort of expect that if we're putting a root port in a low-power
> state, all downstream devices are already in the same or a lower-power
> state (but I don't understand PM well enough to be confident).
>
> And I don't really *expect* devices in a low-power state to generate
> PTM Requests, but I haven't seen anything in the spec that prohibits
> it.
>
> This leads me to believe that if we disable PTM in a root port, we
> must first disable PTM in any downstream devices. Otherwise, the root
> port may log UR errors if the downstream device issues a PTM Request.
I don't know that Kai-Heng's case is due to this, but it's a fair reading of the
spec that downstream devices should be disabled first. We'll change the patch to
disable PTM on all devices. Thanks.
David
>
> > > When we're putting a device into a low-power state, I think we want to
> > > disable PTM *always*, no matter what the driver did. And I think we
> > > want to do it for all devices, not just Root Ports.
> > >
> > > Bjorn
^ permalink raw reply [flat|nested] 17+ messages in thread