[PATCH v2 00/14] PCI/DPC: Simplify RP PIO logging

[PATCH v2 00/14] PCI/DPC: Simplify RP PIO logging

[Bjorn Helgaas]

These are almost all just cleanups.

The only behavior change I intend is this: If a port does not support
the "RP Extensions for DPC" and it reports an "RP PIO error", we
previously read the RP PIO log registers.  I don't know if that's a
legal situation, but I couldn't find an explicit prohibition in the
spec.

Anyway, the rest is all cleanups and simplifications that are not
intended to change any behavior.

These are currently on my pci/dpc branch.

Changes from v1 to v2:
  - Split up the "Consolidate RP PIO get/print functions" patch to
    make it more reviewable.  The resulting git diff is empty except
    for a trival local variable declaration.

---

Bjorn Helgaas (14):
      PCI/DPC: Rename interrupt_event_handler() to dpc_work()
      PCI/DPC: Add local variable for DPC capability offset
      PCI/DPC: Rename struct dpc_dev.rp to rp_extensions
      PCI/DPC: Read RP PIO Log Size once at probe
      PCI/DPC: Process RP PIO details only if RP PIO extensions supported
      PCI/DPC: Squash dpc_rp_pio_print_tlp_header() into dpc_rp_pio_print_error()
      PCI/DPC: Rename local "status" to "dpc_status"
      PCI/DPC: Make RP PIO log size check more generic
      PCI/DPC: Squash dpc_rp_pio_print_error() into dpc_rp_pio_get_info()
      PCI/DPC: Push dpc->rp_pio_status assignment into dpc_rp_pio_get_info()
      PCI/DPC: Remove unnecessary RP PIO register structs
      PCI/DPC: Squash dpc_rp_pio_get_info() into dpc_process_rp_pio_error()
      PCI/DPC: Add and use DPC Status register field definitions
      PCI/DPC: Reformat DPC register definitions


 drivers/pci/pcie/pcie-dpc.c   |  214 +++++++++++++++--------------------------
 include/uapi/linux/pci_regs.h |   24 ++---
 2 files changed, 91 insertions(+), 147 deletions(-)

[PATCH v2 01/14] PCI/DPC: Rename interrupt_event_handler() to dpc_work()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Rename interrupt_event_handler() to dpc_work() so there's more useful
information in stack traces and similar situations.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 0515abd20a48..48e1e3f8dd69 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -105,7 +105,7 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
 		dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
