[PATCH 0/2] PCI/DPC: Improve register field accessing

[PATCH 0/2] PCI/DPC: Improve register field accessing

[Ilpo Järvinen]

This an alternative approach to the patch in:
  https://lore.kernel.org/linux-pci/20231010204436.1000644-7-helgaas@kernel.org/

It adds names to all the reason literals too (which makes it incompatible
with FIELD_GET() for the reason and ext_reason). When the reasons are
named instead of literals, it's very easy to understand the code just by
reading it (no need to lookup the meaning of those numbers from spec or
otherwise).

Also 0xfff4 the other patch missed is converted here.

Just let me know if I should, for example, base the additional changes
on top of that other change.

Ilpo Järvinen (2):
  PCI: Add PCI_EXP_DPC_* field details
  PCI/DPC: Use defines with register fields

 drivers/pci/pcie/dpc.c        | 39 +++++++++++++++++++++--------------
 include/uapi/linux/pci_regs.h |  7 +++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH 1/2] PCI: Add PCI_EXP_DPC_* field details

[Ilpo Järvinen]

Add PCI_EXP_DPC_RP_PIO_FEP and PCI_EXP_DPC_STATUS_TRIGGER_RSN_*
to be able to replace literals with them.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 include/uapi/linux/pci_regs.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5f558d96493..ce4512147ee7 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1042,9 +1042,16 @@
 #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER	    0x0001 /* Trigger Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	    0x0006 /* Trigger Reason */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR  0x0000 /* DPC due to unmasked uncorrectable error */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE    0x0002 /* DPC due to receiving ERR_NONFATAL */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE     0x0004 /* DPC due to receiving ERR_FATAL */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT 0x0006 /* Reason in Trig Reason Extension field */
 #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_STATUS_TRIGGER_RSN_RP_PIO		0x0000	/* DPC due to RP PIO error */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER	0x0020	/* DPC due to DPC SW Trigger bit */
+#define  PCI_EXP_DPC_RP_PIO_FEP		    0x1f00 /* Root Port PIO First Error Pointer */
 
 #define PCI_EXP_DPC_SOURCE_ID		 0x0A	/* DPC Source Identifier */
 
-- 
2.30.2

[PATCH 2/2] PCI/DPC: Use defines with register fields

[Ilpo Järvinen]

Use defines instead of literals and replace custom masking and shifts
with FIELD_GET() where it makes sense.

While at it, group PCI_EXP_DPC_CTL RMW and pci_info() lines together
as it makes the code intent slightly easier to follow.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/dpc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..81e06639cb8a 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -9,6 +9,7 @@
 #define dev_fmt(fmt) "DPC: " fmt
 
 #include <linux/aer.h>
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
@@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
-	first_error = (dpc_status & 0x1f00) >> 8;
+	first_error = FIELD_GET(PCI_EXP_DPC_RP_PIO_FEP, dpc_status);
 
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if ((status & ~mask) & (1 << i))
@@ -270,20 +271,27 @@ void dpc_process_error(struct pci_dev *pdev)
 	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
 		 status, source);
 
-	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
-	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
+	reason = status & PCI_EXP_DPC_STATUS_TRIGGER_RSN;
+	ext_reason = status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;
 	pci_warn(pdev, "%s detected\n",
-		 (reason == 0) ? "unmasked uncorrectable error" :
-		 (reason == 1) ? "ERR_NONFATAL" :
-		 (reason == 2) ? "ERR_FATAL" :
-		 (ext_reason == 0) ? "RP PIO error" :
-		 (ext_reason == 1) ? "software trigger" :
-				     "reserved error");
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR) ?
+		 "unmasked uncorrectable error" :
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE) ?
+		 "ERR_NONFATAL" :
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) ?
+		 "ERR_FATAL" :
+		 (ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO) ?
+		 "RP PIO error" :
+		 (ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER) ?
+		 "software trigger" :
+		 "reserved error");
 
 	/* show RP PIO error detail information */
