[PATCH v1 00/11] PCI/PM: Rework powering up PCI devices

[PATCH v1 00/11] PCI/PM: Rework powering up PCI devices

[Rafael J. Wysocki]

Hi All,

This patch set replaces patches [4-9/9] from the series at

https://lore.kernel.org/linux-pm/4419002.LvFx2qVVIh@kreacher/T/#mf7ed30e7cf114b131e6067e4e10c28e59d661cb4

which had to be dropped, because they were problematic:

https://lore.kernel.org/linux-pm/4419002.LvFx2qVVIh@kreacher/T/#ma71172f00a95708f0cd4d21741bcc248d394caf1

This time there are more patches making smaller changes each and I've done my
best to avoid making any changes without a good enough motivation.

Please refer to the patch changelogs for details.

Thanks!

[PATCH v1 01/11] PCI/PM: Split pci_raw_set_power_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The transitions from low-power states to D0 and the other way around
are unnecessarily tangled in pci_raw_set_power_state() which makes it
rather hard to follow.

Moreover, the only caller of pci_raw_set_power_state() passing PCI_D0
as its state argument is pci_power_up(), so the code carrying out
transitions into D0 can be put directly into that function.

Accordingly, move the code handling transitions from low-power states
into D0 directly into pci_power_up() and rename the remaining part
of pci_raw_set_power_state() to pci_set_low_power_state(), because
it only handles transitions into low-power state now.

While at it, fix up some white space, update some comments and modify
messages printed by pci_power_up() and pci_set_low_power_state() to
be less confusing (which is the only expected functional impact of
this change).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |  143 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 83 insertions(+), 60 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1068,10 +1068,11 @@ static inline bool platform_pci_bridge_d
 }
 
 /**
- * pci_raw_set_power_state - Use PCI PM registers to set the power state of
- *			     given PCI device
+ * pci_set_low_power_state - Put a PCI device into a low-power state.
  * @dev: PCI device to handle.
- * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.
+ * @state: PCI power state (D1, D2, D3hot) to put the device into.
+ *
+ * Use the device's PCI_PM_CTRL register to put it into a low-power state.
  *
  * RETURN VALUE:
  * -EINVAL if the requested state is invalid.
@@ -1080,10 +1081,9 @@ static inline bool platform_pci_bridge_d
  * 0 if device already is in the requested state.
  * 0 if device's power state has been successfully changed.
  */