-static void interrupt_event_handler(struct work_struct *work)
+static void dpc_work(struct work_struct *work)
 {
 	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
@@ -319,7 +319,7 @@ static int dpc_probe(struct pcie_device *dev)
 
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
-	INIT_WORK(&dpc->work, interrupt_event_handler);
+	INIT_WORK(&dpc->work, dpc_work);
 	set_service_data(dev, dpc);
 
 	status = devm_request_irq(device, dev->irq, dpc_irq, IRQF_SHARED,

[PATCH v2 02/14] PCI/DPC: Add local variable for DPC capability offset

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add a local variable for DPC capability offset and replace repeated use of
"dpc->cap_pos" with simply "cap".  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c |   65 +++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 48e1e3f8dd69..52702f7b911a 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -41,7 +41,7 @@ struct dpc_rp_pio_regs {
 struct dpc_dev {
 	struct pcie_device	*dev;
 	struct work_struct	work;
-	int			cap_pos;
+	u16			cap_pos;
 	bool			rp;
 	u32			rp_pio_status;
 };
@@ -73,13 +73,13 @@ static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 	unsigned long timeout = jiffies + HZ;
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
-	u16 status;
+	u16 cap = dpc->cap_pos, status;
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	while (status & PCI_EXP_DPC_RP_BUSY &&
 					!time_after(jiffies, timeout)) {
 		msleep(10);
-		pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
+		pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	}
 	if (status & PCI_EXP_DPC_RP_BUSY) {
 		dev_warn(dev, "DPC root port still busy\n");
@@ -110,7 +110,7 @@ static void dpc_work(struct work_struct *work)
 	struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
 	struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
 	struct pci_bus *parent = pdev->subordinate;
-	u16 ctl;
+	u16 cap = dpc->cap_pos, ctl;
 
 	pci_lock_rescan_remove();
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
@@ -129,17 +129,16 @@ static void dpc_work(struct work_struct *work)
 	if (dpc->rp && dpc_wait_rp_inactive(dpc))
 		return;
 	if (dpc->rp && dpc->rp_pio_status) {
-		pci_write_config_dword(pdev,
-				      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_STATUS,
-				      dpc->rp_pio_status);
+		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
+				       dpc->rp_pio_status);
 		dpc->rp_pio_status = 0;
 	}
 
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 		PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
@@ -189,23 +188,22 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
 	int i;
-	u16 cap;
-	u16 status;
+	u16 cap = dpc->cap_pos, status;
 
-	pci_read_config_dword(pdev, dpc->cap_pos + PCI_EXP_DPC_RP_PIO_STATUS,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
 			      &rp_pio->status);
-	pci_read_config_dword(pdev, dpc->cap_pos + PCI_EXP_DPC_RP_PIO_MASK,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK,
 			      &rp_pio->mask);
 
-	pci_read_config_dword(pdev, dpc->cap_pos + PCI_EXP_DPC_RP_PIO_SEVERITY,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY,
 			      &rp_pio->severity);
-	pci_read_config_dword(pdev, dpc->cap_pos + PCI_EXP_DPC_RP_PIO_SYSERROR,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR,
 			      &rp_pio->syserror);
-	pci_read_config_dword(pdev, dpc->cap_pos + PCI_EXP_DPC_RP_PIO_EXCEPTION,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION,
 			      &rp_pio->exception);
 
 	/* Get First Error Pointer */
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	rp_pio->first_error = (status & 0x1f00) >> 8;
 
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
@@ -216,27 +214,22 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 		return;
 	}
 
-	pci_read_config_dword(pdev,
-			      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
 			      &rp_pio->header_log.dw0);
-	pci_read_config_dword(pdev,
-			      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
 			      &rp_pio->header_log.dw1);
-	pci_read_config_dword(pdev,
-			      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 8,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 8,
 			      &rp_pio->header_log.dw2);
-	pci_read_config_dword(pdev,
-			      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
 			      &rp_pio->header_log.dw3);
 	if (rp_pio->log_size == 4)
 		return;
 
-	pci_read_config_dword(pdev,
-			      dpc->cap_pos + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
 			      &rp_pio->impspec_log);
 	for (i = 0; i < rp_pio->log_size - 5; i++)
 		pci_read_config_dword(pdev,
-			dpc->cap_pos + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
+			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
 			&rp_pio->tlp_prefix_log[i]);
 }
 
@@ -255,28 +248,28 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	struct dpc_dev *dpc = (struct dpc_dev *)context;
 	struct pci_dev *pdev = dpc->dev->port;
 	struct device *dev = &dpc->dev->device;
-	u16 ctl, status, source, reason, ext_reason;
+	u16 cap = dpc->cap_pos, ctl, status, source, reason, ext_reason;
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
 
 	if (!(ctl & PCI_EXP_DPC_CTL_INT_EN) || ctl == (u16)(~0))
 		return IRQ_NONE;
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 
 	if (!(status & PCI_EXP_DPC_STATUS_INTERRUPT))
 		return IRQ_NONE;
 
 	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
-		pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
+		pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
 				      PCI_EXP_DPC_STATUS_INTERRUPT);
 		return IRQ_HANDLED;
 	}
 
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
+	pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
 			      ctl & ~PCI_EXP_DPC_CTL_INT_EN);
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_SOURCE_ID,
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_SOURCE_ID,
 			     &source);
 
 	dev_info(dev, "DPC containment event, status:%#06x source:%#06x\n",

[PATCH v2 03/14] PCI/DPC: Rename struct dpc_dev.rp to rp_extensions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

"rp" is ambiguous: it might mean "this DPC device is a Root Port."  But in
fact, it means "this DPC device is a Root Port *and* it supports a set of
DPC Extensions."

Rename "rp" to "rp_extensions" to make this more clear.  No functional
change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 52702f7b911a..0bbdf513067a 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -42,7 +42,7 @@ struct dpc_dev {
 	struct pcie_device	*dev;
 	struct work_struct	work;
 	u16			cap_pos;
-	bool			rp;
+	bool			rp_extensions;
 	u32			rp_pio_status;
 };
 
@@ -126,9 +126,9 @@ static void dpc_work(struct work_struct *work)
 	pci_unlock_rescan_remove();
 
 	dpc_wait_link_inactive(dpc);
-	if (dpc->rp && dpc_wait_rp_inactive(dpc))
+	if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
 		return;
-	if (dpc->rp && dpc->rp_pio_status) {
+	if (dpc->rp_extensions && dpc->rp_pio_status) {
 		pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
 				       dpc->rp_pio_status);
 		dpc->rp_pio_status = 0;
@@ -326,7 +326,7 @@ static int dpc_probe(struct pcie_device *dev)
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 
-	dpc->rp = (cap & PCI_EXP_DPC_CAP_RP_EXT);
+	dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
 
 	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);

[PATCH v2 04/14] PCI/DPC: Read RP PIO Log Size once at probe

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The RP PIO Log Size is a read-only field in the DPC Capability, so it is
constant and known at probe-time, but previously we read it every time we
processed an RP PIO error.

Read it once in dpc_probe() (if the RP Extensions for DPC are supported)
and remember the size in struct dpc_dev.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 0bbdf513067a..17f60bd4ea5f 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -34,7 +34,6 @@ struct dpc_rp_pio_regs {
 	struct rp_pio_header_log_regs header_log;
 	u32 impspec_log;
 	u32 tlp_prefix_log[4];
-	u32 log_size;
 	u16 first_error;
 };
 
@@ -44,6 +43,7 @@ struct dpc_dev {
 	u16			cap_pos;
 	bool			rp_extensions;
 	u32			rp_pio_status;
+	u8			rp_log_size;
 };
 
 static const char * const rp_pio_error_string[] = {
@@ -173,11 +173,12 @@ static void dpc_rp_pio_print_error(struct dpc_dev *dpc,
 	}
 
 	dpc_rp_pio_print_tlp_header(dev, &rp_pio->header_log);
-	if (rp_pio->log_size == 4)
+	if (dpc->rp_log_size == 4)
 		return;
+
 	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", rp_pio->impspec_log);
 
-	for (i = 0; i < rp_pio->log_size - 5; i++)
+	for (i = 0; i < dpc->rp_log_size - 5; i++)
 		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i,
 			rp_pio->tlp_prefix_log[i]);
 }