-	if (pdev->dpc_rp_extensions && reason == 3 && ext_reason == 0)
+	if (pdev->dpc_rp_extensions &&
+	    reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT &&
+	    ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)
 		dpc_process_rp_pio_error(pdev);
-	else if (reason == 0 &&
+	else if (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR &&
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
 		aer_print_error(pdev, &info);
@@ -338,7 +346,7 @@ void pci_dpc_init(struct pci_dev *pdev)
 	/* Quirks may set dpc_rp_log_size if device or firmware is buggy */
 	if (!pdev->dpc_rp_log_size) {
 		pdev->dpc_rp_log_size =
-			(cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+			FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
 		if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
 			pci_err(pdev, "RP PIO log size %u is invalid\n",
 				pdev->dpc_rp_log_size);
@@ -368,12 +376,13 @@ static int dpc_probe(struct pcie_device *dev)
 	}
 
 	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
-	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
+	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
+	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
+	ctl &= ~PCI_EXP_DPC_CTL_EN_NONFATAL;
 	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
-	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 
+	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 	pci_info(pdev, "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),
-- 
2.30.2

Re: [PATCH 2/2] PCI/DPC: Use defines with register fields

[Bjorn Helgaas]

On Fri, Oct 13, 2023 at 02:20:04PM +0300, Ilpo Järvinen wrote:
> Use defines instead of literals and replace custom masking and shifts
> with FIELD_GET() where it makes sense.
> ...

>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
> -	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>  
> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> +	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> +	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> +	ctl &= ~PCI_EXP_DPC_CTL_EN_NONFATAL;

This has been a little obtuse from the beginning.

The original clears bits 0, 1, 3, then sets bits 0 and 3.
The new code sets bits 0, 3, then clears bit 1.

These are equivalent, but it's definitely some work to verify it.

I think the point is to enable DPC on ERR_FATAL (but not ERR_NONFATAL)
and to enable DPC interrupts.  What about something like this?

  #define PCI_EXP_DPC_CTL_EN_MASK  0x0003

  ctl &= ~PCI_EXP_DPC_CTL_EN_MASK;
  ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;

>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> -	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>  
> +	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
>  	pci_info(pdev, "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),
> -- 
> 2.30.2
> 

Re: [PATCH 2/2] PCI/DPC: Use defines with register fields

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2143 bytes --]

On Fri, 13 Oct 2023, Bjorn Helgaas wrote:

> On Fri, Oct 13, 2023 at 02:20:04PM +0300, Ilpo Järvinen wrote:
> > Use defines instead of literals and replace custom masking and shifts
> > with FIELD_GET() where it makes sense.
> > ...
> 
> >  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
> > -	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> >  
> > -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> > +	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> > +	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
> > +	ctl &= ~PCI_EXP_DPC_CTL_EN_NONFATAL;
> 
> This has been a little obtuse from the beginning.
> 
> The original clears bits 0, 1, 3, then sets bits 0 and 3.
> The new code sets bits 0, 3, then clears bit 1.
> 
> These are equivalent, but it's definitely some work to verify it.
>
> I think the point is to enable DPC on ERR_FATAL (but not ERR_NONFATAL)
> and to enable DPC interrupts.  What about something like this?
>   #define PCI_EXP_DPC_CTL_EN_MASK  0x0003
> 
>   ctl &= ~PCI_EXP_DPC_CTL_EN_MASK;
>   ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;

Thanks for the suggestion, it looks cleaner, yes. I realized the bit 
changes weren't as obvious as they could be but I failed to see I could 
add such a combined mask to make the intent dead obvious.

With a small change replacing the 0x0003 literal with the combined defines 
of the actual bits it seems very clear what's going on. I'll go for that. 

I'll place my non-FIELD_GET() changes on DPC top of your 
FIELD_GET/PREP()-only change so each patch can be smaller and more 
focused.

-- 
 i.


> >  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> > -	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
> >  
> > +	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
> >  	pci_info(pdev, "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),
> > -- 
> > 2.30.2
> > 
>