-static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
+static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state)
 {
 	u16 pmcsr;
-	bool need_restore = false;
 
 	/* Check if we're already there */
 	if (dev->current_state == state)
@@ -1092,7 +1092,7 @@ static int pci_raw_set_power_state(struc
 	if (!dev->pm_cap)
 		return -EIO;
 
-	if (state < PCI_D0 || state > PCI_D3hot)
+	if (state < PCI_D1 || state > PCI_D3hot)
 		return -EINVAL;
 
 	/*
@@ -1101,8 +1101,7 @@ static int pci_raw_set_power_state(struc
 	 * we can go from D1 to D3, but we can't go directly from D3 to D1;
 	 * we'd have to go from D3 to D0, then to D1.
 	 */
-	if (state != PCI_D0 && dev->current_state <= PCI_D3cold
-	    && dev->current_state > state) {
+	if (dev->current_state <= PCI_D3cold && dev->current_state > state) {
 		pci_err(dev, "invalid power transition (from %s to %s)\n",
 			pci_power_name(dev->current_state),
 			pci_power_name(state));
@@ -1116,70 +1115,30 @@ static int pci_raw_set_power_state(struc
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	if (PCI_POSSIBLE_ERROR(pmcsr)) {
-		pci_err(dev, "can't change power state from %s to %s (config space inaccessible)\n",
+		pci_err(dev, "Unable to change power state from %s to %s, device inaccessible\n",
 			pci_power_name(dev->current_state),
 			pci_power_name(state));
 		return -EIO;
 	}
 
-	/*
-	 * If we're (effectively) in D3, force entire word to 0.
-	 * This doesn't affect PME_Status, disables PME_En, and
-	 * sets PowerState to 0.
-	 */
-	switch (dev->current_state) {
-	case PCI_D0:
-	case PCI_D1:
-	case PCI_D2:
-		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
-		pmcsr |= state;
-		break;
-	case PCI_D3hot:
-	case PCI_D3cold:
-	case PCI_UNKNOWN: /* Boot-up */
-		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot
-		 && !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
-			need_restore = true;
-		fallthrough;	/* force to D0 */
-	default:
-		pmcsr = 0;
-		break;
-	}
+	pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
+	pmcsr |= state;
 
 	/* Enter specified state */
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 
-	/*
-	 * Mandatory power management transition delays; see PCI PM 1.1
-	 * 5.6.1 table 18
-	 */
-	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
+	/* Mandatory power management transition delays; see PCI PM 1.2. */
+	if (state == PCI_D3hot)
 		pci_dev_d3_sleep(dev);
-	else if (state == PCI_D2 || dev->current_state == PCI_D2)
+	else if (state == PCI_D2)
 		udelay(PCI_PM_D2_DELAY);
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
-	dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
+	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
 	if (dev->current_state != state)
-		pci_info_ratelimited(dev, "refused to change power state from %s to %s\n",
-			 pci_power_name(dev->current_state),
-			 pci_power_name(state));
-
-	/*
-	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
-	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
-	 * from D3hot to D0 _may_ perform an internal reset, thereby
-	 * going to "D0 Uninitialized" rather than "D0 Initialized".
-	 * For example, at least some versions of the 3c905B and the
-	 * 3c556B exhibit this behaviour.
-	 *
-	 * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
-	 * devices in a D3hot state at boot.  Consequently, we need to
-	 * restore at least the BARs so that the device will be
-	 * accessible to its driver.
-	 */
-	if (need_restore)
-		pci_restore_bars(dev);
+		pci_info_ratelimited(dev, "Refused to change power state from %s to %s\n",
+				     pci_power_name(dev->current_state),
+				     pci_power_name(state));
 
 	if (dev->bus->self)
 		pcie_aspm_pm_state_change(dev->bus->self);
@@ -1312,8 +1271,72 @@ static int pci_dev_wait(struct pci_dev *
  */
 int pci_power_up(struct pci_dev *dev)
 {
+	bool need_restore = false;
+	u16 pmcsr;
+
 	pci_platform_power_transition(dev, PCI_D0);
-	return pci_raw_set_power_state(dev, PCI_D0);
+
+	if (dev->current_state == PCI_D0)
+		return 0;
+
+	if (!dev->pm_cap)
+		return -EIO;
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+	if (PCI_POSSIBLE_ERROR(pmcsr)) {
+		pci_err(dev, "Unable to change power state from %s to D0, device inaccessible\n",
+			pci_power_name(dev->current_state));
+		return -EIO;
+	}
+
+	/*
+	 * If we're (effectively) in D3, force entire word to 0. This doesn't
+	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
+	 */
+	if (dev->current_state >= PCI_D3hot) {
+		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot &&
+		    !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
+			need_restore = true;
+
+		pmcsr = 0;
+	} else {
+		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
+	}
+
+	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
+
+	/* Mandatory transition delays; see PCI PM 1.2. */
+	if (dev->current_state == PCI_D3hot)
+		pci_dev_d3_sleep(dev);
+	else if (dev->current_state == PCI_D2)
+		udelay(PCI_PM_D2_DELAY);
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
+	if (dev->current_state != PCI_D0)
+		pci_info_ratelimited(dev, "Refused to change power state from %s to D0\n",
+				     pci_power_name(dev->current_state));
+
+	/*
+	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
+	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
+	 * from D3hot to D0 _may_ perform an internal reset, thereby
+	 * going to "D0 Uninitialized" rather than "D0 Initialized".
+	 * For example, at least some versions of the 3c905B and the
+	 * 3c556B exhibit this behaviour.
+	 *
+	 * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
+	 * devices in a D3hot state at boot.  Consequently, we need to
+	 * restore at least the BARs so that the device will be
+	 * accessible to its driver.
+	 */
+	if (need_restore)
+		pci_restore_bars(dev);
+
+	if (dev->bus->self)
+		pcie_aspm_pm_state_change(dev->bus->self);
+
+	return 0;
 }
 
 /**
@@ -1394,7 +1417,7 @@ int pci_set_power_state(struct pci_dev *
 	 * To put device in D3cold, we put device into D3hot in native
 	 * way, then put device into D3cold with platform ops
 	 */
-	error = pci_raw_set_power_state(dev, state > PCI_D3hot ?
+	error = pci_set_low_power_state(dev, state > PCI_D3hot ?
 					PCI_D3hot : state);
 
 	if (pci_platform_power_transition(dev, state))

[PATCH v1 02/11] PCI/PM: Relocate pci_set_low_power_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because pci_set_power_state() is the only caller of
pci_set_low_power_state(), put the latter next to the former.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |  158 +++++++++++++++++++++++++++---------------------------
 1 file changed, 79 insertions(+), 79 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1068,85 +1068,6 @@ static inline bool platform_pci_bridge_d
 }
 
 /**
- * pci_set_low_power_state - Put a PCI device into a low-power state.
- * @dev: PCI device to handle.
- * @state: PCI power state (D1, D2, D3hot) to put the device into.
- *
- * Use the device's PCI_PM_CTRL register to put it into a low-power state.
- *
- * RETURN VALUE:
- * -EINVAL if the requested state is invalid.
- * -EIO if device does not support PCI PM or its PM capabilities register has a
- * wrong version, or device doesn't support the requested state.
- * 0 if device already is in the requested state.
- * 0 if device's power state has been successfully changed.
- */
-static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state)
-{
-	u16 pmcsr;
-
-	/* Check if we're already there */
-	if (dev->current_state == state)
-		return 0;
-
-	if (!dev->pm_cap)
-		return -EIO;
-
-	if (state < PCI_D1 || state > PCI_D3hot)
-		return -EINVAL;
-
-	/*
-	 * Validate transition: We can enter D0 from any state, but if
-	 * we're already in a low-power state, we can only go deeper.  E.g.,
-	 * we can go from D1 to D3, but we can't go directly from D3 to D1;
-	 * we'd have to go from D3 to D0, then to D1.
-	 */
-	if (dev->current_state <= PCI_D3cold && dev->current_state > state) {
-		pci_err(dev, "invalid power transition (from %s to %s)\n",
-			pci_power_name(dev->current_state),
-			pci_power_name(state));
-		return -EINVAL;
-	}
-
-	/* Check if this device supports the desired state */
-	if ((state == PCI_D1 && !dev->d1_support)
-	   || (state == PCI_D2 && !dev->d2_support))
-		return -EIO;
-
-	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
-	if (PCI_POSSIBLE_ERROR(pmcsr)) {
-		pci_err(dev, "Unable to change power state from %s to %s, device inaccessible\n",
-			pci_power_name(dev->current_state),
-			pci_power_name(state));
-		return -EIO;
-	}
-
-	pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
-	pmcsr |= state;
-
-	/* Enter specified state */
-	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
-
-	/* Mandatory power management transition delays; see PCI PM 1.2. */
-	if (state == PCI_D3hot)
-		pci_dev_d3_sleep(dev);
-	else if (state == PCI_D2)
-		udelay(PCI_PM_D2_DELAY);
-
-	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
-	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
-	if (dev->current_state != state)
-		pci_info_ratelimited(dev, "Refused to change power state from %s to %s\n",
-				     pci_power_name(dev->current_state),
-				     pci_power_name(state));
-
-	if (dev->bus->self)
-		pcie_aspm_pm_state_change(dev->bus->self);
-
-	return 0;
-}
-
-/**
  * pci_update_current_state - Read power state of given device and cache it
  * @dev: PCI device to handle.
  * @state: State to cache in case the device doesn't have the PM capability
@@ -1364,6 +1285,85 @@ void pci_bus_set_current_state(struct pc
 }
 
 /**
+ * pci_set_low_power_state - Put a PCI device into a low-power state.
+ * @dev: PCI device to handle.
+ * @state: PCI power state (D1, D2, D3hot) to put the device into.
+ *
+ * Use the device's PCI_PM_CTRL register to put it into a low-power state.
+ *
+ * RETURN VALUE:
+ * -EINVAL if the requested state is invalid.
+ * -EIO if device does not support PCI PM or its PM capabilities register has a
+ * wrong version, or device doesn't support the requested state.
+ * 0 if device already is in the requested state.
+ * 0 if device's power state has been successfully changed.
+ */
+static int pci_set_low_power_state(struct pci_dev *dev, pci_power_t state)
+{
+	u16 pmcsr;
+
+	/* Check if we're already there */
+	if (dev->current_state == state)
+		return 0;
+
+	if (!dev->pm_cap)
+		return -EIO;
+
+	if (state < PCI_D1 || state > PCI_D3hot)
+		return -EINVAL;
+
+	/*
+	 * Validate transition: We can enter D0 from any state, but if
+	 * we're already in a low-power state, we can only go deeper.  E.g.,
+	 * we can go from D1 to D3, but we can't go directly from D3 to D1;
+	 * we'd have to go from D3 to D0, then to D1.
+	 */
+	if (dev->current_state <= PCI_D3cold && dev->current_state > state) {
+		pci_err(dev, "invalid power transition (from %s to %s)\n",
+			pci_power_name(dev->current_state),
+			pci_power_name(state));
+		return -EINVAL;
+	}
+
+	/* Check if this device supports the desired state */
+	if ((state == PCI_D1 && !dev->d1_support)
+	   || (state == PCI_D2 && !dev->d2_support))
+		return -EIO;
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+	if (PCI_POSSIBLE_ERROR(pmcsr)) {
+		pci_err(dev, "Unable to change power state from %s to %s, device inaccessible\n",
+			pci_power_name(dev->current_state),
+			pci_power_name(state));
+		return -EIO;
+	}
+
+	pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
+	pmcsr |= state;
+
+	/* Enter specified state */
+	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
+
+	/* Mandatory power management transition delays; see PCI PM 1.2. */
+	if (state == PCI_D3hot)
+		pci_dev_d3_sleep(dev);
+	else if (state == PCI_D2)
+		udelay(PCI_PM_D2_DELAY);
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
+	if (dev->current_state != state)
+		pci_info_ratelimited(dev, "Refused to change power state from %s to %s\n",
+				     pci_power_name(dev->current_state),
+				     pci_power_name(state));
+
+	if (dev->bus->self)
+		pcie_aspm_pm_state_change(dev->bus->self);
+
+	return 0;
+}
+
+/**
  * pci_set_power_state - Set the power state of a PCI device
  * @dev: PCI device to handle.
  * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.

[PATCH v1 03/11] PCI/PM: Set current_state to D3cold if the device is not accessible

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make pci_power_up() and pci_set_low_power_state() change current_state
to PCI_D3cold when the device is not accessible along the lines of
pci_update_current_state().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1207,6 +1207,7 @@ int pci_power_up(struct pci_dev *dev)
 	if (PCI_POSSIBLE_ERROR(pmcsr)) {
 		pci_err(dev, "Unable to change power state from %s to D0, device inaccessible\n",
 			pci_power_name(dev->current_state));
+		dev->current_state = PCI_D3cold;
 		return -EIO;
 	}
 
@@ -1335,6 +1336,7 @@ static int pci_set_low_power_state(struc
 		pci_err(dev, "Unable to change power state from %s to %s, device inaccessible\n",
 			pci_power_name(dev->current_state),
 			pci_power_name(state));
+		dev->current_state = PCI_D3cold;
 		return -EIO;
 	}
 

[PATCH v1 04/11] PCI/PM: Unfold pci_platform_power_transition() in pci_power_up()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some actions carried out by pci_platform_power_transition(() in
pci_power_up() are redundant, but before dealing with them, make
pci_power_up() call the pci_platform_power_transition() code directly
(and avoid a redundant check when pm_cap is unset while at it).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1194,8 +1194,15 @@ int pci_power_up(struct pci_dev *dev)
 {
 	bool need_restore = false;
 	u16 pmcsr;
+	int ret;
 
-	pci_platform_power_transition(dev, PCI_D0);
+	ret = platform_pci_set_power_state(dev, PCI_D0);
+	if (!ret) {
+		pci_update_current_state(dev, PCI_D0);
+	} else if (!dev->pm_cap) { /* Fall back to PCI_D0 */
+		dev->current_state = PCI_D0;
+		return 0;
+	}
 
 	if (dev->current_state == PCI_D0)
 		return 0;

[PATCH v1 05/11] PCI/PM: Do not call pci_update_current_state() from pci_power_up()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that calling pci_update_current_state() from pci_power_up() is
redundant and may be harmful in some cases.

First, if the device is in a low-power state before pci_power_up()
gets called for it and platform_pci_set_power_state() successfully
changes its power state to D0, pci_update_current_state() will update
current_state to reflect that and pci_power_up() will return success
right away without restoring the device's BARs or reconfiguring ASPM
which may be necessary.  This is arguably incorrect and definitely
inconsistent with the case when platform_pci_set_power_state() returns
an error (for example, because the device is not power-manageable by
the platform firmware).

Second, current_state should not be overwritten until the decision
whether or not to restore the device's BARs is made, because that
decision generally depends on its value.  Again, calling
pci_update_current_state() in pci_power_up() is not consistent with
this observation.

Next, pci_power_up() attempts to read from the device's PCI_PM_CTRL
register regardless of the current_state value unless it is PCI_D0,
including the case when pci_update_current_state() sets current_state
to PCI_D3cold to indicate that the device is not accessible.  If the
register read is not successful, current_state will be set to
PCI_D3cold anyway, so that pci_update_current_state() action is
redundant.

Further, if pci_update_current_state() reads the device's PCI_PM_CTRL
register, pci_power_up() will repeat that read going forward and
it is not necessary to update current_state in the meantime.

Finally, if pm_cap is not set (in which case the PCI_PM_CTRL register
is not present), the power state of the device should be determined
with the help of the platform firmware or set to D0 if that's not
possible and pci_update_current_state() does not do that.

Accordingly, rearrange pci_power_up() so as to address the above
shortcomings.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1192,23 +1192,24 @@ static int pci_dev_wait(struct pci_dev *
  */
 int pci_power_up(struct pci_dev *dev)
 {
-	bool need_restore = false;
+	bool need_restore;
+	pci_power_t state;
 	u16 pmcsr;
-	int ret;
 
-	ret = platform_pci_set_power_state(dev, PCI_D0);
-	if (!ret) {
-		pci_update_current_state(dev, PCI_D0);
-	} else if (!dev->pm_cap) { /* Fall back to PCI_D0 */
-		dev->current_state = PCI_D0;
-		return 0;
-	}
+	platform_pci_set_power_state(dev, PCI_D0);
+
+	if (!dev->pm_cap) {
+		state = platform_pci_get_power_state(dev);
+		if (state == PCI_UNKNOWN)
+			dev->current_state = PCI_D0;
+		else
+			dev->current_state = state;
 
-	if (dev->current_state == PCI_D0)
-		return 0;
+		if (state == PCI_D0)
+			return 0;
 
-	if (!dev->pm_cap)
 		return -EIO;
+	}
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	if (PCI_POSSIBLE_ERROR(pmcsr)) {
@@ -1218,26 +1219,31 @@ int pci_power_up(struct pci_dev *dev)
 		return -EIO;
 	}
 
+	state = pmcsr & PCI_PM_CTRL_STATE_MASK;
+
+	need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) &&
+			!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET);
+
+	if (state == PCI_D0) {
+		dev->current_state = PCI_D0;
+		goto end;
+	}
+
 	/*
 	 * If we're (effectively) in D3, force entire word to 0. This doesn't
 	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
 	 */
-	if (dev->current_state >= PCI_D3hot) {
-		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) == PCI_D3hot &&
-		    !(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET))
-			need_restore = true;
-
+	if (state == PCI_D3hot)
 		pmcsr = 0;
-	} else {
+	else
 		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
-	}
 
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 
 	/* Mandatory transition delays; see PCI PM 1.2. */
-	if (dev->current_state == PCI_D3hot)
+	if (state == PCI_D3hot)
 		pci_dev_d3_sleep(dev);
-	else if (dev->current_state == PCI_D2)
+	else if (state == PCI_D2)
 		udelay(PCI_PM_D2_DELAY);
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
@@ -1246,6 +1252,7 @@ int pci_power_up(struct pci_dev *dev)
 		pci_info_ratelimited(dev, "Refused to change power state from %s to D0\n",
 				     pci_power_name(dev->current_state));
 
+end:
 	/*
 	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
 	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning

[PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
order to put it into D0 regardless of the power state returned by
the previous read from that register which should not matter.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
 	}
 
 	/*
-	 * If we're (effectively) in D3, force entire word to 0. This doesn't
-	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
+	 * Force the entire word to 0. This doesn't affect PME_Status, disables
+	 * PME_En, and sets PowerState to 0.
 	 */
-	if (state == PCI_D3hot)
-		pmcsr = 0;
-	else
-		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
-
-	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
+	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
 
 	/* Mandatory transition delays; see PCI PM 1.2. */
 	if (state == PCI_D3hot)

[PATCH v1 07/11] PCI/PM: Split pci_power_up()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

One of the two callers of pci_power_up() invokes
pci_update_current_state() and pci_restore_state() right after calling
it, in which case running the part of it happening after the mandatory
transition delays is redundant, so move that part out of it into a new
function called pci_set_full_power_state() that will be invoked from
pci_set_power_state() which is the other caller of pci_power_up().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1189,6 +1189,9 @@ static int pci_dev_wait(struct pci_dev *
 /**
  * pci_power_up - Put the given device into D0
  * @dev: PCI device to power up
+ *
+ * On success, return 0 or 1, depending on whether or not it is necessary to
+ * restore the device's BARs subsequently (1 is returned in that case).
  */
 int pci_power_up(struct pci_dev *dev)
 {
@@ -1224,10 +1227,8 @@ int pci_power_up(struct pci_dev *dev)
 	need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) &&
 			!(pmcsr & PCI_PM_CTRL_NO_SOFT_RESET);
 
-	if (state == PCI_D0) {
-		dev->current_state = PCI_D0;
+	if (state == PCI_D0)
 		goto end;
-	}
 
 	/*
 	 * Force the entire word to 0. This doesn't affect PME_Status, disables
@@ -1241,13 +1242,41 @@ int pci_power_up(struct pci_dev *dev)
 	else if (state == PCI_D2)
 		udelay(PCI_PM_D2_DELAY);
 
+end:
+	dev->current_state = PCI_D0;
+	if (need_restore)
+		return 1;
+
+	return 0;
+}
+
+/**
+ * pci_set_full_power_state - Put a PCI device into D0 and update its state
+ * @dev: PCI device to power up
+ *
+ * Call pci_power_up() to put @dev into D0, read from its PCI_PM_CTRL register
+ * to confirm the state change, restore its BARs if they might be lost and
+ * reconfigure ASPM in acordance with the new power state.
+ *
+ * If pci_restore_state() is going to be called right after a power state change
+ * to D0, it is more efficient to use pci_power_up() directly instead of this
+ * function.
+ */
+static int pci_set_full_power_state(struct pci_dev *dev)
+{
+	u16 pmcsr;
+	int ret;
+
+	ret = pci_power_up(dev);
+	if (ret < 0)
+		return ret;
+
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
 	if (dev->current_state != PCI_D0)
 		pci_info_ratelimited(dev, "Refused to change power state from %s to D0\n",
 				     pci_power_name(dev->current_state));
 
-end:
 	/*
 	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
 	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
@@ -1261,7 +1290,7 @@ end:
 	 * restore at least the BARs so that the device will be
 	 * accessible to its driver.
 	 */
-	if (need_restore)
+	if (ret > 0)
 		pci_restore_bars(dev);
 
 	if (dev->bus->self)
@@ -1415,7 +1444,7 @@ int pci_set_power_state(struct pci_dev *
 		return 0;
 
 	if (state == PCI_D0)
-		return pci_power_up(dev);
+		return pci_set_full_power_state(dev);
 
 	/*
 	 * This device is quirked not to be put into D3, so don't put it in

[PATCH v1 08/11] PCI/PM: Do not restore BARs if device is not in D0

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Do not attempt to restore the device's BARs in
pci_set_full_power_state() if the actual current
power state of the device is not D0.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1273,25 +1273,25 @@ static int pci_set_full_power_state(stru
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
-	if (dev->current_state != PCI_D0)
+	if (dev->current_state != PCI_D0) {
 		pci_info_ratelimited(dev, "Refused to change power state from %s to D0\n",
 				     pci_power_name(dev->current_state));
-
-	/*
-	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
-	 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
-	 * from D3hot to D0 _may_ perform an internal reset, thereby
-	 * going to "D0 Uninitialized" rather than "D0 Initialized".
-	 * For example, at least some versions of the 3c905B and the
-	 * 3c556B exhibit this behaviour.
-	 *
-	 * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
-	 * devices in a D3hot state at boot.  Consequently, we need to
-	 * restore at least the BARs so that the device will be
-	 * accessible to its driver.
-	 */
-	if (ret > 0)
+	} else if (ret > 0) {
+		/*
+		 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
+		 * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
+		 * from D3hot to D0 _may_ perform an internal reset, thereby
+		 * going to "D0 Uninitialized" rather than "D0 Initialized".
+		 * For example, at least some versions of the 3c905B and the
+		 * 3c556B exhibit this behaviour.
+		 *
+		 * At least some laptop BIOSen (e.g. the Thinkpad T21) leave
+		 * devices in a D3hot state at boot.  Consequently, we need to
+		 * restore at least the BARs so that the device will be
+		 * accessible to its driver.
+		 */
 		pci_restore_bars(dev);
+	}
 
 	if (dev->bus->self)
 		pcie_aspm_pm_state_change(dev->bus->self);

[PATCH v1 09/11] PCI/PM: Clean up pci_set_low_power_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the following assorted non-essential changes in
pci_set_low_power_state():

 1. Drop two redundant checks from it (the caller takes care of these
    conditions).

 2. Change the log level of a messages printed by it to "debug",
    because it only indicates a programming mistake.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1341,16 +1341,9 @@ static int pci_set_low_power_state(struc
 {
 	u16 pmcsr;
 
-	/* Check if we're already there */
-	if (dev->current_state == state)
-		return 0;
-
 	if (!dev->pm_cap)
 		return -EIO;
 
-	if (state < PCI_D1 || state > PCI_D3hot)
-		return -EINVAL;
-
 	/*
 	 * Validate transition: We can enter D0 from any state, but if
 	 * we're already in a low-power state, we can only go deeper.  E.g.,
@@ -1358,7 +1351,7 @@ static int pci_set_low_power_state(struc
 	 * we'd have to go from D3 to D0, then to D1.
 	 */
 	if (dev->current_state <= PCI_D3cold && dev->current_state > state) {
-		pci_err(dev, "invalid power transition (from %s to %s)\n",
+		pci_dbg(dev, "Invalid power transition (from %s to %s)\n",
 			pci_power_name(dev->current_state),
 			pci_power_name(state));
 		return -EINVAL;

[PATCH v1 10/11] PCI/PM: Rearrange pci_set_power_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The part of pci_set_power_state() related to transitions into
low-power states is unnecessary convoluted, so clearly divide it
into the D3cold special case and the general case covering all of
the other states.

Also fix a potential issue with calling pci_bus_set_current_state()
to set the current state of all devices on the subordinate bus to
D3cold without checking if the power state of the parent bridge has
really changed to D3cold.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pci/pci.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -1446,19 +1446,25 @@ int pci_set_power_state(struct pci_dev *
 	if (state >= PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
-	/*
-	 * To put device in D3cold, we put device into D3hot in native
-	 * way, then put device into D3cold with platform ops
-	 */
-	error = pci_set_low_power_state(dev, state > PCI_D3hot ?
-					PCI_D3hot : state);
+	if (state == PCI_D3cold) {
+		/*
+		 * To put the device in D3cold, put it into D3hot in the native
+		 * way, then put it into D3cold using platform ops.
+		 */
+		error = pci_set_low_power_state(dev, PCI_D3hot);
 
-	if (pci_platform_power_transition(dev, state))
-		return error;
+		if (pci_platform_power_transition(dev, PCI_D3cold))
+			return error;
 
-	/* Powering off a bridge may power off the whole hierarchy */
-	if (state == PCI_D3cold)
-		pci_bus_set_current_state(dev->subordinate, PCI_D3cold);
+		/* Powering off a bridge may power off the whole hierarchy */
+		if (dev->current_state == PCI_D3cold)
+			pci_bus_set_current_state(dev->subordinate, PCI_D3cold);
+	} else {
+		error = pci_set_low_power_state(dev, state);
+
+		if (pci_platform_power_transition(dev, state))
+			return error;
+	}
 
 	return 0;
 }

[PATCH v1 11/11] PCI/PM: Replace pci_set_power_state() in pci_pm_thaw_noirq()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Calling pci_set_power_state() to put the given device into D0 in
pci_pm_thaw_noirq() may cause it to restore the device's BARs, which
is redundant before calling pci_restore_state(), so replace it with
a direct pci_power_up() call followed by pci_update_current_state()
if it returns a nonzeor value, in analogy with
pci_pm_default_resume_early().

Avoid code duplication by introducing a wrapper function to contain
the repeating pattern and calling it in both places.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -551,10 +551,15 @@ static void pci_pm_default_resume(struct
 	pci_enable_wake(pci_dev, PCI_D0, false);
 }
 
-static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
+static void pci_pm_power_up_and_verify_state(struct pci_dev *pci_dev)
 {
 	pci_power_up(pci_dev);
 	pci_update_current_state(pci_dev, PCI_D0);
+}
+
+static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
+{
+	pci_pm_power_up_and_verify_state(pci_dev);
 	pci_restore_state(pci_dev);
 	pci_pme_restore(pci_dev);
 }
@@ -1079,7 +1084,7 @@ static int pci_pm_thaw_noirq(struct devi
 	 * in case the driver's "freeze" callbacks put it into a low-power
 	 * state.
 	 */
-	pci_set_power_state(pci_dev, PCI_D0);
+	pci_pm_power_up_and_verify_state(pci_dev);
 	pci_restore_state(pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))

Re: [PATCH v1 00/11] PCI/PM: Rework powering up PCI devices

[Bjorn Helgaas]

[+cc Krzysztof, just FYI]

On Thu, May 05, 2022 at 07:57:15PM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> This patch set replaces patches [4-9/9] from the series at
> 
> https://lore.kernel.org/linux-pm/4419002.LvFx2qVVIh@kreacher/T/#mf7ed30e7cf114b131e6067e4e10c28e59d661cb4

I applied this to pci/pm, thanks!

I reordered the branch so Krzysztof's "pci_restore_standard_config()
defined but not used" fix is first, followed by your changes, Rafael.

Bjorn

Re: [PATCH v1 00/11] PCI/PM: Rework powering up PCI devices

[Nathan Chancellor]

On Thu, May 05, 2022 at 02:22:41PM -0500, Bjorn Helgaas wrote:
> [+cc Krzysztof, just FYI]
> 
> On Thu, May 05, 2022 at 07:57:15PM +0200, Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > This patch set replaces patches [4-9/9] from the series at
> > 
> > https://lore.kernel.org/linux-pm/4419002.LvFx2qVVIh@kreacher/T/#mf7ed30e7cf114b131e6067e4e10c28e59d661cb4
> 
> I applied this to pci/pm, thanks!
> 
> I reordered the branch so Krzysztof's "pci_restore_standard_config()
> defined but not used" fix is first, followed by your changes, Rafael.

I can confirm that at commit 0f40ac35e4ec ("PCI/PM: Replace
pci_set_power_state() in pci_pm_thaw_noirq()"), the problematic machine
boots up with no visible issues. Thanks a lot for the quick fixes!

Cheers,
Nathan

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Bjorn Helgaas]

On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> order to put it into D0 regardless of the power state returned by
> the previous read from that register which should not matter.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pci.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
>  	}
>  
>  	/*
> -	 * If we're (effectively) in D3, force entire word to 0. This doesn't
> -	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
> +	 * Force the entire word to 0. This doesn't affect PME_Status, disables
> +	 * PME_En, and sets PowerState to 0.
>  	 */
> -	if (state == PCI_D3hot)
> -		pmcsr = 0;
> -	else
> -		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> -
> -	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> +	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);

Can you reassure me why this is safe and useful?

This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):

  0x0003 PowerState     RW
  0x0004                RsvdP
  0x0008 No_Soft_Reset  RO
  0x00f0                RsvdP
  0x0100 PME_En         RW/RWS
  0x1e00 Data_Select    RW, VF ROZ
  0x6000 Data_Scale     RO, VF ROZ
  0x8000 PME_Status     RW1CS

We intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, and PME_Status is "write 1 to clear" to writing 0 does
nothing, so those look OK.

But the RsvdP fields are reserved for future RW bits and should be
preserved, and it looks like clearing Data_Select could potentially
break the Data Register power consumption reporting (which I don't
think we support today).

It seems like maybe we should do this instead:

  pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
                        pmcsr & ~PCI_PM_CTRL_STATE_MASK)

to just unconditionally clear PowerState?

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Bjorn Helgaas]

On Thu, May 26, 2022 at 11:54:22AM -0500, Bjorn Helgaas wrote:
> On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> > order to put it into D0 regardless of the power state returned by
> > the previous read from that register which should not matter.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/pci/pci.c |   11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > Index: linux-pm/drivers/pci/pci.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci.c
> > +++ linux-pm/drivers/pci/pci.c
> > @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
> >  	}
> >  
> >  	/*
> > -	 * If we're (effectively) in D3, force entire word to 0. This doesn't
> > -	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
> > +	 * Force the entire word to 0. This doesn't affect PME_Status, disables
> > +	 * PME_En, and sets PowerState to 0.
> >  	 */
> > -	if (state == PCI_D3hot)
> > -		pmcsr = 0;
> > -	else
> > -		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > -
> > -	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > +	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> 
> Can you reassure me why this is safe and useful?
> 
> This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
> 
>   0x0003 PowerState     RW
>   0x0004                RsvdP
>   0x0008 No_Soft_Reset  RO
>   0x00f0                RsvdP
>   0x0100 PME_En         RW/RWS
>   0x1e00 Data_Select    RW, VF ROZ
>   0x6000 Data_Scale     RO, VF ROZ
>   0x8000 PME_Status     RW1CS
> 
> We intend to set PowerState to 0 (D0), apparently intend to clear
> PME_En, and PME_Status is "write 1 to clear" to writing 0 does
> nothing, so those look OK.
> 
> But the RsvdP fields are reserved for future RW bits and should be
> preserved, and it looks like clearing Data_Select could potentially
> break the Data Register power consumption reporting (which I don't
> think we support today).
> 
> It seems like maybe we should do this instead:
> 
>   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
>                         pmcsr & ~PCI_PM_CTRL_STATE_MASK)
> 
> to just unconditionally clear PowerState?

Or I guess this, since we want to clear PME_En as well?

  pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
                        ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Rafael J. Wysocki]

On Thu, May 26, 2022 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, May 26, 2022 at 11:54:22AM -0500, Bjorn Helgaas wrote:
> > On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> > > order to put it into D0 regardless of the power state returned by
> > > the previous read from that register which should not matter.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/pci/pci.c |   11 +++--------
> > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > >
> > > Index: linux-pm/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/pci/pci.c
> > > +++ linux-pm/drivers/pci/pci.c
> > > @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
> > >     }
> > >
> > >     /*
> > > -    * If we're (effectively) in D3, force entire word to 0. This doesn't
> > > -    * affect PME_Status, disables PME_En, and sets PowerState to 0.
> > > +    * Force the entire word to 0. This doesn't affect PME_Status, disables
> > > +    * PME_En, and sets PowerState to 0.
> > >      */
> > > -   if (state == PCI_D3hot)
> > > -           pmcsr = 0;
> > > -   else
> > > -           pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > -
> > > -   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > > +   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> >
> > Can you reassure me why this is safe and useful?
> >
> > This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
> >
> >   0x0003 PowerState     RW
> >   0x0004                RsvdP
> >   0x0008 No_Soft_Reset  RO
> >   0x00f0                RsvdP
> >   0x0100 PME_En         RW/RWS
> >   0x1e00 Data_Select    RW, VF ROZ
> >   0x6000 Data_Scale     RO, VF ROZ
> >   0x8000 PME_Status     RW1CS
> >
> > We intend to set PowerState to 0 (D0), apparently intend to clear
> > PME_En, and PME_Status is "write 1 to clear" to writing 0 does
> > nothing, so those look OK.
> >
> > But the RsvdP fields are reserved for future RW bits and should be
> > preserved, and it looks like clearing Data_Select could potentially
> > break the Data Register power consumption reporting (which I don't
> > think we support today).
> >
> > It seems like maybe we should do this instead:
> >
> >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
> >                         pmcsr & ~PCI_PM_CTRL_STATE_MASK)
> >
> > to just unconditionally clear PowerState?
>
> Or I guess this, since we want to clear PME_En as well?
>
>   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
>                         ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));