@@ -186,7 +187,6 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 				struct dpc_rp_pio_regs *rp_pio)
 {
 	struct pci_dev *pdev = dpc->dev->port;
-	struct device *dev = &dpc->dev->device;
 	int i;
 	u16 cap = dpc->cap_pos, status;
 
@@ -206,13 +206,8 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
 	rp_pio->first_error = (status & 0x1f00) >> 8;
 
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
-	rp_pio->log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
-	if (rp_pio->log_size < 4 || rp_pio->log_size > 9) {
-		dev_err(dev, "RP PIO log size %u is invalid\n",
-			rp_pio->log_size);
+	if (dpc->rp_log_size < 4)
 		return;
-	}
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
 			      &rp_pio->header_log.dw0);
@@ -222,12 +217,12 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 			      &rp_pio->header_log.dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
 			      &rp_pio->header_log.dw3);
-	if (rp_pio->log_size == 4)
+	if (dpc->rp_log_size == 4)
 		return;
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
 			      &rp_pio->impspec_log);
-	for (i = 0; i < rp_pio->log_size - 5; i++)
+	for (i = 0; i < dpc->rp_log_size - 5; i++)
 		pci_read_config_dword(pdev,
 			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
 			&rp_pio->tlp_prefix_log[i]);
@@ -327,6 +322,14 @@ static int dpc_probe(struct pcie_device *dev)
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 
 	dpc->rp_extensions = (cap & PCI_EXP_DPC_CAP_RP_EXT);
+	if (dpc->rp_extensions) {
+		dpc->rp_log_size = (cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+		if (dpc->rp_log_size < 4 || dpc->rp_log_size > 9) {
+			dev_err(device, "RP PIO log size %u is invalid\n",
+				dpc->rp_log_size);
+			dpc->rp_log_size = 0;
+		}
+	}
 
 	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_NONFATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
@@ -334,7 +337,7 @@ static int dpc_probe(struct pcie_device *dev)
 	dev_info(device, "DPC error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
 		cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
 		FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
-		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), (cap >> 8) & 0xf,
+		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
 		FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
 	return status;
 }

[PATCH v2 05/14] PCI/DPC: Process RP PIO details only if RP PIO extensions supported

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The RP PIO registers (status, mask, severity, etc) are only implemented if
the "RP Extensions for DPC" bit is set in the DPC Capabilities register.

Previously we called dpc_process_rp_pio_error(), which reads and decodes
those RP PIO registers, whenever the DPC Status register indicated an "RP
PIO error" (Trigger Reason == 3 and Trigger Reason Extension == 0).

It does seem reasonable to assume that DPC Status would only indicate an RP
PIO error if the RP extensions are supported, but PCIe r4.0, sec 7.9.15.4,
is actually not explicit about that: it does not say "Trigger Reason
Extension == 0 is valid only for Root Ports that support RP Extensions for
DPC."

Check whether the RP Extensions for DPC are supported before trying to read
the RP PIO registers.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 17f60bd4ea5f..a6b8d1496322 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -281,7 +281,7 @@ static irqreturn_t dpc_irq(int irq, void *context)
 		 (ext_reason == 1) ? "software trigger" :
 				     "reserved error");
 	/* show RP PIO error detail information */
-	if (reason == 3 && ext_reason == 0)
+	if (dpc->rp_extensions && reason == 3 && ext_reason == 0)
 		dpc_process_rp_pio_error(dpc);
 
 	schedule_work(&dpc->work);

[PATCH v2 06/14] PCI/DPC: Squash dpc_rp_pio_print_tlp_header() into dpc_rp_pio_print_error()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Separating dpc_rp_pio_print_tlp_header() doesn't really provide any useful
abstraction, so squash it into its caller, dpc_rp_pio_print_error().  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pcie-dpc.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index a6b8d1496322..16019d1ecc2c 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -142,13 +142,6 @@ static void dpc_work(struct work_struct *work)
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
-static void dpc_rp_pio_print_tlp_header(struct device *dev,
-					struct rp_pio_header_log_regs *t)
-{
-	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
-		t->dw0, t->dw1, t->dw2, t->dw3);
-}
-
 static void dpc_rp_pio_print_error(struct dpc_dev *dpc,
 				   struct dpc_rp_pio_regs *rp_pio)
 {
@@ -172,7 +165,9 @@ static void dpc_rp_pio_print_error(struct dpc_dev *dpc,
 			rp_pio->first_error == i ? " (First)" : "");
 	}
 
