linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors
@ 2019-08-05 20:52 Bjorn Helgaas
  2019-08-05 20:52 ` [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Reads from a PCI device may fail if the device has been turned off (put
into D3cold), removed, or if some other error occurs.  The PCI host bridge
typically fabricates ~0 data to complete the CPU's read.

We check for that in a few places, but not in a consistent way.  This
series adds a PCI_ERROR_RESPONSE definition to make the checks more
consistent and easier to find.  Note that ~0 may indicate a PCI error, but
it may also be valid read data, so you need more information (such as
knowing that a register can never contain ~0) before concluding that it's
an error.

This series also adds some new checks for PCI_ERROR_RESPONSE in the power
management code because that code frequently encounters devices in D3cold,
where we previously misinterpreted ~0 data.

Bjorn Helgaas (5):
  PCI: Add PCI_ERROR_RESPONSE definition
  PCI / PM: Return error when changing power state from D3cold
  PCI / PM: Check for error when reading PME status
  PCI / PM: Check for error when reading Power State
  PCI / PM: Decode D3cold power state correctly

 drivers/pci/access.c                          | 13 +++----
 .../pci/controller/dwc/pcie-designware-host.c |  2 +-
 drivers/pci/controller/pci-aardvark.c         |  2 +-
 drivers/pci/controller/pci-mvebu.c            |  4 +-
 drivers/pci/controller/pci-thunder-ecam.c     | 20 +++++-----
 drivers/pci/controller/pci-thunder-pem.c      |  2 +-
 drivers/pci/controller/pcie-altera.c          |  2 +-
 drivers/pci/controller/pcie-iproc.c           |  2 +-
 drivers/pci/controller/pcie-mediatek.c        |  4 +-
 drivers/pci/controller/pcie-rcar.c            |  2 +-
 drivers/pci/controller/pcie-rockchip-host.c   |  2 +-
 drivers/pci/controller/vmd.c                  |  2 +-
 drivers/pci/hotplug/cpqphp_ctrl.c             | 12 +++---
 drivers/pci/hotplug/cpqphp_pci.c              | 20 +++++-----
 drivers/pci/hotplug/pciehp_hpc.c              |  6 +--
 drivers/pci/pci.c                             | 38 ++++++++++++-------
 drivers/pci/pcie/dpc.c                        |  3 +-
 drivers/pci/pcie/pme.c                        |  4 +-
 drivers/pci/probe.c                           |  4 +-
 drivers/pci/quirks.c                          |  2 +-
 include/linux/pci.h                           | 20 ++++++++++
 21 files changed, 98 insertions(+), 68 deletions(-)

-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition
  2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
@ 2019-08-05 20:52 ` Bjorn Helgaas
  2019-08-05 21:16   ` Rafael J. Wysocki
  2019-08-05 20:52 ` [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold Bjorn Helgaas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

An MMIO read from a PCI device that doesn't exist or doesn't respond causes
a PCI error.  There's no real data to return to satisfy the CPU read, so
most hardware fabricates ~0 data.

Add a PCI_ERROR_RESPONSE definition for that and use it where appropriate
to make these checks consistent and easier to find.

Note that successful reads *also* may return ~0 data, so additional
information (e.g., knowledge that ~0 is not a valid register value) is
needed to reliably identify errors.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/access.c                          | 13 ++++++------
 .../pci/controller/dwc/pcie-designware-host.c |  2 +-
 drivers/pci/controller/pci-aardvark.c         |  2 +-
 drivers/pci/controller/pci-mvebu.c            |  4 ++--
 drivers/pci/controller/pci-thunder-ecam.c     | 20 +++++++++----------
 drivers/pci/controller/pci-thunder-pem.c      |  2 +-
 drivers/pci/controller/pcie-altera.c          |  2 +-
 drivers/pci/controller/pcie-iproc.c           |  2 +-
 drivers/pci/controller/pcie-mediatek.c        |  4 ++--
 drivers/pci/controller/pcie-rcar.c            |  2 +-
 drivers/pci/controller/pcie-rockchip-host.c   |  2 +-
 drivers/pci/controller/vmd.c                  |  2 +-
 drivers/pci/hotplug/cpqphp_ctrl.c             | 12 +++++------
 drivers/pci/hotplug/cpqphp_pci.c              | 20 +++++++++----------
 drivers/pci/hotplug/pciehp_hpc.c              |  6 +++---
 drivers/pci/pci.c                             |  8 ++++----
 drivers/pci/pcie/dpc.c                        |  3 ++-
 drivers/pci/pcie/pme.c                        |  4 ++--
 drivers/pci/probe.c                           |  4 ++--
 drivers/pci/quirks.c                          |  2 +-
 include/linux/pci.h                           |  7 +++++++
 21 files changed, 65 insertions(+), 58 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 544922f097c0..02186f3471d8 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, where);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -123,7 +123,7 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -536,7 +536,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
 int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
-		*val = ~0;
+		*val = (u8) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
@@ -546,18 +546,17 @@ EXPORT_SYMBOL(pci_read_config_byte);
 int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
-		*val = ~0;
+		*val = (u16) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_read_config_word);
 
-int pci_read_config_dword(const struct pci_dev *dev, int where,
-					u32 *val)
+int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f93252d0da5b..eb2e8b080a7d 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -594,7 +594,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 	struct pcie_port *pp = bus->sysdata;
 
 	if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index fc0fe4d4de49..77ad25ed7d38 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -520,7 +520,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	int ret;
 
 	if (!advk_pcie_valid_device(pcie, bus, devfn)) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index d3a0419e42f2..a45e45226790 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -648,7 +648,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 
 	port = mvebu_pcie_find_port(pcie, bus, devfn);
 	if (!port) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -658,7 +658,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 						 size, val);
 
 	if (!mvebu_pcie_link_up(port)) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pci-thunder-ecam.c b/drivers/pci/controller/pci-thunder-ecam.c