Yes.

Also, this patch actually only makes a difference if the device is
going into D0 from D1 or D2, because we have always written 0 to the
PMCSR during transitions from D3hot.

It is inconsistent and confusing to do different things depending on
the initial power state here and the code is simpler when 0 is written
regardless.

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Bjorn Helgaas]

On Fri, May 27, 2022 at 08:52:17PM +0200, Rafael J. Wysocki wrote:
> On Thu, May 26, 2022 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, May 26, 2022 at 11:54:22AM -0500, Bjorn Helgaas wrote:
> > > On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> > > > order to put it into D0 regardless of the power state returned by
> > > > the previous read from that register which should not matter.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/pci/pci.c |   11 +++--------
> > > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > > >
> > > > Index: linux-pm/drivers/pci/pci.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/pci/pci.c
> > > > +++ linux-pm/drivers/pci/pci.c
> > > > @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
> > > >     }
> > > >
> > > >     /*
> > > > -    * If we're (effectively) in D3, force entire word to 0. This doesn't
> > > > -    * affect PME_Status, disables PME_En, and sets PowerState to 0.
> > > > +    * Force the entire word to 0. This doesn't affect PME_Status, disables
> > > > +    * PME_En, and sets PowerState to 0.
> > > >      */
> > > > -   if (state == PCI_D3hot)
> > > > -           pmcsr = 0;
> > > > -   else
> > > > -           pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > > -
> > > > -   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > > > +   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > >
> > > Can you reassure me why this is safe and useful?
> > >
> > > This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
> > >
> > >   0x0003 PowerState     RW
> > >   0x0004                RsvdP
> > >   0x0008 No_Soft_Reset  RO
> > >   0x00f0                RsvdP
> > >   0x0100 PME_En         RW/RWS
> > >   0x1e00 Data_Select    RW, VF ROZ
> > >   0x6000 Data_Scale     RO, VF ROZ
> > >   0x8000 PME_Status     RW1CS
> > >
> > > We intend to set PowerState to 0 (D0), apparently intend to clear
> > > PME_En, and PME_Status is "write 1 to clear" to writing 0 does
> > > nothing, so those look OK.
> > >
> > > But the RsvdP fields are reserved for future RW bits and should be
> > > preserved, and it looks like clearing Data_Select could potentially
> > > break the Data Register power consumption reporting (which I don't
> > > think we support today).
> > >
> > > It seems like maybe we should do this instead:
> > >
> > >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
> > >                         pmcsr & ~PCI_PM_CTRL_STATE_MASK)
> > >
> > > to just unconditionally clear PowerState?
> >
> > Or I guess this, since we want to clear PME_En as well?
> >
> >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
> >                         ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));
> 
> Yes.
> 
> Also, this patch actually only makes a difference if the device is
> going into D0 from D1 or D2, because we have always written 0 to the
> PMCSR during transitions from D3hot.
> 
> It is inconsistent and confusing to do different things depending on
> the initial power state here and the code is simpler when 0 is written
> regardless.