-	dpc_rp_pio_print_tlp_header(dev, &rp_pio->header_log);
+	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
+		rp_pio->header_log.dw0, rp_pio->header_log.dw1,
+		rp_pio->header_log.dw2, rp_pio->header_log.dw3);
 	if (dpc->rp_log_size == 4)
 		return;
 

[PATCH v2 07/14] PCI/DPC: Rename local "status" to "dpc_status"

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

In dpc_rp_pio_get_info() rename the local "status" variable to
"dpc_status".  This is to make room for another variable named "status" in
a subsequent patch.  No functional change intended.

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

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 16019d1ecc2c..7e02d335b5ef 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -183,7 +183,7 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 {
 	struct pci_dev *pdev = dpc->dev->port;
 	int i;
-	u16 cap = dpc->cap_pos, status;
+	u16 cap = dpc->cap_pos, dpc_status;
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
 			      &rp_pio->status);
@@ -198,8 +198,8 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 			      &rp_pio->exception);
 
 	/* Get First Error Pointer */
-	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
-	rp_pio->first_error = (status & 0x1f00) >> 8;
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
+	rp_pio->first_error = (dpc_status & 0x1f00) >> 8;
 
 	if (dpc->rp_log_size < 4)
 		return;

[PATCH v2 08/14] PCI/DPC: Make RP PIO log size check more generic

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

In dpc_probe(), we set dpc->rp_log_size to zero if we think the hardware
reports an invalid size.  In this case, we could have dpc->rp_extensions
set but dpc->rp_log_size == 0, and we should print the basic RP PIO
registers but not the variable-size portion.  We already checked for
dpc->rp_log_size < 4 above, so this patch is just for consistency of style.
No functional change intended.

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

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 7e02d335b5ef..fede1d4bc7b9 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -212,11 +212,12 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 			      &rp_pio->header_log.dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
 			      &rp_pio->header_log.dw3);
-	if (dpc->rp_log_size == 4)
-		return;
 
+	if (dpc->rp_log_size < 5)
+		return;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
 			      &rp_pio->impspec_log);
+
 	for (i = 0; i < dpc->rp_log_size - 5; i++)
 		pci_read_config_dword(pdev,
 			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,

[PATCH v2 09/14] PCI/DPC: Squash dpc_rp_pio_print_error() into dpc_rp_pio_get_info()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Separating dpc_rp_pio_print_error() doesn't really provide any useful
abstraction, so squash it into its caller, dpc_rp_pio_get_info().  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pcie-dpc.c |   60 +++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index fede1d4bc7b9..fb7ddbe96811 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -142,53 +142,21 @@ static void dpc_work(struct work_struct *work)
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
-static void dpc_rp_pio_print_error(struct dpc_dev *dpc,
-				   struct dpc_rp_pio_regs *rp_pio)
-{
-	struct device *dev = &dpc->dev->device;
-	int i;
-	u32 status;
-
-	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
-		rp_pio->status, rp_pio->mask);
-
-	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
-		rp_pio->severity, rp_pio->syserror, rp_pio->exception);
-
-	status = (rp_pio->status & ~rp_pio->mask);
-
-	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
-		if (!(status & (1 << i)))
-			continue;
-
-		dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
-			rp_pio->first_error == i ? " (First)" : "");
-	}
-
-	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
-		rp_pio->header_log.dw0, rp_pio->header_log.dw1,
-		rp_pio->header_log.dw2, rp_pio->header_log.dw3);
-	if (dpc->rp_log_size == 4)
-		return;
-
-	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", rp_pio->impspec_log);
-
-	for (i = 0; i < dpc->rp_log_size - 5; i++)
-		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i,
-			rp_pio->tlp_prefix_log[i]);
-}
-
 static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 				struct dpc_rp_pio_regs *rp_pio)
 {
+	struct device *dev = &dpc->dev->device;
 	struct pci_dev *pdev = dpc->dev->port;
 	int i;
 	u16 cap = dpc->cap_pos, dpc_status;
+	u32 status;
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
 			      &rp_pio->status);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK,
 			      &rp_pio->mask);
+	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
+		rp_pio->status, rp_pio->mask);
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY,
 			      &rp_pio->severity);
@@ -196,14 +164,22 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 			      &rp_pio->syserror);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION,
 			      &rp_pio->exception);
+	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
+		rp_pio->severity, rp_pio->syserror, rp_pio->exception);
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
 	rp_pio->first_error = (dpc_status & 0x1f00) >> 8;
 
+	status = (rp_pio->status & ~rp_pio->mask);
+	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
+		if (status & (1 << i))
+			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
+				rp_pio->first_error == i ? " (First)" : "");
+	}
+
 	if (dpc->rp_log_size < 4)
 		return;