index 32d1d7b81ef4..b0332f349863 100644
--- a/drivers/pci/controller/pci-thunder-ecam.c
+++ b/drivers/pci/controller/pci-thunder-ecam.c
@@ -42,7 +42,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 	if (where_a == 0x4) {
 		addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
 		if (!addr) {
-			*val = ~0;
+			*val = (u32) PCI_ERROR_RESPONSE;
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		v = readl(addr);
@@ -57,7 +57,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 
 		addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
 		if (!addr) {
-			*val = ~0;
+			*val = (u32) PCI_ERROR_RESPONSE;
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		barl_orig = readl(addr + 0);
@@ -73,7 +73,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
 	if (where_a == 0xc) {
 		addr = bus->ops->map_bus(bus, devfn, bar + 4); /* BAR 1 */
 		if (!addr) {
-			*val = ~0;
+			*val = (u32) PCI_ERROR_RESPONSE;
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		v = readl(addr); /* EA entry-3. Base-H */
@@ -105,7 +105,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, where_a);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -136,7 +136,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 0xc);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -147,7 +147,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 8);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -177,7 +177,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	addr = bus->ops->map_bus(bus, devfn, 0);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
@@ -197,7 +197,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 
 		addr = bus->ops->map_bus(bus, devfn, 0x70);
 		if (!addr) {
-			*val = ~0;
+			*val = (u32) PCI_ERROR_RESPONSE;
 			return PCIBIOS_DEVICE_NOT_FOUND;
 		}
 		/* E_CAP */
@@ -212,7 +212,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 		if (where_a == 0xb0) {
 			addr = bus->ops->map_bus(bus, devfn, where_a);
 			if (!addr) {
-				*val = ~0;
+				*val = (u32) PCI_ERROR_RESPONSE;
 				return PCIBIOS_DEVICE_NOT_FOUND;
 			}
 			v = readl(addr);
@@ -269,7 +269,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
 		if (where_a == 0x70) {
 			addr = bus->ops->map_bus(bus, devfn, where_a);
 			if (!addr) {
-				*val = ~0;
+				*val = (u32) PCI_ERROR_RESPONSE;
 				return PCIBIOS_DEVICE_NOT_FOUND;
 			}
 			v = readl(addr);
diff --git a/drivers/pci/controller/pci-thunder-pem.c b/drivers/pci/controller/pci-thunder-pem.c
index f127ce8bd4ef..115e2c94d968 100644
--- a/drivers/pci/controller/pci-thunder-pem.c
+++ b/drivers/pci/controller/pci-thunder-pem.c
@@ -31,7 +31,7 @@ static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
 	struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
 
 	if (devfn != 0 || where >= 2048) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index d2497ca43828..3bd63b507eb1 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -512,7 +512,7 @@ static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
 	if (!altera_pcie_valid_device(pcie, bus, PCI_SLOT(devfn))) {
-		*value = 0xffffffff;
+		*value = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
index 2d457bfdaf66..1fafedb39c00 100644
--- a/drivers/pci/controller/pcie-iproc.c
+++ b/drivers/pci/controller/pcie-iproc.c
@@ -668,7 +668,7 @@ static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
 
 	addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
 	if (!addr) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 80601e1b939e..38f15a5e18ad 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -361,13 +361,13 @@ static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	port = mtk_pcie_find_port(bus, devfn);
 	if (!port) {
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
 	ret = mtk_pcie_hw_rd_cfg(port, bn, devfn, where, size, val);
 	if (ret)
-		*val = ~0;
+		*val = (u32) PCI_ERROR_RESPONSE;
 
 	return ret;
 }
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index f6a669a9af41..ae8d95146351 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -277,7 +277,7 @@ static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 	ret = rcar_pcie_config_access(pcie, RCAR_PCI_ACCESS_READ,
 				      bus, devfn, where, val);
 	if (ret != PCIBIOS_SUCCESSFUL) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return ret;
 	}
 
diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 8d20f1793a61..48d85f648d79 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -226,7 +226,7 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 	struct rockchip_pcie *rockchip = bus->sysdata;
 
 	if (!rockchip_pcie_valid_device(rockchip, bus, PCI_SLOT(devfn))) {
-		*val = 0xffffffff;
+		*val = (u32) PCI_ERROR_RESPONSE;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 4575e0c6dc4b..2af43b9e85db 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -578,7 +578,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 		membar2_offset = 0x2018;
 		ret = pci_read_config_dword(vmd->dev, PCI_REG_VMLOCK, &vmlock);
-		if (ret || vmlock == ~0)
+		if (ret || vmlock == (u32) PCI_ERROR_RESPONSE)
 			return -ENODEV;
 
 		if (MB2_SHADOW_EN(vmlock)) {
diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
index b7f4e1f099d9..e9d2bf4eeeef 100644
--- a/drivers/pci/hotplug/cpqphp_ctrl.c
+++ b/drivers/pci/hotplug/cpqphp_ctrl.c
@@ -1497,7 +1497,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
 	/* Check for a power fault */
 	if (func->status == 0xFF) {
 		/* power fault occurred, but it was benign */
-		temp_register = 0xFFFFFFFF;
+		temp_register = (u32) PCI_ERROR_RESPONSE;
 		dbg("%s: temp register set to %x by power fault\n", __func__, temp_register);
 		rc = POWER_FAILURE;
 		func->status = 0;
@@ -1510,7 +1510,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
 
 		if (rc != 0) {
 			/* Something's wrong here */
-			temp_register = 0xFFFFFFFF;
+			temp_register = (u32) PCI_ERROR_RESPONSE;
 			dbg("%s: temp register set to %x by error\n", __func__, temp_register);
 		}
 		/* Preset return code.  It will be changed later if things go okay. */
@@ -1518,7 +1518,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
 	}
 
 	/* All F's is an empty slot or an invalid board */
-	if (temp_register != 0xFFFFFFFF) {
+	if (temp_register != (u32) PCI_ERROR_RESPONSE) {
 		res_lists.io_head = ctrl->io_head;
 		res_lists.mem_head = ctrl->mem_head;
 		res_lists.p_mem_head = ctrl->p_mem_head;
@@ -2277,7 +2277,7 @@ static u32 configure_new_device(struct controller  *ctrl, struct pci_func  *func
 		while ((function < max_functions) && (!stop_it)) {
 			pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(func->device, function), 0x00, &ID);
 
-			if (ID == 0xFFFFFFFF) {
+			if (ID == (u32) PCI_ERROR_RESPONSE) {
 				function++;
 			} else {
 				/* Setup slot structure. */
@@ -2516,12 +2516,12 @@ static int configure_new_function(struct controller *ctrl, struct pci_func *func
 		for (device = 0; (device <= 0x1F) && !rc; device++) {
 			irqs.barber_pole = (irqs.barber_pole + 1) & 0x03;
 
-			ID = 0xFFFFFFFF;
+			ID = (u32) PCI_ERROR_RESPONSE;
 			pci_bus->number = hold_bus_node->base;
 			pci_bus_read_config_dword(pci_bus, PCI_DEVFN(device, 0), 0x00, &ID);
 			pci_bus->number = func->bus;
 
-			if (ID != 0xFFFFFFFF) {	  /*  device present */
+			if (ID != (u32) PCI_ERROR_RESPONSE) {	  /*  device present */
 				/* Setup slot structure. */
 				new_slot = cpqhp_slot_create(hold_bus_node->base);
 
diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
index 1b2b3f3b648b..d6ab73b9b280 100644
--- a/drivers/pci/hotplug/cpqphp_pci.c
+++ b/drivers/pci/hotplug/cpqphp_pci.c
@@ -138,7 +138,7 @@ static int PCI_RefinedAccessConfig(struct pci_bus *bus, unsigned int devfn, u8 o
 
 	if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &vendID) == -1)
 		return -1;
-	if (vendID == 0xffffffff)
+	if (vendID == (u32) PCI_ERROR_RESPONSE)
 		return -1;
 	return pci_bus_read_config_dword(bus, devfn, offset, value);
 }
@@ -251,7 +251,7 @@ static int PCI_GetBusDevHelper(struct controller *ctrl, u8 *bus_num, u8 *dev_num
 			*dev_num = tdevice;
 			ctrl->pci_bus->number = tbus;
 			pci_bus_read_config_dword(ctrl->pci_bus, *dev_num, PCI_VENDOR_ID, &work);
-			if (!nobridge || (work == 0xffffffff))
+			if (!nobridge || (work == (u32) PCI_ERROR_RESPONSE))
 				return 0;
 
 			dbg("bus_num %d devfn %d\n", *bus_num, *dev_num);
@@ -330,10 +330,10 @@ int cpqhp_save_config(struct controller *ctrl, int busnumber, int is_hot_plug)
 	/* Save PCI configuration space for all devices in supported slots */
 	ctrl->pci_bus->number = busnumber;
 	for (device = FirstSupported; device <= LastSupported; device++) {
-		ID = 0xFFFFFFFF;
+		ID = (u32) PCI_ERROR_RESPONSE;
 		rc = pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(device, 0), PCI_VENDOR_ID, &ID);
 
-		if (ID == 0xFFFFFFFF) {
+		if (ID == (u32) PCI_ERROR_RESPONSE) {
 			if (is_hot_plug) {
 				/* Setup slot structure with entry for empty
 				 * slot
@@ -431,7 +431,7 @@ int cpqhp_save_config(struct controller *ctrl, int busnumber, int is_hot_plug)
 			 */
 			while ((function < max_functions) && (!stop_it)) {
 				rc = pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(device, function), PCI_VENDOR_ID, &ID);
-				if (ID == 0xFFFFFFFF) {
+				if (ID == (u32) PCI_ERROR_RESPONSE) {
 					function++;
 					continue;
 				}
@@ -474,12 +474,12 @@ int cpqhp_save_slot_config(struct controller *ctrl, struct pci_func *new_slot)
 	int cloop = 0;
 	int stop_it;
 
-	ID = 0xFFFFFFFF;
+	ID = (u32) PCI_ERROR_RESPONSE;
 
 	ctrl->pci_bus->number = new_slot->bus;
 	pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(new_slot->device, 0), PCI_VENDOR_ID, &ID);
 
-	if (ID == 0xFFFFFFFF)
+	if (ID == (u32) PCI_ERROR_RESPONSE)
 		return 2;
 
 	pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(new_slot->device, 0), 0x0B, &class_code);
@@ -522,7 +522,7 @@ int cpqhp_save_slot_config(struct controller *ctrl, struct pci_func *new_slot)
 		while ((function < max_functions) && (!stop_it)) {
 			pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(new_slot->device, function), PCI_VENDOR_ID, &ID);
 
-			if (ID == 0xFFFFFFFF)
+			if (ID == (u32) PCI_ERROR_RESPONSE)
 				function++;
 			else {
 				pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(new_slot->device, function), 0x0B, &class_code);
@@ -1049,7 +1049,7 @@ int cpqhp_valid_replace(struct controller *ctrl, struct pci_func *func)
 		pci_bus_read_config_dword(pci_bus, devfn, PCI_VENDOR_ID, &temp_register);
 
 		/* No adapter present */
-		if (temp_register == 0xFFFFFFFF)
+		if (temp_register == (u32) PCI_ERROR_RESPONSE)
 			return(NO_ADAPTER_PRESENT);
 
 		if (temp_register != func->config_space[0])
@@ -1267,7 +1267,7 @@ int cpqhp_find_available_resources(struct controller *ctrl, void __iomem *rom_st
 		pci_bus_read_config_dword(ctrl->pci_bus, dev_func, PCI_VENDOR_ID, &temp_dword);
 		dbg("temp_D_word = %x\n", temp_dword);
 
-		if (temp_dword != 0xFFFFFFFF) {
+		if (temp_dword != (u32) PCI_ERROR_RESPONSE) {
 			index = 0;
 			func = cpqhp_slot_find(primary_bus, dev_func >> 3, 0);
 
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bd990e3371e3..f0489f305ad7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -70,7 +70,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 
 	while (true) {
 		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
-		if (slot_status == (u16) ~0) {
+		if (slot_status == (u16) PCI_ERROR_RESPONSE) {
 			ctrl_info(ctrl, "%s: no response from device\n",
 				  __func__);
 			return 0;
@@ -148,7 +148,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
 	pcie_wait_cmd(ctrl);
 
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
-	if (slot_ctrl == (u16) ~0) {
+	if (slot_ctrl == (u16) PCI_ERROR_RESPONSE) {
 		ctrl_info(ctrl, "%s: no response from device\n", __func__);
 		goto out;
 	}
@@ -543,7 +543,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	}
 
 	pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
-	if (status == (u16) ~0) {
+	if (status == (u16) PCI_ERROR_RESPONSE) {
 		ctrl_info(ctrl, "%s: no response from device\n", __func__);
 		if (parent)
 			pm_runtime_put(parent);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ed5ec1ac27..bfc739dc6ada 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4434,16 +4434,16 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
 	 * After reset, the device should not silently discard config
 	 * requests, but it may still indicate that it needs more time by
 	 * responding to them with CRS completions.  The Root Port will
-	 * generally synthesize ~0 data to complete the read (except when
-	 * CRS SV is enabled and the read was for the Vendor ID; in that
-	 * case it synthesizes 0x0001 data).
+	 * generally synthesize ~0 data (PCI_ERROR_RESPONSE) to complete
+	 * the read (except when CRS SV is enabled and the read was for the
+	 * Vendor ID; in that case it synthesizes 0x0001 data).
 	 *
 	 * Wait for the device to return a non-CRS completion.  Read the
 	 * Command register instead of Vendor ID so we don't have to
 	 * contend with the CRS SV value.
 	 */
 	pci_read_config_dword(dev, PCI_COMMAND, &id);
-	while (id == ~0) {
+	while (id == (u32) PCI_ERROR_RESPONSE) {
 		if (delay > timeout) {
 			pci_warn(dev, "not ready %dms after %s; giving up\n",
 				 delay - 1, reset_type);
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index a32ec3487a8d..96b6b87a0af3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -272,7 +272,8 @@ static irqreturn_t dpc_irq(int irq, void *context)
 
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 
-	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
+	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) ||
+	    status == (u16)(PCI_ERROR_RESPONSE))
 		return IRQ_NONE;
 
 	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index f38e6c19dd50..3c46622e6c3f 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -224,7 +224,7 @@ static void pcie_pme_work_fn(struct work_struct *work)
 			break;
 
 		pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
-		if (rtsta == (u32) ~0)
+		if (rtsta == (u32) PCI_ERROR_RESPONSE)
 			break;
 
 		if (rtsta & PCI_EXP_RTSTA_PME) {
@@ -274,7 +274,7 @@ static irqreturn_t pcie_pme_irq(int irq, void *context)
 	spin_lock_irqsave(&data->lock, flags);
 	pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
 
-	if (rtsta == (u32) ~0 || !(rtsta & PCI_EXP_RTSTA_PME)) {
+	if (rtsta == (u32) PCI_ERROR_RESPONSE || !(rtsta & PCI_EXP_RTSTA_PME)) {
 		spin_unlock_irqrestore(&data->lock, flags);
 		return IRQ_NONE;
 	}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3c7338fad86..fb953f2666b9 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1549,7 +1549,7 @@ static int pci_cfg_space_size_ext(struct pci_dev *dev)
 
 	if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
 		return PCI_CFG_SPACE_SIZE;
-	if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
+	if (status == (u32) PCI_ERROR_RESPONSE || pci_ext_cfg_is_aliased(dev))
 		return PCI_CFG_SPACE_SIZE;
 
 	return PCI_CFG_SPACE_EXP_SIZE;
@@ -2488,7 +2488,7 @@ bool pci_bus_generic_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
 		return false;
 
 	/* Some broken boards return 0 or ~0 if a slot is empty: */
-	if (*l == 0xffffffff || *l == 0x00000000 ||
+	if (*l == (u32) PCI_ERROR_RESPONSE || *l == 0x00000000 ||
 	    *l == 0x0000ffff || *l == 0xffff0000)
 		return false;
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 208aacf39329..3d5c92b53310 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4854,7 +4854,7 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
 
 		pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
 		if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
-		    PCIBIOS_SUCCESSFUL || (status == 0xffffffff))
+		    PCIBIOS_SUCCESSFUL || (status == (u32) PCI_ERROR_RESPONSE))
 			pdev->cfg_size = PCI_CFG_SPACE_SIZE;
 
 		if (pci_find_saved_cap(pdev, PCI_CAP_ID_EXP))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9e700d9f9f28..d64fd3788061 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -123,6 +123,13 @@ enum pci_interrupt_pin {
 /* The number of legacy PCI INTx interrupts */
 #define PCI_NUM_INTX	4
 
+/*
+ * Reading from a device that doesn't respond typically returns ~0.  A
+ * successful read from a device may also return ~0, so you need additional
+ * information to reliably identify errors.
+ */
+#define PCI_ERROR_RESPONSE		(~0ULL)
+
 /*
  * pci_power_t values must match the bits in the Capabilities PME_Support
  * and Control/Status PowerState fields in the Power Management capability.
-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold
  2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
  2019-08-05 20:52 ` [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
@ 2019-08-05 20:52 ` Bjorn Helgaas
  2019-08-05 21:15   ` Rafael J. Wysocki
  2019-08-05 20:52 ` [PATCH 3/5] PCI / PM: Check for error when reading PME status Bjorn Helgaas
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

pci_raw_set_power_state() uses the Power Management capability to change a
device's power state.  The capability is in config space, which is
accessible in D0, D1, D2, and D3hot, but not in D3cold.

If we call pci_raw_set_power_state() on a device that's in D3cold, config
reads fail and return ~0 data, which we erroneously interpreted as "the
device is in D3hot", leading to messages like this:

  pcieport 0000:03:00.0: Refused to change power state, currently in D3

The PCI_PM_CTRL has several RsvdP fields, so ~0 is never a valid register
value.  Notice if we get that data, print a more informative message, and
return an error.

Changing the power state of a device from D3cold must be done by a platform
power management method or some other non-config space mechanism.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bfc739dc6ada..984171d40858 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -851,6 +851,11 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 		return -EIO;
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+	if (pmcsr == (u16) PCI_ERROR_RESPONSE) {
+		pci_err(dev, "device not responding; can't change to power state D%d\n",
+			state);
+		return -EIO;
+	}
 
 	/*
 	 * If we're (effectively) in D3, force entire word to 0.
-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/5] PCI / PM: Check for error when reading PME status
  2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
  2019-08-05 20:52 ` [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
  2019-08-05 20:52 ` [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold Bjorn Helgaas
@ 2019-08-05 20:52 ` Bjorn Helgaas
  2019-08-05 21:02   ` Rafael J. Wysocki
  2019-08-05 20:52 ` [PATCH 4/5] PCI / PM: Check for error when reading Power State Bjorn Helgaas
  2019-08-05 20:52 ` [PATCH 5/5] PCI / PM: Decode D3cold power state correctly Bjorn Helgaas
  4 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

pci_check_pme_status() reads the Power Management capability to determine
whether a device has generated a PME.  The capability is in config space,
which is accessible in D0, D1, D2, and D3hot, but not in D3cold.

If we call pci_check_pme_status() on a device that's in D3cold, config
reads fail and return ~0 data, which we erroneously interpreted as "the
device has generated a PME".

000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
avoided many of these problems by avoiding pci_check_pme_status() if we
think the device is in D3cold.  However, it is not a complete fix because
the device may go to D3cold after we check its power state but before
pci_check_pme_status() reads the Power Management Status Register.

Return false ("device has not generated a PME") if we get an error response
reading the Power Management Status Register.

Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 984171d40858..af6a97d7012b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
 
 	pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
 	pci_read_config_word(dev, pmcsr_pos, &pmcsr);
+	if (pmcsr == (u16) PCI_ERROR_RESPONSE)
+		return false;
+
 	if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
 		return false;
 
-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/5] PCI / PM: Check for error when reading Power State
  2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2019-08-05 20:52 ` [PATCH 3/5] PCI / PM: Check for error when reading PME status Bjorn Helgaas
@ 2019-08-05 20:52 ` Bjorn Helgaas
  2019-08-05 21:09   ` Rafael J. Wysocki
  2019-08-05 20:52 ` [PATCH 5/5] PCI / PM: Decode D3cold power state correctly Bjorn Helgaas
  4 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

The Power Management Status Register is in config space, and reads while
the device is in D3cold typically return ~0 data (PCI_ERROR_RESPONSE).  If
we just look at the PCI_PM_CTRL_STATE_MASK bits, that is 0x3, which looks
like D3hot, not D3cold.

Check the entire register for PCI_ERROR_RESPONSE so we can distinguish
D3cold from D3hot.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   |  6 +++---
 include/linux/pci.h | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index af6a97d7012b..d8686e3cd5eb 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -894,7 +894,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 		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 = pci_power_state(pmcsr);
 	if (dev->current_state != state && printk_ratelimit())
 		pci_info(dev, "Refused to change power state, currently in D%d\n",
 			 dev->current_state);
@@ -942,7 +942,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 		u16 pmcsr;
 
 		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
-		dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
+		dev->current_state = pci_power_state(pmcsr);
 	} else {
 		dev->current_state = state;
 	}
@@ -1677,7 +1677,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 	if (dev->pm_cap) {
 		u16 pmcsr;
 		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
-		dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
+		dev->current_state = pci_power_state(pmcsr);
 	}
 
 	if (atomic_inc_return(&dev->enable_cnt) > 1)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d64fd3788061..fdfe990e9661 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -152,6 +152,19 @@ static inline const char *pci_power_name(pci_power_t state)
 	return pci_power_names[1 + (__force int) state];
 }
 
+/*
+ * Convert a Power Management Status Register value to a pci_power_t.
+ * Note that if we read the register while the device is in D3cold, we
+ * typically get PCI_ERROR_RESPONSE, which looks like D3hot (0x3) if we
+ * only look at the PCI_PM_CTRL_STATE_MASK bits.
+ */
+static inline pci_power_t pci_power_state(u16 pmcsr)
+{
+	if (pmcsr == (u16) PCI_ERROR_RESPONSE)
+		return PCI_D3cold;
+	return pmcsr & PCI_PM_CTRL_STATE_MASK;
+}
+
 #define PCI_PM_D2_DELAY		200
 #define PCI_PM_D3_WAIT		10
 #define PCI_PM_D3COLD_WAIT	100
-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/5] PCI / PM: Decode D3cold power state correctly
  2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
                   ` (3 preceding siblings ...)
  2019-08-05 20:52 ` [PATCH 4/5] PCI / PM: Check for error when reading Power State Bjorn Helgaas
@ 2019-08-05 20:52 ` Bjorn Helgaas
  2019-08-05 21:14   ` Rafael J. Wysocki
  4 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-05 20:52 UTC (permalink / raw)
  To: linux-pci
  Cc: Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, linux-pm, linux-kernel,
	Bjorn Helgaas

From: Bjorn Helgaas <bhelgaas@google.com>

Use pci_power_name() to print pci_power_t correctly.  This changes:

  "state 0" or "D0"   to   "D0"
  "state 1" or "D1"   to   "D1"
  "state 2" or "D2"   to   "D2"
  "state 3" or "D3"   to   "D3hot"
  "state 4" or "D4"   to   "D3cold"

Changes dmesg logging only, no other functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d8686e3cd5eb..17ae2615ac11 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -834,14 +834,16 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 		return -EINVAL;
 
 	/*
-	 * Validate current state:
-	 * Can enter D0 from any state, but if we can only go deeper
-	 * to sleep if we're already in a low power state
+	 * 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 (state != PCI_D0 && dev->current_state <= PCI_D3cold
 	    && dev->current_state > state) {
-		pci_err(dev, "invalid power transition (from state %d to %d)\n",
-			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;
 	}
 
@@ -896,8 +898,8 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	dev->current_state = pci_power_state(pmcsr);
 	if (dev->current_state != state && printk_ratelimit())
-		pci_info(dev, "Refused to change power state, currently in D%d\n",
-			 dev->current_state);
+		pci_info(dev, "refused to change power state (currently %s)\n",
+			 pci_power_name(dev->current_state));
 
 	/*
 	 * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
-- 
2.22.0.770.g0f2c4a37fd-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] PCI / PM: Check for error when reading PME status
  2019-08-05 20:52 ` [PATCH 3/5] PCI / PM: Check for error when reading PME status Bjorn Helgaas
@ 2019-08-05 21:02   ` Rafael J. Wysocki
  2019-08-06 13:36     ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-05 21:02 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List, Bjorn Helgaas

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> pci_check_pme_status() reads the Power Management capability to determine
> whether a device has generated a PME.  The capability is in config space,
> which is accessible in D0, D1, D2, and D3hot, but not in D3cold.
>
> If we call pci_check_pme_status() on a device that's in D3cold, config
> reads fail and return ~0 data, which we erroneously interpreted as "the
> device has generated a PME".
>
> 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> avoided many of these problems by avoiding pci_check_pme_status() if we
> think the device is in D3cold.  However, it is not a complete fix because
> the device may go to D3cold after we check its power state but before
> pci_check_pme_status() reads the Power Management Status Register.
>
> Return false ("device has not generated a PME") if we get an error response
> reading the Power Management Status Register.
>
> Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 984171d40858..af6a97d7012b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
>
>         pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
>         pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> +               return false;
> +

No, sorry.

We tried that and it didn't work.

pcie_pme_handle_request() depends on this returning "true" for all
bits set, as from its perspective "device is not accessible" may very
well mean "device may have signaled PME".

If you want to make this change, you need to rework
pcie_pme_handle_request() along with it.

>         if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
>                 return false;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] PCI / PM: Check for error when reading Power State
  2019-08-05 20:52 ` [PATCH 4/5] PCI / PM: Check for error when reading Power State Bjorn Helgaas
@ 2019-08-05 21:09   ` Rafael J. Wysocki
  2019-08-09 22:01     ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-05 21:09 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List, Bjorn Helgaas

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> The Power Management Status Register is in config space, and reads while
> the device is in D3cold typically return ~0 data (PCI_ERROR_RESPONSE).  If
> we just look at the PCI_PM_CTRL_STATE_MASK bits, that is 0x3, which looks
> like D3hot, not D3cold.
>
> Check the entire register for PCI_ERROR_RESPONSE so we can distinguish
> D3cold from D3hot.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.c   |  6 +++---
>  include/linux/pci.h | 13 +++++++++++++
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index af6a97d7012b..d8686e3cd5eb 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -894,7 +894,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>                 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 = pci_power_state(pmcsr);

But pci_raw_set_power_state() should not even be called for devices in
D3_cold, so this at best is redundant.

>         if (dev->current_state != state && printk_ratelimit())
>                 pci_info(dev, "Refused to change power state, currently in D%d\n",
>                          dev->current_state);
> @@ -942,7 +942,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
>                 u16 pmcsr;
>
>                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> +               dev->current_state = pci_power_state(pmcsr);

The if () branch above should cover the D3cold case, shouldn't it?

>         } else {
>                 dev->current_state = state;
>         }
> @@ -1677,7 +1677,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>         if (dev->pm_cap) {
>                 u16 pmcsr;
>                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> +               dev->current_state = pci_power_state(pmcsr);

So this appears to be only case in which pci_power_state(pmcsr) is
useful at all.

It might be better to use the code from it directly here IMO.

>         }
>
>         if (atomic_inc_return(&dev->enable_cnt) > 1)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d64fd3788061..fdfe990e9661 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -152,6 +152,19 @@ static inline const char *pci_power_name(pci_power_t state)
>         return pci_power_names[1 + (__force int) state];
>  }
>
> +/*
> + * Convert a Power Management Status Register value to a pci_power_t.
> + * Note that if we read the register while the device is in D3cold, we
> + * typically get PCI_ERROR_RESPONSE, which looks like D3hot (0x3) if we
> + * only look at the PCI_PM_CTRL_STATE_MASK bits.
> + */
> +static inline pci_power_t pci_power_state(u16 pmcsr)
> +{
> +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> +               return PCI_D3cold;
> +       return pmcsr & PCI_PM_CTRL_STATE_MASK;
> +}
> +
>  #define PCI_PM_D2_DELAY                200
>  #define PCI_PM_D3_WAIT         10
>  #define PCI_PM_D3COLD_WAIT     100
> --
> 2.22.0.770.g0f2c4a37fd-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] PCI / PM: Decode D3cold power state correctly
  2019-08-05 20:52 ` [PATCH 5/5] PCI / PM: Decode D3cold power state correctly Bjorn Helgaas
@ 2019-08-05 21:14   ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-05 21:14 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List, Bjorn Helgaas

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Use pci_power_name() to print pci_power_t correctly.  This changes:
>
>   "state 0" or "D0"   to   "D0"
>   "state 1" or "D1"   to   "D1"
>   "state 2" or "D2"   to   "D2"
>   "state 3" or "D3"   to   "D3hot"
>   "state 4" or "D4"   to   "D3cold"
>
> Changes dmesg logging only, no other functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d8686e3cd5eb..17ae2615ac11 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -834,14 +834,16 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>                 return -EINVAL;
>
>         /*
> -        * Validate current state:
> -        * Can enter D0 from any state, but if we can only go deeper
> -        * to sleep if we're already in a low power state
> +        * 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 (state != PCI_D0 && dev->current_state <= PCI_D3cold
>             && dev->current_state > state) {
> -               pci_err(dev, "invalid power transition (from state %d to %d)\n",
> -                       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;
>         }
>
> @@ -896,8 +898,8 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>         dev->current_state = pci_power_state(pmcsr);
>         if (dev->current_state != state && printk_ratelimit())
> -               pci_info(dev, "Refused to change power state, currently in D%d\n",
> -                        dev->current_state);
> +               pci_info(dev, "refused to change power state (currently %s)\n",
> +                        pci_power_name(dev->current_state));
>
>         /*
>          * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT
> --
> 2.22.0.770.g0f2c4a37fd-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold
  2019-08-05 20:52 ` [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold Bjorn Helgaas
@ 2019-08-05 21:15   ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-05 21:15 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List, Bjorn Helgaas

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> pci_raw_set_power_state() uses the Power Management capability to change a
> device's power state.  The capability is in config space, which is
> accessible in D0, D1, D2, and D3hot, but not in D3cold.
>
> If we call pci_raw_set_power_state() on a device that's in D3cold, config
> reads fail and return ~0 data, which we erroneously interpreted as "the
> device is in D3hot", leading to messages like this:
>
>   pcieport 0000:03:00.0: Refused to change power state, currently in D3
>
> The PCI_PM_CTRL has several RsvdP fields, so ~0 is never a valid register
> value.  Notice if we get that data, print a more informative message, and
> return an error.
>
> Changing the power state of a device from D3cold must be done by a platform
> power management method or some other non-config space mechanism.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bfc739dc6ada..984171d40858 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -851,6 +851,11 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>                 return -EIO;
>
>         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> +       if (pmcsr == (u16) PCI_ERROR_RESPONSE) {
> +               pci_err(dev, "device not responding; can't change to power state D%d\n",
> +                       state);
> +               return -EIO;
> +       }
>
>         /*
>          * If we're (effectively) in D3, force entire word to 0.
> --
> 2.22.0.770.g0f2c4a37fd-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition
  2019-08-05 20:52 ` [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
@ 2019-08-05 21:16   ` Rafael J. Wysocki
  0 siblings, 0 replies; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-05 21:16 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List, Bjorn Helgaas

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> An MMIO read from a PCI device that doesn't exist or doesn't respond causes
> a PCI error.  There's no real data to return to satisfy the CPU read, so
> most hardware fabricates ~0 data.
>
> Add a PCI_ERROR_RESPONSE definition for that and use it where appropriate
> to make these checks consistent and easier to find.
>
> Note that successful reads *also* may return ~0 data, so additional
> information (e.g., knowledge that ~0 is not a valid register value) is
> needed to reliably identify errors.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/access.c                          | 13 ++++++------
>  .../pci/controller/dwc/pcie-designware-host.c |  2 +-
>  drivers/pci/controller/pci-aardvark.c         |  2 +-
>  drivers/pci/controller/pci-mvebu.c            |  4 ++--
>  drivers/pci/controller/pci-thunder-ecam.c     | 20 +++++++++----------
>  drivers/pci/controller/pci-thunder-pem.c      |  2 +-
>  drivers/pci/controller/pcie-altera.c          |  2 +-
>  drivers/pci/controller/pcie-iproc.c           |  2 +-
>  drivers/pci/controller/pcie-mediatek.c        |  4 ++--
>  drivers/pci/controller/pcie-rcar.c            |  2 +-
>  drivers/pci/controller/pcie-rockchip-host.c   |  2 +-
>  drivers/pci/controller/vmd.c                  |  2 +-
>  drivers/pci/hotplug/cpqphp_ctrl.c             | 12 +++++------
>  drivers/pci/hotplug/cpqphp_pci.c              | 20 +++++++++----------
>  drivers/pci/hotplug/pciehp_hpc.c              |  6 +++---
>  drivers/pci/pci.c                             |  8 ++++----
>  drivers/pci/pcie/dpc.c                        |  3 ++-
>  drivers/pci/pcie/pme.c                        |  4 ++--
>  drivers/pci/probe.c                           |  4 ++--
>  drivers/pci/quirks.c                          |  2 +-
>  include/linux/pci.h                           |  7 +++++++
>  21 files changed, 65 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 544922f097c0..02186f3471d8 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, where);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -123,7 +123,7 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -536,7 +536,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
>         if (pci_dev_is_disconnected(dev)) {
> -               *val = ~0;
> +               *val = (u8) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>         return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
> @@ -546,18 +546,17 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
>         if (pci_dev_is_disconnected(dev)) {
> -               *val = ~0;
> +               *val = (u16) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>         return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_read_config_word);
>
> -int pci_read_config_dword(const struct pci_dev *dev, int where,
> -                                       u32 *val)
> +int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val)
>  {
>         if (pci_dev_is_disconnected(dev)) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>         return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f93252d0da5b..eb2e8b080a7d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -594,7 +594,7 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>         struct pcie_port *pp = bus->sysdata;
>
>         if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index fc0fe4d4de49..77ad25ed7d38 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -520,7 +520,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>         int ret;
>
>         if (!advk_pcie_valid_device(pcie, bus, devfn)) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index d3a0419e42f2..a45e45226790 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -648,7 +648,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>
>         port = mvebu_pcie_find_port(pcie, bus, devfn);
>         if (!port) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -658,7 +658,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>                                                  size, val);
>
>         if (!mvebu_pcie_link_up(port)) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pci-thunder-ecam.c b/drivers/pci/controller/pci-thunder-ecam.c
> index 32d1d7b81ef4..b0332f349863 100644
> --- a/drivers/pci/controller/pci-thunder-ecam.c
> +++ b/drivers/pci/controller/pci-thunder-ecam.c
> @@ -42,7 +42,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
>         if (where_a == 0x4) {
>                 addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
>                 if (!addr) {
> -                       *val = ~0;
> +                       *val = (u32) PCI_ERROR_RESPONSE;
>                         return PCIBIOS_DEVICE_NOT_FOUND;
>                 }
>                 v = readl(addr);
> @@ -57,7 +57,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
>
>                 addr = bus->ops->map_bus(bus, devfn, bar); /* BAR 0 */
>                 if (!addr) {
> -                       *val = ~0;
> +                       *val = (u32) PCI_ERROR_RESPONSE;
>                         return PCIBIOS_DEVICE_NOT_FOUND;
>                 }
>                 barl_orig = readl(addr + 0);
> @@ -73,7 +73,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,
>         if (where_a == 0xc) {
>                 addr = bus->ops->map_bus(bus, devfn, bar + 4); /* BAR 1 */
>                 if (!addr) {
> -                       *val = ~0;
> +                       *val = (u32) PCI_ERROR_RESPONSE;
>                         return PCIBIOS_DEVICE_NOT_FOUND;
>                 }
>                 v = readl(addr); /* EA entry-3. Base-H */
> @@ -105,7 +105,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, where_a);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -136,7 +136,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, 0xc);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -147,7 +147,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, 8);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -177,7 +177,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         addr = bus->ops->map_bus(bus, devfn, 0);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> @@ -197,7 +197,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>
>                 addr = bus->ops->map_bus(bus, devfn, 0x70);
>                 if (!addr) {
> -                       *val = ~0;
> +                       *val = (u32) PCI_ERROR_RESPONSE;
>                         return PCIBIOS_DEVICE_NOT_FOUND;
>                 }
>                 /* E_CAP */
> @@ -212,7 +212,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>                 if (where_a == 0xb0) {
>                         addr = bus->ops->map_bus(bus, devfn, where_a);
>                         if (!addr) {
> -                               *val = ~0;
> +                               *val = (u32) PCI_ERROR_RESPONSE;
>                                 return PCIBIOS_DEVICE_NOT_FOUND;
>                         }
>                         v = readl(addr);
> @@ -269,7 +269,7 @@ static int thunder_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
>                 if (where_a == 0x70) {
>                         addr = bus->ops->map_bus(bus, devfn, where_a);
>                         if (!addr) {
> -                               *val = ~0;
> +                               *val = (u32) PCI_ERROR_RESPONSE;
>                                 return PCIBIOS_DEVICE_NOT_FOUND;
>                         }
>                         v = readl(addr);
> diff --git a/drivers/pci/controller/pci-thunder-pem.c b/drivers/pci/controller/pci-thunder-pem.c
> index f127ce8bd4ef..115e2c94d968 100644
> --- a/drivers/pci/controller/pci-thunder-pem.c
> +++ b/drivers/pci/controller/pci-thunder-pem.c
> @@ -31,7 +31,7 @@ static int thunder_pem_bridge_read(struct pci_bus *bus, unsigned int devfn,
>         struct thunder_pem_pci *pem_pci = (struct thunder_pem_pci *)cfg->priv;
>
>         if (devfn != 0 || where >= 2048) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index d2497ca43828..3bd63b507eb1 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -512,7 +512,7 @@ static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
>                 return PCIBIOS_BAD_REGISTER_NUMBER;
>
>         if (!altera_pcie_valid_device(pcie, bus, PCI_SLOT(devfn))) {
> -               *value = 0xffffffff;
> +               *value = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> index 2d457bfdaf66..1fafedb39c00 100644
> --- a/drivers/pci/controller/pcie-iproc.c
> +++ b/drivers/pci/controller/pcie-iproc.c
> @@ -668,7 +668,7 @@ static int iproc_pci_raw_config_read32(struct iproc_pcie *pcie,
>
>         addr = iproc_pcie_map_cfg_bus(pcie, 0, devfn, where & ~0x3);
>         if (!addr) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 80601e1b939e..38f15a5e18ad 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -361,13 +361,13 @@ static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>
>         port = mtk_pcie_find_port(bus, devfn);
>         if (!port) {
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
>         ret = mtk_pcie_hw_rd_cfg(port, bn, devfn, where, size, val);
>         if (ret)
> -               *val = ~0;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>
>         return ret;
>  }
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index f6a669a9af41..ae8d95146351 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -277,7 +277,7 @@ static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
>         ret = rcar_pcie_config_access(pcie, RCAR_PCI_ACCESS_READ,
>                                       bus, devfn, where, val);
>         if (ret != PCIBIOS_SUCCESSFUL) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return ret;
>         }
>
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 8d20f1793a61..48d85f648d79 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -226,7 +226,7 @@ static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>         struct rockchip_pcie *rockchip = bus->sysdata;
>
>         if (!rockchip_pcie_valid_device(rockchip, bus, PCI_SLOT(devfn))) {
> -               *val = 0xffffffff;
> +               *val = (u32) PCI_ERROR_RESPONSE;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 4575e0c6dc4b..2af43b9e85db 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -578,7 +578,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>
>                 membar2_offset = 0x2018;
>                 ret = pci_read_config_dword(vmd->dev, PCI_REG_VMLOCK, &vmlock);
> -               if (ret || vmlock == ~0)
> +               if (ret || vmlock == (u32) PCI_ERROR_RESPONSE)
>                         return -ENODEV;
>
>                 if (MB2_SHADOW_EN(vmlock)) {
> diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c b/drivers/pci/hotplug/cpqphp_ctrl.c
> index b7f4e1f099d9..e9d2bf4eeeef 100644
> --- a/drivers/pci/hotplug/cpqphp_ctrl.c
> +++ b/drivers/pci/hotplug/cpqphp_ctrl.c
> @@ -1497,7 +1497,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
>         /* Check for a power fault */
>         if (func->status == 0xFF) {
>                 /* power fault occurred, but it was benign */
> -               temp_register = 0xFFFFFFFF;
> +               temp_register = (u32) PCI_ERROR_RESPONSE;
>                 dbg("%s: temp register set to %x by power fault\n", __func__, temp_register);
>                 rc = POWER_FAILURE;
>                 func->status = 0;
> @@ -1510,7 +1510,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
>
>                 if (rc != 0) {
>                         /* Something's wrong here */
> -                       temp_register = 0xFFFFFFFF;
> +                       temp_register = (u32) PCI_ERROR_RESPONSE;
>                         dbg("%s: temp register set to %x by error\n", __func__, temp_register);
>                 }
>                 /* Preset return code.  It will be changed later if things go okay. */
> @@ -1518,7 +1518,7 @@ static u32 board_added(struct pci_func *func, struct controller *ctrl)
>         }
>
>         /* All F's is an empty slot or an invalid board */
> -       if (temp_register != 0xFFFFFFFF) {
> +       if (temp_register != (u32) PCI_ERROR_RESPONSE) {
>                 res_lists.io_head = ctrl->io_head;
>                 res_lists.mem_head = ctrl->mem_head;
>                 res_lists.p_mem_head = ctrl->p_mem_head;
> @@ -2277,7 +2277,7 @@ static u32 configure_new_device(struct controller  *ctrl, struct pci_func  *func
>                 while ((function < max_functions) && (!stop_it)) {
>                         pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(func->device, function), 0x00, &ID);
>
> -                       if (ID == 0xFFFFFFFF) {
> +                       if (ID == (u32) PCI_ERROR_RESPONSE) {
>                                 function++;
>                         } else {
>                                 /* Setup slot structure. */
> @@ -2516,12 +2516,12 @@ static int configure_new_function(struct controller *ctrl, struct pci_func *func
>                 for (device = 0; (device <= 0x1F) && !rc; device++) {
>                         irqs.barber_pole = (irqs.barber_pole + 1) & 0x03;
>
> -                       ID = 0xFFFFFFFF;
> +                       ID = (u32) PCI_ERROR_RESPONSE;
>                         pci_bus->number = hold_bus_node->base;
>                         pci_bus_read_config_dword(pci_bus, PCI_DEVFN(device, 0), 0x00, &ID);
>                         pci_bus->number = func->bus;
>
> -                       if (ID != 0xFFFFFFFF) {   /*  device present */
> +                       if (ID != (u32) PCI_ERROR_RESPONSE) {     /*  device present */
>                                 /* Setup slot structure. */
>                                 new_slot = cpqhp_slot_create(hold_bus_node->base);
>
> diff --git a/drivers/pci/hotplug/cpqphp_pci.c b/drivers/pci/hotplug/cpqphp_pci.c
> index 1b2b3f3b648b..d6ab73b9b280 100644
> --- a/drivers/pci/hotplug/cpqphp_pci.c
> +++ b/drivers/pci/hotplug/cpqphp_pci.c
> @@ -138,7 +138,7 @@ static int PCI_RefinedAccessConfig(struct pci_bus *bus, unsigned int devfn, u8 o
>
>         if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, &vendID) == -1)
>                 return -1;
> -       if (vendID == 0xffffffff)
> +       if (vendID == (u32) PCI_ERROR_RESPONSE)
>                 return -1;
>         return pci_bus_read_config_dword(bus, devfn, offset, value);
>  }
> @@ -251,7 +251,7 @@ static int PCI_GetBusDevHelper(struct controller *ctrl, u8 *bus_num, u8 *dev_num
>                         *dev_num = tdevice;
>                         ctrl->pci_bus->number = tbus;
>                         pci_bus_read_config_dword(ctrl->pci_bus, *dev_num, PCI_VENDOR_ID, &work);
> -                       if (!nobridge || (work == 0xffffffff))
> +                       if (!nobridge || (work == (u32) PCI_ERROR_RESPONSE))
>                                 return 0;
>
>                         dbg("bus_num %d devfn %d\n", *bus_num, *dev_num);
> @@ -330,10 +330,10 @@ int cpqhp_save_config(struct controller *ctrl, int busnumber, int is_hot_plug)
>         /* Save PCI configuration space for all devices in supported slots */
>         ctrl->pci_bus->number = busnumber;
>         for (device = FirstSupported; device <= LastSupported; device++) {
> -               ID = 0xFFFFFFFF;
> +               ID = (u32) PCI_ERROR_RESPONSE;
>                 rc = pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(device, 0), PCI_VENDOR_ID, &ID);
>
> -               if (ID == 0xFFFFFFFF) {
> +               if (ID == (u32) PCI_ERROR_RESPONSE) {
>                         if (is_hot_plug) {
>                                 /* Setup slot structure with entry for empty
>                                  * slot
> @@ -431,7 +431,7 @@ int cpqhp_save_config(struct controller *ctrl, int busnumber, int is_hot_plug)
>                          */
>                         while ((function < max_functions) && (!stop_it)) {
>                                 rc = pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(device, function), PCI_VENDOR_ID, &ID);
> -                               if (ID == 0xFFFFFFFF) {
> +                               if (ID == (u32) PCI_ERROR_RESPONSE) {
>                                         function++;
>                                         continue;
>                                 }
> @@ -474,12 +474,12 @@ int cpqhp_save_slot_config(struct controller *ctrl, struct pci_func *new_slot)
>         int cloop = 0;
>         int stop_it;
>
> -       ID = 0xFFFFFFFF;
> +       ID = (u32) PCI_ERROR_RESPONSE;
>
>         ctrl->pci_bus->number = new_slot->bus;
>         pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(new_slot->device, 0), PCI_VENDOR_ID, &ID);
>
> -       if (ID == 0xFFFFFFFF)
> +       if (ID == (u32) PCI_ERROR_RESPONSE)
>                 return 2;
>
>         pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(new_slot->device, 0), 0x0B, &class_code);
> @@ -522,7 +522,7 @@ int cpqhp_save_slot_config(struct controller *ctrl, struct pci_func *new_slot)
>                 while ((function < max_functions) && (!stop_it)) {
>                         pci_bus_read_config_dword(ctrl->pci_bus, PCI_DEVFN(new_slot->device, function), PCI_VENDOR_ID, &ID);
>
> -                       if (ID == 0xFFFFFFFF)
> +                       if (ID == (u32) PCI_ERROR_RESPONSE)
>                                 function++;
>                         else {
>                                 pci_bus_read_config_byte(ctrl->pci_bus, PCI_DEVFN(new_slot->device, function), 0x0B, &class_code);
> @@ -1049,7 +1049,7 @@ int cpqhp_valid_replace(struct controller *ctrl, struct pci_func *func)
>                 pci_bus_read_config_dword(pci_bus, devfn, PCI_VENDOR_ID, &temp_register);
>
>                 /* No adapter present */
> -               if (temp_register == 0xFFFFFFFF)
> +               if (temp_register == (u32) PCI_ERROR_RESPONSE)
>                         return(NO_ADAPTER_PRESENT);
>
>                 if (temp_register != func->config_space[0])
> @@ -1267,7 +1267,7 @@ int cpqhp_find_available_resources(struct controller *ctrl, void __iomem *rom_st
>                 pci_bus_read_config_dword(ctrl->pci_bus, dev_func, PCI_VENDOR_ID, &temp_dword);
>                 dbg("temp_D_word = %x\n", temp_dword);
>
> -               if (temp_dword != 0xFFFFFFFF) {
> +               if (temp_dword != (u32) PCI_ERROR_RESPONSE) {
>                         index = 0;
>                         func = cpqhp_slot_find(primary_bus, dev_func >> 3, 0);
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bd990e3371e3..f0489f305ad7 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -70,7 +70,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
>
>         while (true) {
>                 pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
> -               if (slot_status == (u16) ~0) {
> +               if (slot_status == (u16) PCI_ERROR_RESPONSE) {
>                         ctrl_info(ctrl, "%s: no response from device\n",
>                                   __func__);
>                         return 0;
> @@ -148,7 +148,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
>         pcie_wait_cmd(ctrl);
>
>         pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
> -       if (slot_ctrl == (u16) ~0) {
> +       if (slot_ctrl == (u16) PCI_ERROR_RESPONSE) {
>                 ctrl_info(ctrl, "%s: no response from device\n", __func__);
>                 goto out;
>         }
> @@ -543,7 +543,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>         }
>
>         pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
> -       if (status == (u16) ~0) {
> +       if (status == (u16) PCI_ERROR_RESPONSE) {
>                 ctrl_info(ctrl, "%s: no response from device\n", __func__);
>                 if (parent)
>                         pm_runtime_put(parent);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..bfc739dc6ada 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4434,16 +4434,16 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
>          * After reset, the device should not silently discard config
>          * requests, but it may still indicate that it needs more time by
>          * responding to them with CRS completions.  The Root Port will
> -        * generally synthesize ~0 data to complete the read (except when
> -        * CRS SV is enabled and the read was for the Vendor ID; in that
> -        * case it synthesizes 0x0001 data).
> +        * generally synthesize ~0 data (PCI_ERROR_RESPONSE) to complete
> +        * the read (except when CRS SV is enabled and the read was for the
> +        * Vendor ID; in that case it synthesizes 0x0001 data).
>          *
>          * Wait for the device to return a non-CRS completion.  Read the
>          * Command register instead of Vendor ID so we don't have to
>          * contend with the CRS SV value.
>          */
>         pci_read_config_dword(dev, PCI_COMMAND, &id);
> -       while (id == ~0) {
> +       while (id == (u32) PCI_ERROR_RESPONSE) {
>                 if (delay > timeout) {
>                         pci_warn(dev, "not ready %dms after %s; giving up\n",
>                                  delay - 1, reset_type);
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index a32ec3487a8d..96b6b87a0af3 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -272,7 +272,8 @@ static irqreturn_t dpc_irq(int irq, void *context)
>
>         pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
>
> -       if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) || status == (u16)(~0))
> +       if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT) ||
> +           status == (u16)(PCI_ERROR_RESPONSE))
>                 return IRQ_NONE;
>
>         pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index f38e6c19dd50..3c46622e6c3f 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -224,7 +224,7 @@ static void pcie_pme_work_fn(struct work_struct *work)
>                         break;
>
>                 pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
> -               if (rtsta == (u32) ~0)
> +               if (rtsta == (u32) PCI_ERROR_RESPONSE)
>                         break;
>
>                 if (rtsta & PCI_EXP_RTSTA_PME) {
> @@ -274,7 +274,7 @@ static irqreturn_t pcie_pme_irq(int irq, void *context)
>         spin_lock_irqsave(&data->lock, flags);
>         pcie_capability_read_dword(port, PCI_EXP_RTSTA, &rtsta);
>
> -       if (rtsta == (u32) ~0 || !(rtsta & PCI_EXP_RTSTA_PME)) {
> +       if (rtsta == (u32) PCI_ERROR_RESPONSE || !(rtsta & PCI_EXP_RTSTA_PME)) {
>                 spin_unlock_irqrestore(&data->lock, flags);
>                 return IRQ_NONE;
>         }
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index a3c7338fad86..fb953f2666b9 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1549,7 +1549,7 @@ static int pci_cfg_space_size_ext(struct pci_dev *dev)
>
>         if (pci_read_config_dword(dev, pos, &status) != PCIBIOS_SUCCESSFUL)
>                 return PCI_CFG_SPACE_SIZE;
> -       if (status == 0xffffffff || pci_ext_cfg_is_aliased(dev))
> +       if (status == (u32) PCI_ERROR_RESPONSE || pci_ext_cfg_is_aliased(dev))
>                 return PCI_CFG_SPACE_SIZE;
>
>         return PCI_CFG_SPACE_EXP_SIZE;
> @@ -2488,7 +2488,7 @@ bool pci_bus_generic_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>                 return false;
>
>         /* Some broken boards return 0 or ~0 if a slot is empty: */
> -       if (*l == 0xffffffff || *l == 0x00000000 ||
> +       if (*l == (u32) PCI_ERROR_RESPONSE || *l == 0x00000000 ||
>             *l == 0x0000ffff || *l == 0xffff0000)
>                 return false;
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 208aacf39329..3d5c92b53310 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4854,7 +4854,7 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
>
>                 pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
>                 if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
> -                   PCIBIOS_SUCCESSFUL || (status == 0xffffffff))
> +                   PCIBIOS_SUCCESSFUL || (status == (u32) PCI_ERROR_RESPONSE))
>                         pdev->cfg_size = PCI_CFG_SPACE_SIZE;
>
>                 if (pci_find_saved_cap(pdev, PCI_CAP_ID_EXP))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9e700d9f9f28..d64fd3788061 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -123,6 +123,13 @@ enum pci_interrupt_pin {
>  /* The number of legacy PCI INTx interrupts */
>  #define PCI_NUM_INTX   4
>
> +/*
> + * Reading from a device that doesn't respond typically returns ~0.  A
> + * successful read from a device may also return ~0, so you need additional
> + * information to reliably identify errors.
> + */
> +#define PCI_ERROR_RESPONSE             (~0ULL)
> +
>  /*
>   * pci_power_t values must match the bits in the Capabilities PME_Support
>   * and Control/Status PowerState fields in the Power Management capability.
> --
> 2.22.0.770.g0f2c4a37fd-goog
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] PCI / PM: Check for error when reading PME status
  2019-08-05 21:02   ` Rafael J. Wysocki
@ 2019-08-06 13:36     ` Bjorn Helgaas
  2019-08-13 23:26       ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-06 13:36 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Mon, Aug 05, 2019 at 11:02:51PM +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > pci_check_pme_status() reads the Power Management capability to determine
> > whether a device has generated a PME.  The capability is in config space,
> > which is accessible in D0, D1, D2, and D3hot, but not in D3cold.
> >
> > If we call pci_check_pme_status() on a device that's in D3cold, config
> > reads fail and return ~0 data, which we erroneously interpreted as "the
> > device has generated a PME".
> >
> > 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > avoided many of these problems by avoiding pci_check_pme_status() if we
> > think the device is in D3cold.  However, it is not a complete fix because
> > the device may go to D3cold after we check its power state but before
> > pci_check_pme_status() reads the Power Management Status Register.
> >
> > Return false ("device has not generated a PME") if we get an error response
> > reading the Power Management Status Register.
> >
> > Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pci.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 984171d40858..af6a97d7012b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
> >
> >         pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
> >         pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> > +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> > +               return false;
> 
> No, sorry.
> 
> We tried that and it didn't work.
> 
> pcie_pme_handle_request() depends on this returning "true" for all
> bits set, as from its perspective "device is not accessible" may very
> well mean "device may have signaled PME".

Right, it's obviously wrong in the case of devices that advertise
D3cold in PME_Support, i.e., devices that can generate PME even with
main power off.  Also, we may want to try to wake up devices if the
config read fails for a reason other than the device being in D3cold.

What I don't like about the current code is that it checks
PCI_PM_CTRL_PME_STATUS in data that may be completely bogus.  Do you
think it would be better to do something like this:

  pci_read_config_word(dev, pmcsr_pos, &pmcsr);
  if (pmcsr == (u16) PCI_ERROR_RESPONSE) {
    if (pci_pme_capable(dev, PCI_PM_CAP_PME_D3cold))
      return true;
    return false;
  }

or maybe this:

  pci_read_config_word(dev, pmcsr_pos, &pmcsr);
  if (pmcsr == (u16) PCI_ERROR_RESPONSE)
    return true;

We should get PCI_ERROR_RESPONSE pretty reliably from devices in
D3cold, so the first possibility would cover that case.

But since pci_check_pme_status() basically returns a hint ("true"
means a device *may* have generated a PME), and even if the hint is
wrong, the worst that happens is an unnecessary wakeup, maybe the
second possibility is safer.

What do you think?

> >         if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
> >                 return false;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] PCI / PM: Check for error when reading Power State
  2019-08-05 21:09   ` Rafael J. Wysocki
@ 2019-08-09 22:01     ` Bjorn Helgaas
  2019-08-13 22:59       ` Rafael J. Wysocki
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-09 22:01 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PCI, Lorenzo Pieralisi, Keith Busch, Greg Kroah-Hartman,
	Rafael J . Wysocki, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Mon, Aug 05, 2019 at 11:09:19PM +0200, Rafael J. Wysocki wrote:
> On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > The Power Management Status Register is in config space, and reads while
> > the device is in D3cold typically return ~0 data (PCI_ERROR_RESPONSE).  If
> > we just look at the PCI_PM_CTRL_STATE_MASK bits, that is 0x3, which looks
> > like D3hot, not D3cold.
> >
> > Check the entire register for PCI_ERROR_RESPONSE so we can distinguish
> > D3cold from D3hot.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pci.c   |  6 +++---
> >  include/linux/pci.h | 13 +++++++++++++
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index af6a97d7012b..d8686e3cd5eb 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -894,7 +894,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
> >                 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 = pci_power_state(pmcsr);
> 
> But pci_raw_set_power_state() should not even be called for devices in
> D3_cold, so this at best is redundant.

I tried to verify that we don't call pci_raw_set_power_state() for
devices in D3cold, but it wasn't obvious to me.  Is there an easy way
to verify that?  I'd rather have code that doesn't rely on deep
knowledge about other areas.

Even if the device was in, say D0, what if it is hot-removed just
before we read PCI_PM_CTRL?  We'll set dev->current_state to D3hot,
when I think D3cold would better correspond to the state of the
device.  Maybe that's harmless, but I don't know how to verify that.

> >         if (dev->current_state != state && printk_ratelimit())
> >                 pci_info(dev, "Refused to change power state, currently in D%d\n",
> >                          dev->current_state);
> > @@ -942,7 +942,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
> >                 u16 pmcsr;
> >
> >                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > +               dev->current_state = pci_power_state(pmcsr);
> 
> The if () branch above should cover the D3cold case, shouldn't it?

You mean the "if (platform_pci_get_power_state(dev) == PCI_D3cold)"
test?

platform_pci_get_power_state() returns PCI_UNKNOWN in some cases.
When that happens, might we not read PCI_PM_CTRL of a device in
D3cold?  I think this also has the same hotplug question as above.

> >         } else {
> >                 dev->current_state = state;
> >         }
> > @@ -1677,7 +1677,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> >         if (dev->pm_cap) {
> >                 u16 pmcsr;
> >                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > +               dev->current_state = pci_power_state(pmcsr);
> 
> So this appears to be only case in which pci_power_state(pmcsr) is
> useful at all.
> 
> It might be better to use the code from it directly here IMO.

If we're decoding CSR values, I think it's better to notice error
responses when we can than it is to try to figure out whether the
error response is theoretically impossible or the incorrectly decoded
value (e.g., D3hot instead of D3cold) is harmless.

> >         }
> >
> >         if (atomic_inc_return(&dev->enable_cnt) > 1)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index d64fd3788061..fdfe990e9661 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -152,6 +152,19 @@ static inline const char *pci_power_name(pci_power_t state)
> >         return pci_power_names[1 + (__force int) state];
> >  }
> >
> > +/*
> > + * Convert a Power Management Status Register value to a pci_power_t.
> > + * Note that if we read the register while the device is in D3cold, we
> > + * typically get PCI_ERROR_RESPONSE, which looks like D3hot (0x3) if we
> > + * only look at the PCI_PM_CTRL_STATE_MASK bits.
> > + */
> > +static inline pci_power_t pci_power_state(u16 pmcsr)
> > +{
> > +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> > +               return PCI_D3cold;
> > +       return pmcsr & PCI_PM_CTRL_STATE_MASK;
> > +}
> > +
> >  #define PCI_PM_D2_DELAY                200
> >  #define PCI_PM_D3_WAIT         10
> >  #define PCI_PM_D3COLD_WAIT     100
> > --
> > 2.22.0.770.g0f2c4a37fd-goog
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] PCI / PM: Check for error when reading Power State
  2019-08-09 22:01     ` Bjorn Helgaas
@ 2019-08-13 22:59       ` Rafael J. Wysocki
  2019-08-14  1:08         ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-13 22:59 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J. Wysocki, Linux PCI, Lorenzo Pieralisi, Keith Busch,
	Greg Kroah-Hartman, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Saturday, August 10, 2019 12:01:16 AM CEST Bjorn Helgaas wrote:
> On Mon, Aug 05, 2019 at 11:09:19PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > The Power Management Status Register is in config space, and reads while
> > > the device is in D3cold typically return ~0 data (PCI_ERROR_RESPONSE).  If
> > > we just look at the PCI_PM_CTRL_STATE_MASK bits, that is 0x3, which looks
> > > like D3hot, not D3cold.
> > >
> > > Check the entire register for PCI_ERROR_RESPONSE so we can distinguish
> > > D3cold from D3hot.
> > >
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/pci.c   |  6 +++---
> > >  include/linux/pci.h | 13 +++++++++++++
> > >  2 files changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index af6a97d7012b..d8686e3cd5eb 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -894,7 +894,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
> > >                 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 = pci_power_state(pmcsr);
> > 
> > But pci_raw_set_power_state() should not even be called for devices in
> > D3_cold, so this at best is redundant.
> 
> I tried to verify that we don't call pci_raw_set_power_state() for
> devices in D3cold, but it wasn't obvious to me.  Is there an easy way
> to verify that?  I'd rather have code that doesn't rely on deep
> knowledge about other areas.

It is called in two places, pci_power_up() and pci_set_power_state().

pci_power_up() is called on resume when the whole hierarchy is
turned on and pci_set_power_state() explicitly powers up the
device if in D3cold (with the help of the platform).

And the "device not accessible at all" case should be covered by patch [2/5]
in this series.

> Even if the device was in, say D0, what if it is hot-removed just
> before we read PCI_PM_CTRL?

I guess you mean surprise-hot-removed?

Then it may as well be hot-removed after setting current_state.

> We'll set dev->current_state to D3hot,
> when I think D3cold would better correspond to the state of the
> device.  Maybe that's harmless, but I don't know how to verify that.

Well, D3cold may just be equally misleading, because the device may
very well not be present at all any more.

> > >         if (dev->current_state != state && printk_ratelimit())
> > >                 pci_info(dev, "Refused to change power state, currently in D%d\n",
> > >                          dev->current_state);
> > > @@ -942,7 +942,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
> > >                 u16 pmcsr;
> > >
> > >                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > > +               dev->current_state = pci_power_state(pmcsr);
> > 
> > The if () branch above should cover the D3cold case, shouldn't it?
> 
> You mean the "if (platform_pci_get_power_state(dev) == PCI_D3cold)"
> test?

Not exactly.

I mean "if (platform_pci_get_power_state(dev) == PCI_D3cold ||
!pci_device_is_present(dev))".

> platform_pci_get_power_state() returns PCI_UNKNOWN in some cases.
> When that happens, might we not read PCI_PM_CTRL of a device in
> D3cold?  I think this also has the same hotplug question as above.

Surprise hot-removal can take place at any time, in particular after setting
current_state, so adding extra checks here doesn't prevent the value of
it from becoming stale at least sometimes anyway.

> > >         } else {
> > >                 dev->current_state = state;
> > >         }
> > > @@ -1677,7 +1677,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> > >         if (dev->pm_cap) {
> > >                 u16 pmcsr;
> > >                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > > +               dev->current_state = pci_power_state(pmcsr);
> > 
> > So this appears to be only case in which pci_power_state(pmcsr) is
> > useful at all.
> > 
> > It might be better to use the code from it directly here IMO.
> 
> If we're decoding CSR values, I think it's better to notice error
> responses when we can than it is to try to figure out whether the
> error response is theoretically impossible or the incorrectly decoded
> value (e.g., D3hot instead of D3cold) is harmless.

IMO this means more complex code and extra overhead for a very
little practical value, however.




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] PCI / PM: Check for error when reading PME status
  2019-08-06 13:36     ` Bjorn Helgaas
@ 2019-08-13 23:26       ` Rafael J. Wysocki
  2019-08-14  1:15         ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Rafael J. Wysocki @ 2019-08-13 23:26 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J. Wysocki, Linux PCI, Lorenzo Pieralisi, Keith Busch,
	Greg Kroah-Hartman, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Tuesday, August 6, 2019 3:36:38 PM CEST Bjorn Helgaas wrote:
> On Mon, Aug 05, 2019 at 11:02:51PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > pci_check_pme_status() reads the Power Management capability to determine
> > > whether a device has generated a PME.  The capability is in config space,
> > > which is accessible in D0, D1, D2, and D3hot, but not in D3cold.
> > >
> > > If we call pci_check_pme_status() on a device that's in D3cold, config
> > > reads fail and return ~0 data, which we erroneously interpreted as "the
> > > device has generated a PME".
> > >
> > > 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > > avoided many of these problems by avoiding pci_check_pme_status() if we
> > > think the device is in D3cold.  However, it is not a complete fix because
> > > the device may go to D3cold after we check its power state but before
> > > pci_check_pme_status() reads the Power Management Status Register.
> > >
> > > Return false ("device has not generated a PME") if we get an error response
> > > reading the Power Management Status Register.
> > >
> > > Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > > Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/pci.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 984171d40858..af6a97d7012b 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
> > >
> > >         pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
> > >         pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> > > +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> > > +               return false;
> > 
> > No, sorry.
> > 
> > We tried that and it didn't work.
> > 
> > pcie_pme_handle_request() depends on this returning "true" for all
> > bits set, as from its perspective "device is not accessible" may very
> > well mean "device may have signaled PME".
> 
> Right, it's obviously wrong in the case of devices that advertise
> D3cold in PME_Support, i.e., devices that can generate PME even with
> main power off.  Also, we may want to try to wake up devices if the
> config read fails for a reason other than the device being in D3cold.
> 
> What I don't like about the current code is that it checks
> PCI_PM_CTRL_PME_STATUS in data that may be completely bogus.

Whether or not the other bits in the register make sense doesn't
matter here.  Only the PME_STATUS bit matters.

> Do you think it would be better to do something like this:
> 
>   pci_read_config_word(dev, pmcsr_pos, &pmcsr);
>   if (pmcsr == (u16) PCI_ERROR_RESPONSE) {
>     if (pci_pme_capable(dev, PCI_PM_CAP_PME_D3cold))
>       return true;
>     return false;
>   }
> 
> or maybe this:
> 
>   pci_read_config_word(dev, pmcsr_pos, &pmcsr);
>   if (pmcsr == (u16) PCI_ERROR_RESPONSE)
>     return true;

In this case it still would be prudent to check PME_ENABLE before
returning true and so there is no practical difference between
ERROR_RESPONSE and the valid data with PME_STATUS set.

Except that in the ERROR_RESPONSE case we may as well avoid the
PMCSR write which is not going to make a difference.

> We should get PCI_ERROR_RESPONSE pretty reliably from devices in
> D3cold, so the first possibility would cover that case.
>
> But since pci_check_pme_status() basically returns a hint ("true"
> means a device *may* have generated a PME), and even if the hint is
> wrong, the worst that happens is an unnecessary wakeup, maybe the
> second possibility is safer.
> 
> What do you think?

So if you really want to avoid the PMCSR write in the ERROR_RESPONSE case,
something like this can be done IMO:

 			return false;
 
 	/* Clear PME status. */
-	pmcsr |= PCI_PM_CTRL_PME_STATUS;
 	if (pmcsr & PCI_PM_CTRL_PME_ENABLE) {
+		if (pmcsr == (u16) PCI_ERROR_RESPONSE)
+			return true;
+
 		/* Disable PME to avoid interrupt flood. */
 		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
 		ret = true;





^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] PCI / PM: Check for error when reading Power State
  2019-08-13 22:59       ` Rafael J. Wysocki
@ 2019-08-14  1:08         ` Bjorn Helgaas
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-14  1:08 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linux PCI, Lorenzo Pieralisi, Keith Busch,
	Greg Kroah-Hartman, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Wed, Aug 14, 2019 at 12:59:26AM +0200, Rafael J. Wysocki wrote:
> On Saturday, August 10, 2019 12:01:16 AM CEST Bjorn Helgaas wrote:
> > On Mon, Aug 05, 2019 at 11:09:19PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> > > > @@ -942,7 +942,7 @@ void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
> > > >                 u16 pmcsr;
> > > >
> > > >                 pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > > -               dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> > > > +               dev->current_state = pci_power_state(pmcsr);
> > > 
> > > The if () branch above should cover the D3cold case, shouldn't it?
> > 
> > You mean the "if (platform_pci_get_power_state(dev) == PCI_D3cold)"
> > test?
> 
> Not exactly.
> 
> I mean "if (platform_pci_get_power_state(dev) == PCI_D3cold ||
> !pci_device_is_present(dev))".

I don't see what you mean.  The !pci_device_is_present(dev) test tells
us something about what the state of the device was at some time in
the past, but of course it doesn't say anything about whether reading
PCI_PM_CTRL will succeed, e.g.,

  # dev is present and in D0
  platform_pci_get_power_state(dev) == PCI_D3cold   # currently false
  !pci_device_is_present(dev)                       # currently false
  # dev is surprise hot-removed or put in D3cold
  pci_read_config_word(PCI_PM_CTRL, &pmcsr)
  # pmcsr == ~0 (error response)

(Maybe going to D3cold is impossible, but it's pretty hard to prove
that.  The hot-remove is definitely possible.)

> > platform_pci_get_power_state() returns PCI_UNKNOWN in some cases.
> > When that happens, might we not read PCI_PM_CTRL of a device in
> > D3cold?  I think this also has the same hotplug question as above.
> 
> Surprise hot-removal can take place at any time, in particular after setting
> current_state, so adding extra checks here doesn't prevent the value of
> it from becoming stale at least sometimes anyway.

Definitely.  The point is not to prevent current_state from becoming
stale, it's to prevent us from interpreting ~0 data (known to be
invalid) as though it were a valid register value.

Bjorn

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] PCI / PM: Check for error when reading PME status
  2019-08-13 23:26       ` Rafael J. Wysocki
@ 2019-08-14  1:15         ` Bjorn Helgaas
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2019-08-14  1:15 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Linux PCI, Lorenzo Pieralisi, Keith Busch,
	Greg Kroah-Hartman, Mika Westerberg, Linux PM,
	Linux Kernel Mailing List

On Wed, Aug 14, 2019 at 01:26:56AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, August 6, 2019 3:36:38 PM CEST Bjorn Helgaas wrote:
> > On Mon, Aug 05, 2019 at 11:02:51PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > pci_check_pme_status() reads the Power Management capability to determine
> > > > whether a device has generated a PME.  The capability is in config space,
> > > > which is accessible in D0, D1, D2, and D3hot, but not in D3cold.
> > > >
> > > > If we call pci_check_pme_status() on a device that's in D3cold, config
> > > > reads fail and return ~0 data, which we erroneously interpreted as "the
> > > > device has generated a PME".
> > > >
> > > > 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > > > avoided many of these problems by avoiding pci_check_pme_status() if we
> > > > think the device is in D3cold.  However, it is not a complete fix because
> > > > the device may go to D3cold after we check its power state but before
> > > > pci_check_pme_status() reads the Power Management Status Register.
> > > >
> > > > Return false ("device has not generated a PME") if we get an error response
> > > > reading the Power Management Status Register.
> > > >
> > > > Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> > > > Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/pci.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 984171d40858..af6a97d7012b 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
> > > >
> > > >         pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
> > > >         pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> > > > +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> > > > +               return false;
> > > 
> > > No, sorry.
> > > 
> > > We tried that and it didn't work.
> > > 
> > > pcie_pme_handle_request() depends on this returning "true" for all
> > > bits set, as from its perspective "device is not accessible" may very
> > > well mean "device may have signaled PME".
> > 
> > Right, it's obviously wrong in the case of devices that advertise
> > D3cold in PME_Support, i.e., devices that can generate PME even with
> > main power off.  Also, we may want to try to wake up devices if the
> > config read fails for a reason other than the device being in D3cold.
> > 
> > What I don't like about the current code is that it checks
> > PCI_PM_CTRL_PME_STATUS in data that may be completely bogus.
> 
> Whether or not the other bits in the register make sense doesn't
> matter here.  Only the PME_STATUS bit matters.

Of course.  It just relies on the implicit assumption that the bit in
the error response matches the PME_STATUS state that we want, which is
a little bit ugly.

> > Do you think it would be better to do something like this:
> > 
> >   pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> >   if (pmcsr == (u16) PCI_ERROR_RESPONSE) {
> >     if (pci_pme_capable(dev, PCI_PM_CAP_PME_D3cold))
> >       return true;
> >     return false;
> >   }
> > 
> > or maybe this:
> > 
> >   pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> >   if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> >     return true;
> 
> In this case it still would be prudent to check PME_ENABLE before
> returning true and so there is no practical difference between
> ERROR_RESPONSE and the valid data with PME_STATUS set.
> 
> Except that in the ERROR_RESPONSE case we may as well avoid the
> PMCSR write which is not going to make a difference.
> 
> > We should get PCI_ERROR_RESPONSE pretty reliably from devices in
> > D3cold, so the first possibility would cover that case.
> >
> > But since pci_check_pme_status() basically returns a hint ("true"
> > means a device *may* have generated a PME), and even if the hint is
> > wrong, the worst that happens is an unnecessary wakeup, maybe the
> > second possibility is safer.
> > 
> > What do you think?
> 
> So if you really want to avoid the PMCSR write in the ERROR_RESPONSE case,
> something like this can be done IMO:
> 
>  			return false;
>  
>  	/* Clear PME status. */
> -	pmcsr |= PCI_PM_CTRL_PME_STATUS;
>  	if (pmcsr & PCI_PM_CTRL_PME_ENABLE) {
> +		if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> +			return true;
> +
>  		/* Disable PME to avoid interrupt flood. */
>  		pmcsr &= ~PCI_PM_CTRL_PME_ENABLE;
>  		ret = true;

Agreed, that's the conclusion I came to as well.  I wouldn't do this
just to avoid the config write, since as you mentioned that will get
dropped anyway.  The reason I would consider this is as an example of
how drivers might think about validating data they read from devices.

Bjorn

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-08-14  1:15 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-05 20:52 [PATCH 0/5] PCI: Add PCI_ERROR_RESPONSE, check for errors Bjorn Helgaas
2019-08-05 20:52 ` [PATCH 1/5] PCI: Add PCI_ERROR_RESPONSE definition Bjorn Helgaas
2019-08-05 21:16   ` Rafael J. Wysocki
2019-08-05 20:52 ` [PATCH 2/5] PCI / PM: Return error when changing power state from D3cold Bjorn Helgaas
2019-08-05 21:15   ` Rafael J. Wysocki
2019-08-05 20:52 ` [PATCH 3/5] PCI / PM: Check for error when reading PME status Bjorn Helgaas
2019-08-05 21:02   ` Rafael J. Wysocki
2019-08-06 13:36     ` Bjorn Helgaas
2019-08-13 23:26       ` Rafael J. Wysocki
2019-08-14  1:15         ` Bjorn Helgaas
2019-08-05 20:52 ` [PATCH 4/5] PCI / PM: Check for error when reading Power State Bjorn Helgaas
2019-08-05 21:09   ` Rafael J. Wysocki
2019-08-09 22:01     ` Bjorn Helgaas
2019-08-13 22:59       ` Rafael J. Wysocki
2019-08-14  1:08         ` Bjorn Helgaas
2019-08-05 20:52 ` [PATCH 5/5] PCI / PM: Decode D3cold power state correctly Bjorn Helgaas
2019-08-05 21:14   ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).