I agree that depending on the initial power state is confusing (it
confused me :)).

What would you think of replacing this patch with the one below?


commit defde70748bc ("PCI/PM: Always put device in D0 and disable PME in pci_power_up()")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Fri May 27 17:45:07 2022 -0500

    PCI/PM: Always put device in D0 and disable PME in pci_power_up()
    
    Unconditionally put the device in PCI_D0 and disable PME generation in
    pci_power_up(), regardless of the power state returned by the previous read
    from PCI_PM_CTRL, which should not matter.
    
    Based-on-patch-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Link: https://lore.kernel.org/r/5748066.MhkbZ0Pkbq@kreacher
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a5b93f85377a..8e42a9dc1944 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1229,14 +1229,9 @@ int pci_power_up(struct pci_dev *dev)
 		goto end;
 	}
 
-	/*
-	 * If we're (effectively) in D3, force entire word to 0. This doesn't
-	 * affect PME_Status, disables PME_En, and sets PowerState to 0.
-	 */
-	if (state == PCI_D3hot)
-		pmcsr = 0;
-	else
-		pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
+	/* Set PowerState to 0 (PCI_D0) and disable PME generation */
+	pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
+	pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
 
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
 

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Bjorn Helgaas]

On Fri, May 27, 2022 at 05:51:48PM -0500, Bjorn Helgaas wrote:
> On Fri, May 27, 2022 at 08:52:17PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 26, 2022 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Thu, May 26, 2022 at 11:54:22AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> > > > > order to put it into D0 regardless of the power state returned by
> > > > > the previous read from that register which should not matter.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > ---
> > > > >  drivers/pci/pci.c |   11 +++--------
> > > > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > > > >
> > > > > Index: linux-pm/drivers/pci/pci.c
> > > > > ===================================================================
> > > > > --- linux-pm.orig/drivers/pci/pci.c
> > > > > +++ linux-pm/drivers/pci/pci.c
> > > > > @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
> > > > >     }
> > > > >
> > > > >     /*
> > > > > -    * If we're (effectively) in D3, force entire word to 0. This doesn't
> > > > > -    * affect PME_Status, disables PME_En, and sets PowerState to 0.
> > > > > +    * Force the entire word to 0. This doesn't affect PME_Status, disables
> > > > > +    * PME_En, and sets PowerState to 0.
> > > > >      */
> > > > > -   if (state == PCI_D3hot)
> > > > > -           pmcsr = 0;
> > > > > -   else
> > > > > -           pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > > > -
> > > > > -   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > > > > +   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > > >
> > > > Can you reassure me why this is safe and useful?
> > > >
> > > > This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
> > > >
> > > >   0x0003 PowerState     RW
> > > >   0x0004                RsvdP
> > > >   0x0008 No_Soft_Reset  RO
> > > >   0x00f0                RsvdP
> > > >   0x0100 PME_En         RW/RWS
> > > >   0x1e00 Data_Select    RW, VF ROZ
> > > >   0x6000 Data_Scale     RO, VF ROZ
> > > >   0x8000 PME_Status     RW1CS
> > > >
> > > > We intend to set PowerState to 0 (D0), apparently intend to clear
> > > > PME_En, and PME_Status is "write 1 to clear" to writing 0 does
> > > > nothing, so those look OK.
> > > >
> > > > But the RsvdP fields are reserved for future RW bits and should be
> > > > preserved, and it looks like clearing Data_Select could potentially
> > > > break the Data Register power consumption reporting (which I don't
> > > > think we support today).
> > > >
> > > > It seems like maybe we should do this instead:
> > > >
> > > >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
> > > >                         pmcsr & ~PCI_PM_CTRL_STATE_MASK)
> > > >
> > > > to just unconditionally clear PowerState?
> > >
> > > Or I guess this, since we want to clear PME_En as well?
> > >
> > >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
> > >                         ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));
> > 
> > Yes.
> > 
> > Also, this patch actually only makes a difference if the device is
> > going into D0 from D1 or D2, because we have always written 0 to the
> > PMCSR during transitions from D3hot.
> > 
> > It is inconsistent and confusing to do different things depending on
> > the initial power state here and the code is simpler when 0 is written
> > regardless.
> 
> I agree that depending on the initial power state is confusing (it
> confused me :)).
> 
> What would you think of replacing this patch with the one below?