-
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
 			      &rp_pio->header_log.dw0);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
@@ -212,16 +188,23 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 			      &rp_pio->header_log.dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
 			      &rp_pio->header_log.dw3);
+	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
+		rp_pio->header_log.dw0, rp_pio->header_log.dw1,
+		rp_pio->header_log.dw2, rp_pio->header_log.dw3);
 
 	if (dpc->rp_log_size < 5)
 		return;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
 			      &rp_pio->impspec_log);
+	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", rp_pio->impspec_log);
 
-	for (i = 0; i < dpc->rp_log_size - 5; i++)
+	for (i = 0; i < dpc->rp_log_size - 5; i++) {
 		pci_read_config_dword(pdev,
 			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
 			&rp_pio->tlp_prefix_log[i]);
+		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i,
+			rp_pio->tlp_prefix_log[i]);
+	}
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
@@ -229,7 +212,6 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 	struct dpc_rp_pio_regs rp_pio_regs;
 
 	dpc_rp_pio_get_info(dpc, &rp_pio_regs);
-	dpc_rp_pio_print_error(dpc, &rp_pio_regs);
 
 	dpc->rp_pio_status = rp_pio_regs.status;
 }

[PATCH v2 10/14] PCI/DPC: Push dpc->rp_pio_status assignment into dpc_rp_pio_get_info()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Move the dpc->rp_pio_status assignment into dpc_rp_pio_get_info() since
that's where we read rp_pio->status anway.  No functional change intended.

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

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index fb7ddbe96811..7b96169a808c 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -158,6 +158,8 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
 	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
 		rp_pio->status, rp_pio->mask);
 
+	dpc->rp_pio_status = rp_pio->status;
+
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY,
 			      &rp_pio->severity);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR,
@@ -213,7 +215,6 @@ static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 
 	dpc_rp_pio_get_info(dpc, &rp_pio_regs);
 
-	dpc->rp_pio_status = rp_pio_regs.status;
 }
 
 static irqreturn_t dpc_irq(int irq, void *context)

[PATCH v2 11/14] PCI/DPC: Remove unnecessary RP PIO register structs

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

We read and immediately print the RP PIO log registers.  We don't save
them, so there's no need to define structs for them.  Remove the structs
and read the registers into local variables instead.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pcie-dpc.c |   87 +++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 57 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 7b96169a808c..daad9c4ae607 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -17,26 +17,6 @@
 #include "../pci.h"
 #include "aer/aerdrv.h"
 
-struct rp_pio_header_log_regs {
-	u32 dw0;
-	u32 dw1;
-	u32 dw2;
-	u32 dw3;
-};
-
-struct dpc_rp_pio_regs {
-	u32 status;
-	u32 mask;
-	u32 severity;
-	u32 syserror;
-	u32 exception;
-
-	struct rp_pio_header_log_regs header_log;
-	u32 impspec_log;
-	u32 tlp_prefix_log[4];
-	u16 first_error;
-};
-
 struct dpc_dev {
 	struct pcie_device	*dev;
 	struct work_struct	work;
@@ -142,78 +122,71 @@ static void dpc_work(struct work_struct *work)
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
-static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
-				struct dpc_rp_pio_regs *rp_pio)
+static void dpc_rp_pio_get_info(struct dpc_dev *dpc)
 {
 	struct device *dev = &dpc->dev->device;
 	struct pci_dev *pdev = dpc->dev->port;
 	int i;
-	u16 cap = dpc->cap_pos, dpc_status;
-	u32 status;
-
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
-			      &rp_pio->status);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK,
-			      &rp_pio->mask);
+	u16 cap = dpc->cap_pos;
+	u32 status, mask;
+	u32 sev, syserr, exc;
+	u16 dpc_status, first_error;
+	u32 dw0, dw1, dw2, dw3;
+	u32 log;
+	u32 prefix;
+
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, &status);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK, &mask);
 	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
-		rp_pio->status, rp_pio->mask);
+		status, mask);
 
-	dpc->rp_pio_status = rp_pio->status;
+	dpc->rp_pio_status = status;
 
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY,
-			      &rp_pio->severity);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR,
-			      &rp_pio->syserror);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION,
-			      &rp_pio->exception);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY, &sev);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR, &syserr);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION, &exc);
 	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
-		rp_pio->severity, rp_pio->syserror, rp_pio->exception);
+		sev, syserr, exc);
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
-	rp_pio->first_error = (dpc_status & 0x1f00) >> 8;
+	first_error = (dpc_status & 0x1f00) >> 8;
 
-	status = (rp_pio->status & ~rp_pio->mask);
+	status &= ~mask;
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if (status & (1 << i))
 			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
-				rp_pio->first_error == i ? " (First)" : "");
+				first_error == i ? " (First)" : "");
 	}
 
 	if (dpc->rp_log_size < 4)
 		return;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
