diff mbox

[6/8] PCI/DPC: Consolidate RP PIO get/print functions

Message ID 20180126225622.58614.24127.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas Jan. 26, 2018, 10:56 p.m. UTC
From: Bjorn Helgaas <bhelgaas@google.com>

Previously we defined a structure for RP PIO log information, allocated it
on the stack, called one function to fill it in from DPC registers, and
called another to print it out.

Simplify this by dropping the structure and printing the error information
as soon as we read it from the DPC capability.  This way we don't need a
structure, there's one function instead of four, and everything we do with
the register information is in one place instead of being split between
functions.

No functional change intended.

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

Comments

Sinan Kaya Jan. 30, 2018, 12:42 a.m. UTC | #1
On 2018-01-26 17:56, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Previously we defined a structure for RP PIO log information, allocated 
> it
> on the stack, called one function to fill it in from DPC registers, and
> called another to print it out.
> 
> Simplify this by dropping the structure and printing the error 
> information
> as soon as we read it from the DPC capability.  This way we don't need 
> a
> structure, there's one function instead of four, and everything we do 
> with
> the register information is in one place instead of being split between
> functions.
> 
> No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/pcie-dpc.c |  132 
> +++++++++++++------------------------------
>  1 file changed, 39 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
> index a6b8d1496322..06d3af112580 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,100 +122,66 @@ 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)
> +static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
>  {
>  	struct device *dev = &dpc->dev->device;
> +	struct pci_dev *pdev = dpc->dev->port;
> +	u16 cap = dpc->cap_pos;
> +	u32 status, mask;
> +	u32 sev, syserr, exc;
> +	u16 dpc_status, first_error;
>  	int i;
> -	u32 status;
> +	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 = status;
> +
> +	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);
> 
> -	status = (rp_pio->status & ~rp_pio->mask);
> +	/* Get First Error Pointer */
> +	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> +	first_error = (dpc_status & 0x1f00) >> 8;

This didn't look like a simple change here.

> 
> +	status &= ~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)" : "");
> +		if (status & (1 << i))
> +			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
> +				first_error == i ? " (First)" : "");
>  	}
> 
> -	dpc_rp_pio_print_tlp_header(dev, &rp_pio->header_log);
> -	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 pci_dev *pdev = dpc->dev->port;
> -	int i;
> -	u16 cap = dpc->cap_pos, 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_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);
> -
> -	/* Get First Error Pointer */
> -	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
> -	rp_pio->first_error = (status & 0x1f00) >> 8;
> -
>  	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);
> -	if (dpc->rp_log_size == 4)
> +			      &dw3);
> +	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
> +		dw0, dw1, dw2, dw3);
> +
> +	if (dpc->rp_log_size < 5)
>  		return;
> +	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, 
> &log);
> +	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
> 
> -	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++)
> +	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]);
> -}
> -
> -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;
> +			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
> +		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
> +	}
>  }
> 
>  static irqreturn_t dpc_irq(int irq, void *context)
Bjorn Helgaas Jan. 30, 2018, 6:11 p.m. UTC | #2
On Mon, Jan 29, 2018 at 07:42:54PM -0500, okaya@codeaurora.org wrote:
> On 2018-01-26 17:56, Bjorn Helgaas wrote:
> >From: Bjorn Helgaas <bhelgaas@google.com>
> >
> >Previously we defined a structure for RP PIO log information,
> >allocated it
> >on the stack, called one function to fill it in from DPC registers, and
> >called another to print it out.
> >
> >Simplify this by dropping the structure and printing the error
> >information
> >as soon as we read it from the DPC capability.  This way we don't
> >need a
> >structure, there's one function instead of four, and everything we
> >do with
> >the register information is in one place instead of being split between
> >functions.
> >
> >No functional change intended.
> >
> >Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >---
> > drivers/pci/pcie/pcie-dpc.c |  132
> >+++++++++++++------------------------------
> > 1 file changed, 39 insertions(+), 93 deletions(-)
> >
> >diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
> >index a6b8d1496322..06d3af112580 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,100 +122,66 @@ 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)
> >+static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
> > {
> > 	struct device *dev = &dpc->dev->device;
> >+	struct pci_dev *pdev = dpc->dev->port;
> >+	u16 cap = dpc->cap_pos;
> >+	u32 status, mask;
> >+	u32 sev, syserr, exc;
> >+	u16 dpc_status, first_error;
> > 	int i;
> >-	u32 status;
> >+	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 = status;
> >+
> >+	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);
> >
> >-	status = (rp_pio->status & ~rp_pio->mask);
> >+	/* Get First Error Pointer */
> >+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> >+	first_error = (dpc_status & 0x1f00) >> 8;
> 
> This didn't look like a simple change here.

The diff is ugly, for sure.   I'll split it up and you can see what
you think.
diff mbox

Patch

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index a6b8d1496322..06d3af112580 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,100 +122,66 @@  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)
+static void dpc_process_rp_pio_error(struct dpc_dev *dpc)
 {
 	struct device *dev = &dpc->dev->device;
+	struct pci_dev *pdev = dpc->dev->port;
+	u16 cap = dpc->cap_pos;
+	u32 status, mask;
+	u32 sev, syserr, exc;
+	u16 dpc_status, first_error;
 	int i;
-	u32 status;
+	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 = status;
+
+	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);
 
-	status = (rp_pio->status & ~rp_pio->mask);
+	/* Get First Error Pointer */
+	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
+	first_error = (dpc_status & 0x1f00) >> 8;
 
+	status &= ~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)" : "");
+		if (status & (1 << i))
+			dev_err(dev, "[%2d] %s%s\n", i, rp_pio_error_string[i],
+				first_error == i ? " (First)" : "");
 	}
 
-	dpc_rp_pio_print_tlp_header(dev, &rp_pio->header_log);
-	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 pci_dev *pdev = dpc->dev->port;
-	int i;
-	u16 cap = dpc->cap_pos, 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_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);
-
-	/* Get First Error Pointer */
-	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &status);
-	rp_pio->first_error = (status & 0x1f00) >> 8;
-
 	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);
-	if (dpc->rp_log_size == 4)
+			      &dw3);
+	dev_err(dev, "TLP Header: %#010x %#010x %#010x %#010x\n",
+		dw0, dw1, dw2, dw3);
+
+	if (dpc->rp_log_size < 5)
 		return;
+	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
+	dev_err(dev, "RP PIO ImpSpec Log %#010x\n", log);
 
-	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++)
+	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]);
-}
-
-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;
+			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG, &prefix);
+		dev_err(dev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
+	}
 }
 
 static irqreturn_t dpc_irq(int irq, void *context)