Well, I don't know why I sent this, since I had already sent the pull
request.  Not thinking clearly, I guess.  Anyway, your original patch
is now upstream.  Sorry for the distraction.

Bjorn

Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases

[Rafael J. Wysocki]

On Sat, May 28, 2022 at 1:09 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, May 27, 2022 at 05:51:48PM -0500, Bjorn Helgaas wrote:
> > On Fri, May 27, 2022 at 08:52:17PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 26, 2022 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > On Thu, May 26, 2022 at 11:54:22AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, May 05, 2022 at 08:10:43PM +0200, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in
> > > > > > order to put it into D0 regardless of the power state returned by
> > > > > > the previous read from that register which should not matter.
> > > > > >
> > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > ---
> > > > > >  drivers/pci/pci.c |   11 +++--------
> > > > > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > Index: linux-pm/drivers/pci/pci.c
> > > > > > ===================================================================
> > > > > > --- linux-pm.orig/drivers/pci/pci.c
> > > > > > +++ linux-pm/drivers/pci/pci.c
> > > > > > @@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev)
> > > > > >     }
> > > > > >
> > > > > >     /*
> > > > > > -    * If we're (effectively) in D3, force entire word to 0. This doesn't
> > > > > > -    * affect PME_Status, disables PME_En, and sets PowerState to 0.
> > > > > > +    * Force the entire word to 0. This doesn't affect PME_Status, disables
> > > > > > +    * PME_En, and sets PowerState to 0.
> > > > > >      */
> > > > > > -   if (state == PCI_D3hot)
> > > > > > -           pmcsr = 0;
> > > > > > -   else
> > > > > > -           pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
> > > > > > -
> > > > > > -   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
> > > > > > +   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, 0);
> > > > >
> > > > > Can you reassure me why this is safe and useful?
> > > > >
> > > > > This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
> > > > >
> > > > >   0x0003 PowerState     RW
> > > > >   0x0004                RsvdP
> > > > >   0x0008 No_Soft_Reset  RO
> > > > >   0x00f0                RsvdP
> > > > >   0x0100 PME_En         RW/RWS
> > > > >   0x1e00 Data_Select    RW, VF ROZ
> > > > >   0x6000 Data_Scale     RO, VF ROZ
> > > > >   0x8000 PME_Status     RW1CS
> > > > >
> > > > > We intend to set PowerState to 0 (D0), apparently intend to clear
> > > > > PME_En, and PME_Status is "write 1 to clear" to writing 0 does
> > > > > nothing, so those look OK.
> > > > >
> > > > > But the RsvdP fields are reserved for future RW bits and should be
> > > > > preserved, and it looks like clearing Data_Select could potentially
> > > > > break the Data Register power consumption reporting (which I don't
> > > > > think we support today).
> > > > >
> > > > > It seems like maybe we should do this instead:
> > > > >
> > > > >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
> > > > >                         pmcsr & ~PCI_PM_CTRL_STATE_MASK)
> > > > >
> > > > > to just unconditionally clear PowerState?
> > > >
> > > > Or I guess this, since we want to clear PME_En as well?
> > > >
> > > >   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
> > > >                         ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));
> > >
> > > Yes.
> > >
> > > Also, this patch actually only makes a difference if the device is
> > > going into D0 from D1 or D2, because we have always written 0 to the
> > > PMCSR during transitions from D3hot.
> > >
> > > It is inconsistent and confusing to do different things depending on
> > > the initial power state here and the code is simpler when 0 is written
> > > regardless.
> >
> > I agree that depending on the initial power state is confusing (it
> > confused me :)).
> >
> > What would you think of replacing this patch with the one below?
>
> Well, I don't know why I sent this, since I had already sent the pull
> request.  Not thinking clearly, I guess.  Anyway, your original patch
> is now upstream.  Sorry for the distraction.

No biggie.

If it turns out to be problematic, it can be changed to preserving the
reserved bits easily enough.