-			      &rp_pio->header_log.dw0);
+			      &dw0);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
-			      &rp_pio->header_log.dw1);
+			      &dw1);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 8,
-			      &rp_pio->header_log.dw2);
+			      &dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
-			      &rp_pio->header_log.dw3);
+			      &dw3);
 	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
-		rp_pio->header_log.dw0, rp_pio->header_log.dw1,
-		rp_pio->header_log.dw2, rp_pio->header_log.dw3);
+		dw0, dw1, dw2, dw3);
 
 	if (dpc->rp_log_size < 5)
 		return;
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
-			      &rp_pio->impspec_log);
-	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", rp_pio->impspec_log);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
+	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
 
 	for (i = 0; i < dpc->rp_log_size - 5; i++) {
 		pci_read_config_dword(pdev,
-			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
-			&rp_pio->tlp_prefix_log[i]);
-		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i,
-			rp_pio->tlp_prefix_log[i]);
+			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
+		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
 	}
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
-	struct dpc_rp_pio_regs rp_pio_regs;
-
-	dpc_rp_pio_get_info(dpc, &rp_pio_regs);
+	dpc_rp_pio_get_info(dpc);
 
 }
 

Re: [PATCH v2 11/14] PCI/DPC: Remove unnecessary RP PIO register structs

[Keith Busch]

On Tue, Jan 30, 2018 at 12:26:48PM -0600, Bjorn Helgaas wrote:
> +	u16 cap = dpc->cap_pos;
> +	u32 status, mask;
> +	u32 sev, syserr, exc;
> +	u16 dpc_status, first_error;
> +	u32 dw0, dw1, dw2, dw3;
> +	u32 log;
> +	u32 prefix;

Minor nit, could you squash the declarations of the same type?

Re: [PATCH v2 11/14] PCI/DPC: Remove unnecessary RP PIO register structs

[Bjorn Helgaas]

On Tue, Jan 30, 2018 at 11:57:58AM -0700, Keith Busch wrote:
> On Tue, Jan 30, 2018 at 12:26:48PM -0600, Bjorn Helgaas wrote:
> > +	u16 cap = dpc->cap_pos;
> > +	u32 status, mask;
> > +	u32 sev, syserr, exc;
> > +	u16 dpc_status, first_error;
> > +	u32 dw0, dw1, dw2, dw3;
> > +	u32 log;
> > +	u32 prefix;
> 
> Minor nit, could you squash the declarations of the same type?

I did it that way so it matches the order of the chunks of code where
they're used.  But I guess we don't really do that elsewhere in the
file, so I did.  Here's the result:


commit ea512cad7f7e293eca002039517c4469383c5bcf
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Tue Jan 30 12:12:48 2018 -0600

    PCI/DPC: Remove unnecessary RP PIO register structs
    
    We read and immediately print the RP PIO log registers.  We don't save
    them, so there's no need to define structs for them.  Remove the structs
    and read the registers into local variables instead.  No functional change
    intended.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 7b96169a808c..468440e8fb6f 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -17,26 +17,6 @@
 #include "../pci.h"
 #include "aer/aerdrv.h"
 
-struct rp_pio_header_log_regs {
-	u32 dw0;
-	u32 dw1;
-	u32 dw2;
-	u32 dw3;
-};
-
-struct dpc_rp_pio_regs {
-	u32 status;
-	u32 mask;
-	u32 severity;
-	u32 syserror;
-	u32 exception;
-
-	struct rp_pio_header_log_regs header_log;
-	u32 impspec_log;
-	u32 tlp_prefix_log[4];
-	u16 first_error;
-};
-
 struct dpc_dev {
 	struct pcie_device	*dev;
 	struct work_struct	work;
@@ -142,78 +122,66 @@ static void dpc_work(struct work_struct *work)
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
-static void dpc_rp_pio_get_info(struct dpc_dev *dpc,
-				struct dpc_rp_pio_regs *rp_pio)
+static void dpc_rp_pio_get_info(struct dpc_dev *dpc)
 {
 	struct device *dev = &dpc->dev->device;
 	struct pci_dev *pdev = dpc->dev->port;
+	u16 cap = dpc->cap_pos, dpc_status, first_error;
+	u32 status, mask, sev, syserr, exc, dw0, dw1, dw2, dw3, log, prefix;
 	int i;
-	u16 cap = dpc->cap_pos, dpc_status;
-	u32 status;
 
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
-			      &rp_pio->status);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK,
-			      &rp_pio->mask);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, &status);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_MASK, &mask);
 	dev_err(dev, "rp_pio_status: %#010x, rp_pio_mask: %#010x\n",
-		rp_pio->status, rp_pio->mask);
+		status, mask);
 
-	dpc->rp_pio_status = rp_pio->status;
+	dpc->rp_pio_status = status;
 
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY,
-			      &rp_pio->severity);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR,
-			      &rp_pio->syserror);
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION,
-			      &rp_pio->exception);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SEVERITY, &sev);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_SYSERROR, &syserr);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_EXCEPTION, &exc);
 	dev_err(dev, "RP PIO severity=%#010x, syserror=%#010x, exception=%#010x\n",
-		rp_pio->severity, rp_pio->syserror, rp_pio->exception);
+		sev, syserr, exc);
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
-	rp_pio->first_error = (dpc_status & 0x1f00) >> 8;
+	first_error = (dpc_status & 0x1f00) >> 8;
 
-	status = (rp_pio->status & ~rp_pio->mask);
+	status &= ~mask;
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if (status & (1 << i))
 			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
-				rp_pio->first_error == i ? " (First)" : "");
+				first_error == i ? " (First)" : "");
 	}
 
 	if (dpc->rp_log_size < 4)
 		return;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
-			      &rp_pio->header_log.dw0);
+			      &dw0);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 4,
-			      &rp_pio->header_log.dw1);
+			      &dw1);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 8,
-			      &rp_pio->header_log.dw2);
+			      &dw2);
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG + 12,
-			      &rp_pio->header_log.dw3);
+			      &dw3);
 	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
-		rp_pio->header_log.dw0, rp_pio->header_log.dw1,
-		rp_pio->header_log.dw2, rp_pio->header_log.dw3);
+		dw0, dw1, dw2, dw3);
 
 	if (dpc->rp_log_size < 5)
 		return;
-	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG,
-			      &rp_pio->impspec_log);
-	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", rp_pio->impspec_log);
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
+	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
 
 	for (i = 0; i < dpc->rp_log_size - 5; i++) {
 		pci_read_config_dword(pdev,
-			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
-			&rp_pio->tlp_prefix_log[i]);
-		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i,
-			rp_pio->tlp_prefix_log[i]);
+			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
+		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
 	}
 }
 
 static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
-	struct dpc_rp_pio_regs rp_pio_regs;
-
-	dpc_rp_pio_get_info(dpc, &rp_pio_regs);
+	dpc_rp_pio_get_info(dpc);
 
 }
 

Re: [PATCH v2 11/14] PCI/DPC: Remove unnecessary RP PIO register structs

[Keith Busch]

On Tue, Jan 30, 2018 at 01:11:00PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 30, 2018 at 11:57:58AM -0700, Keith Busch wrote:
> > On Tue, Jan 30, 2018 at 12:26:48PM -0600, Bjorn Helgaas wrote:
> > > +	u16 cap = dpc->cap_pos;
> > > +	u32 status, mask;
> > > +	u32 sev, syserr, exc;
> > > +	u16 dpc_status, first_error;
> > > +	u32 dw0, dw1, dw2, dw3;
> > > +	u32 log;
> > > +	u32 prefix;
> > 
> > Minor nit, could you squash the declarations of the same type?
> 
> I did it that way so it matches the order of the chunks of code where
> they're used.  But I guess we don't really do that elsewhere in the
> file, so I did.  Here's the result:

Thanks! That's all I had on this series. Looks good to me!

Reviewed-by: Keith Busch <keith.busch@intel.com>

[PATCH v2 12/14] PCI/DPC: Squash dpc_rp_pio_get_info() into dpc_process_rp_pio_error()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

dpc_process_rp_pio_error() only calls dpc_rp_pio_get_info(), so squash them
together.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pcie-dpc.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index daad9c4ae607..76a50d03e5db 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -122,17 +122,17 @@ static void dpc_work(struct work_struct *work)
 			      ctl | PCI_EXP_DPC_CTL_INT_EN);
 }
 
-static void dpc_rp_pio_get_info(struct dpc_dev *dpc)
+static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
 	struct device *dev = &dpc->dev->device;
 	struct pci_dev *pdev = dpc->dev->port;
-	int i;
 	u16 cap = dpc->cap_pos;
 	u32 status, mask;
 	u32 sev, syserr, exc;
 	u16 dpc_status, first_error;
 	u32 dw0, dw1, dw2, dw3;
 	u32 log;
+	int i;
 	u32 prefix;
 
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, &status);
@@ -184,12 +184,6 @@ static void dpc_rp_pio_get_info(struct dpc_dev *dpc)
 	}
 }
 
-static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
-{
-	dpc_rp_pio_get_info(dpc);
-
-}
-
 static irqreturn_t dpc_irq(int irq, void *context)
 {
 	struct dpc_dev *dpc = (struct dpc_dev *)context;

[PATCH v2 13/14] PCI/DPC: Add and use DPC Status register field definitions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add definitions for DPC Status register fields and use them in the code.
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/pcie-dpc.c   |    4 ++--
 include/uapi/linux/pci_regs.h |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 76a50d03e5db..5b4c65bc9267 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -216,8 +216,8 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	dev_info(dev, "DPC containment event, status:%#06x source:%#06x\n",
 		status, source);
 
-	reason = (status >> 1) & 0x3;
-	ext_reason = (status >> 5) & 0x3;
+	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
+	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
 
 	dev_warn(dev, "DPC %s detected, remove downstream devices\n",
 		 (reason == 0) ? "unmasked uncorrectable error" :
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 70c2b2ade048..970a0dc535c4 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -979,8 +979,10 @@
 
 #define PCI_EXP_DPC_STATUS		8	/* DPC Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER	0x01	/* Trigger Status */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	0x06	/* Trigger Reason */
 #define  PCI_EXP_DPC_STATUS_INTERRUPT	0x08	/* Interrupt Status */
 #define  PCI_EXP_DPC_RP_BUSY		0x10	/* Root Port Busy */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x60 /* Trig Reason Extension */
 
 #define PCI_EXP_DPC_SOURCE_ID		10	/* DPC Source Identifier */
 

[PATCH v2 14/14] PCI/DPC: Reformat DPC register definitions

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Reformat DPC register definitions to follow the convention that register
field masks indicate the register width, e.g., a field of a 16-bit register
uses a mask of 4 hex digits, with leading zeros included as needed.
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
---
 include/uapi/linux/pci_regs.h |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 970a0dc535c4..66d71461d2f0 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -966,28 +966,28 @@
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			4	/* DPC Capability */
-#define PCI_EXP_DPC_IRQ			0x1f	/* DPC Interrupt Message Number */
-#define  PCI_EXP_DPC_CAP_RP_EXT		0x20	/* Root Port Extensions for DPC */
-#define  PCI_EXP_DPC_CAP_POISONED_TLP	0x40	/* Poisoned TLP Egress Blocking Supported */
-#define  PCI_EXP_DPC_CAP_SW_TRIGGER	0x80	/* Software Triggering Supported */
-#define  PCI_EXP_DPC_RP_PIO_LOG_SIZE	0xF00	/* RP PIO log size */
+#define PCI_EXP_DPC_IRQ			0x001F	/* Interrupt Message Number */
+#define  PCI_EXP_DPC_CAP_RP_EXT		0x0020	/* Root Port Extensions */
+#define  PCI_EXP_DPC_CAP_POISONED_TLP	0x0040	/* Poisoned TLP Egress Blocking Supported */
+#define  PCI_EXP_DPC_CAP_SW_TRIGGER	0x0080	/* Software Triggering Supported */
+#define  PCI_EXP_DPC_RP_PIO_LOG_SIZE	0x0F00	/* RP PIO Log Size */
 #define  PCI_EXP_DPC_CAP_DL_ACTIVE	0x1000	/* ERR_COR signal on DL_Active supported */
 
 #define PCI_EXP_DPC_CTL			6	/* DPC control */
-#define  PCI_EXP_DPC_CTL_EN_NONFATAL 	0x02	/* Enable trigger on ERR_NONFATAL message */
-#define  PCI_EXP_DPC_CTL_INT_EN 	0x08	/* DPC Interrupt Enable */
+#define  PCI_EXP_DPC_CTL_EN_NONFATAL 	0x0002	/* Enable trigger on ERR_NONFATAL message */
+#define  PCI_EXP_DPC_CTL_INT_EN 	0x0008	/* DPC Interrupt Enable */
 
 #define PCI_EXP_DPC_STATUS		8	/* DPC Status */
-#define  PCI_EXP_DPC_STATUS_TRIGGER	0x01	/* Trigger Status */
-#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	0x06	/* Trigger Reason */
-#define  PCI_EXP_DPC_STATUS_INTERRUPT	0x08	/* Interrupt Status */
-#define  PCI_EXP_DPC_RP_BUSY		0x10	/* Root Port Busy */
-#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x60 /* Trig Reason Extension */
+#define  PCI_EXP_DPC_STATUS_TRIGGER	    0x0001 /* Trigger Status */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	    0x0006 /* Trigger Reason */
+#define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
+#define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
 
 #define PCI_EXP_DPC_SOURCE_ID		10	/* DPC Source Identifier */
 
 #define PCI_EXP_DPC_RP_PIO_STATUS	 0x0C	/* RP PIO Status */
-#define PCI_EXP_DPC_RP_PIO_MASK		 0x10	/* RP PIO MASK */
+#define PCI_EXP_DPC_RP_PIO_MASK		 0x10	/* RP PIO Mask */
 #define PCI_EXP_DPC_RP_PIO_SEVERITY	 0x14	/* RP PIO Severity */
 #define PCI_EXP_DPC_RP_PIO_SYSERROR	 0x18	/* RP PIO SysError */
 #define PCI_EXP_DPC_RP_PIO_EXCEPTION	 0x1C	/* RP PIO Exception */

Re: [PATCH v2 00/14] PCI/DPC: Simplify RP PIO logging

[Sinan Kaya]

On 1/30/2018 1:25 PM, Bjorn Helgaas wrote:
> Changes from v1 to v2:
>   - Split up the "Consolidate RP PIO get/print functions" patch to
>     make it more reviewable.  The resulting git diff is empty except
>     for a trival local variable declaration.

Thanks v2 looks good to me too.

Reviewed-by: Sinan Kaya <okaya@codeaurora.org>

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.