[PATCH v6 0/8] PCI: Consolidate TLP Log reading and printing

[PATCH v6 0/8] PCI: Consolidate TLP Log reading and printing

[Ilpo Järvinen]

This series has the remaining patches of the AER & DPC TLP Log handling
consolidation and now includes a few minor improvements to the earlier
accepted TLP Logging code.

v6:
- Preserve "AER:"/"DPC:" prefix on the printed TLP line
- New patch to add "AER:" also  on other lines of the AER error dump

v5:
- Fix build with AER=y and DPC=n
- Match kerneldoc and function parameter name

v4:
- Added patches:
	- Remove EXPORT of pcie_read_tlp_log()
	- Moved code to pcie/tlp.c and build only with AER enabled
	- Match variables in prototype and function
	- int -> unsigned int conversion
	- eetlp_prefix_max into own patch
- struct pcie_tlp_log param consistently called "log" within tlp.c
- Moved function prototypes into drivers/pci/pci.h
- Describe AER/DPC differences more clearly in one commit message

v3:
- Small rewording in a commit message

v2:
- Don't add EXPORT()s
- Don't include igxbe changes
- Don't use pr_cont() as it's incompatible with pci_err() and according
  to Andy Shevchenko should not be used in the first place

Ilpo Järvinen (8):
  PCI: Don't expose pcie_read_tlp_log() outside of PCI subsystem
  PCI: Move TLP Log handling to own file
  PCI: Make pcie_read_tlp_log() signature same
  PCI: Use unsigned int i in pcie_read_tlp_log()
  PCI: Store # of supported End-End TLP Prefixes
  PCI: Add TLP Prefix reading into pcie_read_tlp_log()
  PCI: Create helper to print TLP Header and Prefix Log
  PCI/AER: Add prefixes to printouts

 drivers/pci/ats.c             |   2 +-
 drivers/pci/pci.c             |  28 ---------
 drivers/pci/pci.h             |   9 +++
 drivers/pci/pcie/Makefile     |   2 +-
 drivers/pci/pcie/aer.c        |  26 ++++----
 drivers/pci/pcie/dpc.c        |  14 ++---
 drivers/pci/pcie/tlp.c        | 109 ++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c           |  14 +++--
 include/linux/aer.h           |   3 +-
 include/linux/pci.h           |   2 +-
 include/uapi/linux/pci_regs.h |   2 +
 11 files changed, 149 insertions(+), 62 deletions(-)
 create mode 100644 drivers/pci/pcie/tlp.c

-- 
2.39.2

[PATCH v6 1/8] PCI: Don't expose pcie_read_tlp_log() outside of PCI subsystem

[Ilpo Järvinen]

pcie_read_tlp_log() was exposed by the commit 0a5a46a6a61b ("PCI/AER:
Generalize TLP Header Log reading") but this is now considered a
mistake. No drivers outside of PCI subsystem should build their own
diagnostic logging but should rely on PCI core doing it for them.

There's currently one driver (ixgbe) doing it independently which was
the initial reason why the export was added but it was decided by the
PCI maintainer that it's something that should be eliminated.

Remove the unwanted EXPORT of pcie_read_tlp_log() and remove it from
include/linux/aer.h.

Link: https://lore.kernel.org/all/20240322193011.GA701027@bhelgaas/
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.c   | 1 -
 drivers/pci/pci.h   | 4 ++++
 include/linux/aer.h | 2 --
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e3a49f66982d..378fc645424f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1121,7 +1121,6 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(pcie_read_tlp_log);
 
 /**
  * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 79c8398f3938..b4fc6726eab3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -4,6 +4,8 @@
 
 #include <linux/pci.h>
 
+struct pcie_tlp_log;
+
 /* Number of possible devfns: 0.0 to 1f.7 inclusive */
 #define MAX_NR_DEVFNS 256
 
@@ -482,6 +484,8 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 #endif	/* CONFIG_PCIEAER */
 
+int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
+
 #ifdef CONFIG_PCIEPORTBUS
 /* Cached RCEC Endpoint Association */
 struct rcec_ea {
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 4b97f38f3fcf..190a0a2061cd 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -37,8 +37,6 @@ struct aer_capability_regs {
 	u16 uncor_err_source;
 };
 
-int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
-
 #if defined(CONFIG_PCIEAER)
 int pci_aer_clear_nonfatal_status(struct pci_dev *dev);
 int pcie_aer_is_native(struct pci_dev *dev);
-- 
2.39.2

Re: [PATCH v6 1/8] PCI: Don't expose pcie_read_tlp_log() outside of PCI subsystem

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:25 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> pcie_read_tlp_log() was exposed by the commit 0a5a46a6a61b ("PCI/AER:
> Generalize TLP Header Log reading") but this is now considered a
> mistake. No drivers outside of PCI subsystem should build their own
> diagnostic logging but should rely on PCI core doing it for them.
> 
> There's currently one driver (ixgbe) doing it independently which was
> the initial reason why the export was added but it was decided by the
> PCI maintainer that it's something that should be eliminated.
> 
> Remove the unwanted EXPORT of pcie_read_tlp_log() and remove it from
> include/linux/aer.h.
> 
> Link: https://lore.kernel.org/all/20240322193011.GA701027@bhelgaas/
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
FWIW LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v6 2/8] PCI: Move TLP Log handling to own file

[Ilpo Järvinen]

TLP Log is PCIe feature and is processed only by AER and DPC.
Configwise, DPC depends AER being enabled. In lack of better place, the
TLP Log handling code was initially placed into pci.c but it can be
easily placed in a separate file.

Move TLP Log handling code to own file under pcie/ subdirectory and
include it only when AER is enabled.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.c         | 27 ---------------------------
 drivers/pci/pci.h         |  2 +-
 drivers/pci/pcie/Makefile |  2 +-
 drivers/pci/pcie/tlp.c    | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 29 deletions(-)
 create mode 100644 drivers/pci/pcie/tlp.c

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 378fc645424f..47969712afe1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1095,33 +1095,6 @@ static void pci_enable_acs(struct pci_dev *dev)
 	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
 }
 
-/**
- * pcie_read_tlp_log - read TLP Header Log
- * @dev: PCIe device
- * @where: PCI Config offset of TLP Header Log
- * @tlp_log: TLP Log structure to fill
- *
- * Fill @tlp_log from TLP Header Log registers, e.g., AER or DPC.
- *
- * Return: 0 on success and filled TLP Log structure, <0 on error.
- */
-int pcie_read_tlp_log(struct pci_dev *dev, int where,
-		      struct pcie_tlp_log *tlp_log)
-{
-	int i, ret;
-
-	memset(tlp_log, 0, sizeof(*tlp_log));
-
-	for (i = 0; i < 4; i++) {
-		ret = pci_read_config_dword(dev, where + i * 4,
-					    &tlp_log->dw[i]);
-		if (ret)
-			return pcibios_err_to_errno(ret);
-	}
-
-	return 0;
-}
-
 /**
  * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
  * @dev: PCI device to have its BARs restored
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b4fc6726eab3..532bb05a0411 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -482,9 +482,9 @@ struct aer_err_info {
 
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
-#endif	/* CONFIG_PCIEAER */
 
 int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
+#endif	/* CONFIG_PCIEAER */
 
 #ifdef CONFIG_PCIEPORTBUS
 /* Cached RCEC Endpoint Association */
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 6461aa93fe76..591ef3177777 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -7,7 +7,7 @@ pcieportdrv-y			:= portdrv.o rcec.o
 obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o
 
 obj-y				+= aspm.o
-obj-$(CONFIG_PCIEAER)		+= aer.o err.o
+obj-$(CONFIG_PCIEAER)		+= aer.o err.o tlp.o
 obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
 obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
new file mode 100644
index 000000000000..3f053cc62290
--- /dev/null
+++ b/drivers/pci/pcie/tlp.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe TLP Log handling
+ *
+ * Copyright (C) 2024 Intel Corporation
+ */
+
+#include <linux/aer.h>
+#include <linux/pci.h>
+#include <linux/string.h>
+
+#include "../pci.h"
+
+/**
+ * pcie_read_tlp_log - read TLP Header Log
+ * @dev: PCIe device
+ * @where: PCI Config offset of TLP Header Log
+ * @tlp_log: TLP Log structure to fill
+ *
+ * Fill @tlp_log from TLP Header Log registers, e.g., AER or DPC.
+ *
+ * Return: 0 on success and filled TLP Log structure, <0 on error.
+ */
+int pcie_read_tlp_log(struct pci_dev *dev, int where,
+		      struct pcie_tlp_log *tlp_log)
+{
+	int i, ret;
+
+	memset(tlp_log, 0, sizeof(*tlp_log));
+
+	for (i = 0; i < 4; i++) {
+		ret = pci_read_config_dword(dev, where + i * 4,
+					    &tlp_log->dw[i]);
+		if (ret)
+			return pcibios_err_to_errno(ret);
+	}
+
+	return 0;
+}
-- 
2.39.2

Re: [PATCH v6 2/8] PCI: Move TLP Log handling to own file

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:26 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> TLP Log is PCIe feature and is processed only by AER and DPC.
> Configwise, DPC depends AER being enabled. In lack of better place, the
> TLP Log handling code was initially placed into pci.c but it can be
> easily placed in a separate file.
> 
> Move TLP Log handling code to own file under pcie/ subdirectory and
> include it only when AER is enabled.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Can sort of see this might get used outside AER sometime in the future
but for now this seems sensible.

Jonathan

[PATCH v6 3/8] PCI: Make pcie_read_tlp_log() signature same

[Ilpo Järvinen]

pcie_read_tlp_log()'s prototype and function signature diverged due to
changes made while applying.

Make the parameters of pcie_read_tlp_log() named identically.

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

diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index 3f053cc62290..2bf15749cd31 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -15,22 +15,21 @@
  * pcie_read_tlp_log - read TLP Header Log
  * @dev: PCIe device
  * @where: PCI Config offset of TLP Header Log
- * @tlp_log: TLP Log structure to fill
+ * @log: TLP Log structure to fill
  *
- * Fill @tlp_log from TLP Header Log registers, e.g., AER or DPC.
+ * Fill @log from TLP Header Log registers, e.g., AER or DPC.
  *
  * Return: 0 on success and filled TLP Log structure, <0 on error.
  */
 int pcie_read_tlp_log(struct pci_dev *dev, int where,
-		      struct pcie_tlp_log *tlp_log)
+		      struct pcie_tlp_log *log)
 {
 	int i, ret;
 
-	memset(tlp_log, 0, sizeof(*tlp_log));
+	memset(log, 0, sizeof(*log));
 
 	for (i = 0; i < 4; i++) {
-		ret = pci_read_config_dword(dev, where + i * 4,
-					    &tlp_log->dw[i]);
+		ret = pci_read_config_dword(dev, where + i * 4, &log->dw[i]);
 		if (ret)
 			return pcibios_err_to_errno(ret);
 	}
-- 
2.39.2

Re: [PATCH v6 3/8] PCI: Make pcie_read_tlp_log() signature same

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:27 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> pcie_read_tlp_log()'s prototype and function signature diverged due to
> changes made while applying.
> 
> Make the parameters of pcie_read_tlp_log() named identically.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Good to clean this up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v6 4/8] PCI: Use unsigned int i in pcie_read_tlp_log()

[Ilpo Järvinen]

Loop variable i counting from 0 upwards does not need to be signed so
make it unsigned int.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/tlp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index 2bf15749cd31..65ac7b5d8a87 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -24,7 +24,8 @@
 int pcie_read_tlp_log(struct pci_dev *dev, int where,
 		      struct pcie_tlp_log *log)
 {
-	int i, ret;
+	unsigned int i;
+	int ret;
 
 	memset(log, 0, sizeof(*log));
 
-- 
2.39.2

Re: [PATCH v6 4/8] PCI: Use unsigned int i in pcie_read_tlp_log()

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:28 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Loop variable i counting from 0 upwards does not need to be signed so
> make it unsigned int.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
To me this is unnecessary noise, but it's harmless so no problem
if you really like it.

Jonathan

> ---
>  drivers/pci/pcie/tlp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index 2bf15749cd31..65ac7b5d8a87 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -24,7 +24,8 @@
>  int pcie_read_tlp_log(struct pci_dev *dev, int where,
>  		      struct pcie_tlp_log *log)
>  {
> -	int i, ret;
> +	unsigned int i;
> +	int ret;
>  
>  	memset(log, 0, sizeof(*log));
>  

[PATCH v6 5/8] PCI: Store # of supported End-End TLP Prefixes

[Ilpo Järvinen]

eetlp_prefix_path in the struct pci_dev tells if End-End TLP Prefixes
are supported by the path or not, the value is only calculated if
CONFIG_PCI_PASID is set.

The Max End-End TLP Prefixes field in the Device Capabilities Register
2 also tells how many (1-4) End-End TLP Prefixes are supported (PCIe r6
sec 7.5.3.15). The number of supported End-End Prefixes is useful for
reading correct number of DWORDs from TLP Prefix Log register in AER
capability (PCIe r6 sec 7.8.4.12).

Replace eetlp_prefix_path with eetlp_prefix_max and determine the
number of supported End-End Prefixes regardless of CONFIG_PCI_PASID so
that an upcoming commit generalizing TLP Prefix Log register reading
does not have to read extra DWORDs for End-End Prefixes that never will
be there.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/ats.c             |  2 +-
 drivers/pci/probe.c           | 14 +++++++++-----
 include/linux/pci.h           |  2 +-
 include/uapi/linux/pci_regs.h |  1 +
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c570892b2090..e13433dcfc82 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -377,7 +377,7 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
 	if (WARN_ON(pdev->pasid_enabled))
 		return -EBUSY;
 
-	if (!pdev->eetlp_prefix_path && !pdev->pasid_no_tlp)
+	if (!pdev->eetlp_prefix_max && !pdev->pasid_no_tlp)
 		return -EINVAL;
 
 	if (!pasid)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b14b9876c030..0ab70ea6840c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2228,8 +2228,8 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
 
 static void pci_configure_eetlp_prefix(struct pci_dev *dev)
 {
-#ifdef CONFIG_PCI_PASID
 	struct pci_dev *bridge;
+	unsigned int eetlp_max;
 	int pcie_type;
 	u32 cap;
 
@@ -2241,15 +2241,19 @@ static void pci_configure_eetlp_prefix(struct pci_dev *dev)
 		return;
 
 	pcie_type = pci_pcie_type(dev);
+
+	eetlp_max = FIELD_GET(PCI_EXP_DEVCAP2_EE_PREFIX_MAX, cap);
+	/* 00b means 4 */
+	eetlp_max = eetlp_max ?: 4;
+
 	if (pcie_type == PCI_EXP_TYPE_ROOT_PORT ||
 	    pcie_type == PCI_EXP_TYPE_RC_END)
-		dev->eetlp_prefix_path = 1;
+		dev->eetlp_prefix_max = eetlp_max;
 	else {
 		bridge = pci_upstream_bridge(dev);
-		if (bridge && bridge->eetlp_prefix_path)
-			dev->eetlp_prefix_path = 1;
+		if (bridge && bridge->eetlp_prefix_max)
+			dev->eetlp_prefix_max = eetlp_max;
 	}
-#endif
 }
 
 static void pci_configure_serr(struct pci_dev *dev)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4cf89a4b4cbc..29b754d32f79 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -398,7 +398,7 @@ struct pci_dev {
 					   supported from root to here */
 #endif
 	unsigned int	pasid_no_tlp:1;		/* PASID works without TLP Prefix */
-	unsigned int	eetlp_prefix_path:1;	/* End-to-End TLP Prefix */
+	unsigned int	eetlp_prefix_max:3;	/* Max # of End-End TLP Prefixes, 0=not supported */
 
 	pci_channel_state_t error_state;	/* Current connectivity state */
 	struct device	dev;			/* Generic device interface */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 94c00996e633..b297bc30ddcb 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -661,6 +661,7 @@
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
 #define  PCI_EXP_DEVCAP2_EE_PREFIX	0x00200000 /* End-End TLP Prefix */
+#define  PCI_EXP_DEVCAP2_EE_PREFIX_MAX	0x00c00000 /* Max End-End TLP Prefixes */
 #define PCI_EXP_DEVCTL2		0x28	/* Device Control 2 */
 #define  PCI_EXP_DEVCTL2_COMP_TIMEOUT	0x000f	/* Completion Timeout Value */
 #define  PCI_EXP_DEVCTL2_COMP_TMOUT_DIS	0x0010	/* Completion Timeout Disable */
-- 
2.39.2

Re: [PATCH v6 5/8] PCI: Store # of supported End-End TLP Prefixes

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:29 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> eetlp_prefix_path in the struct pci_dev tells if End-End TLP Prefixes
> are supported by the path or not, the value is only calculated if
> CONFIG_PCI_PASID is set.
> 
> The Max End-End TLP Prefixes field in the Device Capabilities Register
> 2 also tells how many (1-4) End-End TLP Prefixes are supported (PCIe r6
> sec 7.5.3.15). The number of supported End-End Prefixes is useful for
> reading correct number of DWORDs from TLP Prefix Log register in AER
> capability (PCIe r6 sec 7.8.4.12).
> 
> Replace eetlp_prefix_path with eetlp_prefix_max and determine the
> number of supported End-End Prefixes regardless of CONFIG_PCI_PASID so
> that an upcoming commit generalizing TLP Prefix Log register reading
> does not have to read extra DWORDs for End-End Prefixes that never will
> be there.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/ats.c             |  2 +-
>  drivers/pci/probe.c           | 14 +++++++++-----
>  include/linux/pci.h           |  2 +-
>  include/uapi/linux/pci_regs.h |  1 +
>  4 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index c570892b2090..e13433dcfc82 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -377,7 +377,7 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
>  	if (WARN_ON(pdev->pasid_enabled))
>  		return -EBUSY;
>  
> -	if (!pdev->eetlp_prefix_path && !pdev->pasid_no_tlp)
> +	if (!pdev->eetlp_prefix_max && !pdev->pasid_no_tlp)
>  		return -EINVAL;
>  
>  	if (!pasid)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b14b9876c030..0ab70ea6840c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2228,8 +2228,8 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
>  
>  static void pci_configure_eetlp_prefix(struct pci_dev *dev)
>  {
> -#ifdef CONFIG_PCI_PASID
>  	struct pci_dev *bridge;
> +	unsigned int eetlp_max;
>  	int pcie_type;
>  	u32 cap;
>  
> @@ -2241,15 +2241,19 @@ static void pci_configure_eetlp_prefix(struct pci_dev *dev)
>  		return;
>  
>  	pcie_type = pci_pcie_type(dev);
> +
> +	eetlp_max = FIELD_GET(PCI_EXP_DEVCAP2_EE_PREFIX_MAX, cap);
> +	/* 00b means 4 */
> +	eetlp_max = eetlp_max ?: 4;
> +
>  	if (pcie_type == PCI_EXP_TYPE_ROOT_PORT ||
>  	    pcie_type == PCI_EXP_TYPE_RC_END)
> -		dev->eetlp_prefix_path = 1;
> +		dev->eetlp_prefix_max = eetlp_max;
>  	else {
>  		bridge = pci_upstream_bridge(dev);
> -		if (bridge && bridge->eetlp_prefix_path)
> -			dev->eetlp_prefix_path = 1;
> +		if (bridge && bridge->eetlp_prefix_max)

What happens if they disagree?  That is the bridge supports 2
and the device 3?


> +			dev->eetlp_prefix_max = eetlp_max;
>  	}
> -#endif
>  }



>  

Re: [PATCH v6 5/8] PCI: Store # of supported End-End TLP Prefixes

[Ilpo Järvinen]

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

On Wed, 11 Dec 2024, Jonathan Cameron wrote:

> On Fri, 13 Sep 2024 17:36:29 +0300
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > eetlp_prefix_path in the struct pci_dev tells if End-End TLP Prefixes
> > are supported by the path or not, the value is only calculated if
> > CONFIG_PCI_PASID is set.
> > 
> > The Max End-End TLP Prefixes field in the Device Capabilities Register
> > 2 also tells how many (1-4) End-End TLP Prefixes are supported (PCIe r6
> > sec 7.5.3.15). The number of supported End-End Prefixes is useful for
> > reading correct number of DWORDs from TLP Prefix Log register in AER
> > capability (PCIe r6 sec 7.8.4.12).
> > 
> > Replace eetlp_prefix_path with eetlp_prefix_max and determine the
> > number of supported End-End Prefixes regardless of CONFIG_PCI_PASID so
> > that an upcoming commit generalizing TLP Prefix Log register reading
> > does not have to read extra DWORDs for End-End Prefixes that never will
> > be there.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/pci/ats.c             |  2 +-
> >  drivers/pci/probe.c           | 14 +++++++++-----
> >  include/linux/pci.h           |  2 +-
> >  include/uapi/linux/pci_regs.h |  1 +
> >  4 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index c570892b2090..e13433dcfc82 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -377,7 +377,7 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
> >  	if (WARN_ON(pdev->pasid_enabled))
> >  		return -EBUSY;
> >  
> > -	if (!pdev->eetlp_prefix_path && !pdev->pasid_no_tlp)
> > +	if (!pdev->eetlp_prefix_max && !pdev->pasid_no_tlp)
> >  		return -EINVAL;
> >  
> >  	if (!pasid)
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b14b9876c030..0ab70ea6840c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2228,8 +2228,8 @@ static void pci_configure_relaxed_ordering(struct pci_dev *dev)
> >  
> >  static void pci_configure_eetlp_prefix(struct pci_dev *dev)
> >  {
> > -#ifdef CONFIG_PCI_PASID
> >  	struct pci_dev *bridge;
> > +	unsigned int eetlp_max;
> >  	int pcie_type;
> >  	u32 cap;
> >  
> > @@ -2241,15 +2241,19 @@ static void pci_configure_eetlp_prefix(struct pci_dev *dev)
> >  		return;
> >  
> >  	pcie_type = pci_pcie_type(dev);
> > +
> > +	eetlp_max = FIELD_GET(PCI_EXP_DEVCAP2_EE_PREFIX_MAX, cap);
> > +	/* 00b means 4 */
> > +	eetlp_max = eetlp_max ?: 4;
> > +
> >  	if (pcie_type == PCI_EXP_TYPE_ROOT_PORT ||
> >  	    pcie_type == PCI_EXP_TYPE_RC_END)
> > -		dev->eetlp_prefix_path = 1;
> > +		dev->eetlp_prefix_max = eetlp_max;
> >  	else {
> >  		bridge = pci_upstream_bridge(dev);
> > -		if (bridge && bridge->eetlp_prefix_path)
> > -			dev->eetlp_prefix_path = 1;
> > +		if (bridge && bridge->eetlp_prefix_max)
> 
> What happens if they disagree?  That is the bridge supports 2
> and the device 3?

That's a good question.

The current code obviously only checks if Prefixes are supported or not so 
the max value doesn't matter for the existing code.

I went to read spec and my reading from TLP logging point of view is that 
the device's own maximum matters even if it might never get >2 Prefixes
(r6.1 2.2.10.4 & 6.2.4).

AFAIK, things happen on low level and there's no way to program this 
value. So it's not like the kernel is telling that hw must only use x 
Prefixes at most if that's what you were worried about.

But there are more things to consider, e.g., I noticed End-End TLP Prefix 
Blocking in DevCtl2 which might impact the existing usage too and is not
checked by the kernel currently.

My interest here lies on cleaning this up so I'm not sure if functional 
changes such as considering End-End TLP Prefix Blocking really matter for 
some case or not. I can obviously easily add the code for that too if it's 
required for this series to be acceptable but I don't have a test case for 
it. My main goal with this TLP logging consolidation series is to get to a 
point that extending it to support Flit mode is easier.

There's also TLP Prefix Log Present which I think the reader should 
consider, which matters to another patch in this series and I'm going to 
alter the length based on that flag.

-- 
 i.

[PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Ilpo Järvinen]

pcie_read_tlp_log() handles only 4 Header Log DWORDs but TLP Prefix Log
(PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.

Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
TLP Prefix Log. The relevant registers are formatted identically in AER
and DPC Capability, but has these variations:

a) The offsets of TLP Prefix Log registers vary.
b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.

Therefore callers must pass the offset of the TLP Prefix Log register
and the entire length to pcie_read_tlp_log() to be able to read the
correct number of TLP Prefix DWORDs from the correct offset.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.h             |  5 +++-
 drivers/pci/pcie/aer.c        |  4 ++-
 drivers/pci/pcie/dpc.c        | 13 +++++-----
 drivers/pci/pcie/tlp.c        | 49 +++++++++++++++++++++++++++++++----
 include/linux/aer.h           |  1 +
 include/uapi/linux/pci_regs.h |  1 +
 6 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 532bb05a0411..b47844b97428 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -483,7 +483,9 @@ struct aer_err_info {
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 
-int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
+int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
+		      unsigned int tlp_len, struct pcie_tlp_log *log);
+unsigned int aer_tlp_log_len(struct pci_dev *dev);
 #endif	/* CONFIG_PCIEAER */
 
 #ifdef CONFIG_PCIEPORTBUS
@@ -502,6 +504,7 @@ void pci_dpc_init(struct pci_dev *pdev);
 void dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
+unsigned int dpc_tlp_log_len(struct pci_dev *dev);
 #else
 static inline void pci_save_dpc_state(struct pci_dev *dev) { }
 static inline void pci_restore_dpc_state(struct pci_dev *dev) { }
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 13b8586924ea..651d0c72802a 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1245,7 +1245,9 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 
 		if (info->status & AER_LOG_TLP_MASKS) {
 			info->tlp_header_valid = 1;
-			pcie_read_tlp_log(dev, aer + PCI_ERR_HEADER_LOG, &info->tlp);
+			pcie_read_tlp_log(dev, aer + PCI_ERR_HEADER_LOG,
+					  aer + PCI_ERR_PREFIX_LOG,
+					  aer_tlp_log_len(dev), &info->tlp);
 		}
 	}
 
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 2b6ef7efa3c1..7933b3cedb59 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -190,7 +190,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 {
 	u16 cap = pdev->dpc_cap, dpc_status, first_error;
-	u32 status, mask, sev, syserr, exc, log, prefix;
+	u32 status, mask, sev, syserr, exc, log;
 	struct pcie_tlp_log tlp_log;
 	int i;
 
@@ -217,20 +217,19 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 
 	if (pdev->dpc_rp_log_size < 4)
 		goto clear_status;
-	pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG, &tlp_log);
+	pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
+			  cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
+			  dpc_tlp_log_len(pdev), &tlp_log);
 	pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
 		tlp_log.dw[0], tlp_log.dw[1], tlp_log.dw[2], tlp_log.dw[3]);
+	for (i = 0; i < pdev->dpc_rp_log_size - 5; i++)
+		pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, tlp_log.prefix[i]);
 
 	if (pdev->dpc_rp_log_size < 5)
 		goto clear_status;
 	pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
 	pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
 
-	for (i = 0; i < pdev->dpc_rp_log_size - 5; i++) {
-		pci_read_config_dword(pdev,
-			cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG + i * 4, &prefix);
-		pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
-	}
  clear_status:
 	pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
 }
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index 65ac7b5d8a87..def9dd7b73e8 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -11,26 +11,65 @@
 
 #include "../pci.h"
 
+/**
+ * aer_tlp_log_len - Calculates AER Capability TLP Header/Prefix Log length
+ * @dev: PCIe device
+ *
+ * Return: TLP Header/Prefix Log length
+ */
+unsigned int aer_tlp_log_len(struct pci_dev *dev)
+{
+	return 4 + dev->eetlp_prefix_max;
+}
+
+#ifdef CONFIG_PCIE_DPC
+/**
+ * dpc_tlp_log_len - Calculates DPC RP PIO TLP Header/Prefix Log length
+ * @dev: PCIe device
+ *
+ * Return: TLP Header/Prefix Log length
+ */
+unsigned int dpc_tlp_log_len(struct pci_dev *dev)
+{
+	/* Remove ImpSpec Log register from the count */
+	if (dev->dpc_rp_log_size >= 5)
+		return dev->dpc_rp_log_size - 1;
+
+	return dev->dpc_rp_log_size;
+}
+#endif
+
 /**
  * pcie_read_tlp_log - read TLP Header Log
  * @dev: PCIe device
  * @where: PCI Config offset of TLP Header Log
+ * @where2: PCI Config offset of TLP Prefix Log
+ * @tlp_len: TLP Log length (Header Log + TLP Prefix Log in DWORDs)
  * @log: TLP Log structure to fill
  *
  * Fill @log from TLP Header Log registers, e.g., AER or DPC.
  *
  * Return: 0 on success and filled TLP Log structure, <0 on error.
  */
-int pcie_read_tlp_log(struct pci_dev *dev, int where,
-		      struct pcie_tlp_log *log)
+int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
+		      unsigned int tlp_len, struct pcie_tlp_log *log)
 {
 	unsigned int i;
-	int ret;
+	int off, ret;
+	u32 *to;
 
 	memset(log, 0, sizeof(*log));
 
-	for (i = 0; i < 4; i++) {
-		ret = pci_read_config_dword(dev, where + i * 4, &log->dw[i]);
+	for (i = 0; i < tlp_len; i++) {
+		if (i < 4) {
+			off = where + i * 4;
+			to = &log->dw[i];
+		} else {
+			off = where2 + (i - 4) * 4;
+			to = &log->prefix[i - 4];
+		}
+
+		ret = pci_read_config_dword(dev, off, to);
 		if (ret)
 			return pcibios_err_to_errno(ret);
 	}
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 190a0a2061cd..dc498adaa1c8 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -20,6 +20,7 @@ struct pci_dev;
 
 struct pcie_tlp_log {
 	u32 dw[4];
+	u32 prefix[4];
 };
 
 struct aer_capability_regs {
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index b297bc30ddcb..b6f9012a3fc4 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -803,6 +803,7 @@
 #define  PCI_ERR_ROOT_FATAL_RCV		0x00000040 /* Fatal Received */
 #define  PCI_ERR_ROOT_AER_IRQ		0xf8000000 /* Advanced Error Interrupt Message Number */
 #define PCI_ERR_ROOT_ERR_SRC	0x34	/* Error Source Identification */
+#define PCI_ERR_PREFIX_LOG	0x38	/* TLP Prefix LOG Register (up to 16 bytes) */
 
 /* Virtual Channel */
 #define PCI_VC_PORT_CAP1	0x04
-- 
2.39.2

Re: [PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:30 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> pcie_read_tlp_log() handles only 4 Header Log DWORDs but TLP Prefix Log
> (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
> 
> Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
> TLP Prefix Log. The relevant registers are formatted identically in AER
> and DPC Capability, but has these variations:
> 
> a) The offsets of TLP Prefix Log registers vary.
> b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.
> 
> Therefore callers must pass the offset of the TLP Prefix Log register
> and the entire length to pcie_read_tlp_log() to be able to read the
> correct number of TLP Prefix DWORDs from the correct offset.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Trivial comments below
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Would have been nice if they'd just made the formats have the
same sized holes etc!

> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index 65ac7b5d8a87..def9dd7b73e8 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -11,26 +11,65 @@

>  /**
>   * pcie_read_tlp_log - read TLP Header Log
Maybe update this to read TLP Header and Prefix Logs
>   * @dev: PCIe device
>   * @where: PCI Config offset of TLP Header Log
> + * @where2: PCI Config offset of TLP Prefix Log

Is it worth giving it a more specific name than where2?
Possibly renaming where as well!

> + * @tlp_len: TLP Log length (Header Log + TLP Prefix Log in DWORDs)
>   * @log: TLP Log structure to fill
>   *
>   * Fill @log from TLP Header Log registers, e.g., AER or DPC.
>   *
>   * Return: 0 on success and filled TLP Log structure, <0 on error.
>   */
> -int pcie_read_tlp_log(struct pci_dev *dev, int where,
> -		      struct pcie_tlp_log *log)
> +int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
> +		      unsigned int tlp_len, struct pcie_tlp_log *log)
>  {

Re: [PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Ilpo Järvinen]

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

On Wed, 11 Dec 2024, Jonathan Cameron wrote:

> On Fri, 13 Sep 2024 17:36:30 +0300
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > pcie_read_tlp_log() handles only 4 Header Log DWORDs but TLP Prefix Log
> > (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
> > 
> > Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
> > TLP Prefix Log. The relevant registers are formatted identically in AER
> > and DPC Capability, but has these variations:
> > 
> > a) The offsets of TLP Prefix Log registers vary.
> > b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.
> > 
> > Therefore callers must pass the offset of the TLP Prefix Log register
> > and the entire length to pcie_read_tlp_log() to be able to read the
> > correct number of TLP Prefix DWORDs from the correct offset.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Trivial comments below
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Would have been nice if they'd just made the formats have the
> same sized holes etc!

That's not even the worst problem.

They managed to copy-paste most of the stuff into DPC (copy-paste is 
really obvious because the text still refers to AER in a DPC section :-)) 
but forgot to add a few capability fields into the DPC capability, most 
importantly, the bit that indicates whether TLP was logged in Flit mode
or not And now we get to keep the pieces how to interpret the Log 
Registers (relates to the follow up series). :-(

> > diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> > index 65ac7b5d8a87..def9dd7b73e8 100644
> > --- a/drivers/pci/pcie/tlp.c
> > +++ b/drivers/pci/pcie/tlp.c
> > @@ -11,26 +11,65 @@
> 
> >  /**
> >   * pcie_read_tlp_log - read TLP Header Log
> Maybe update this to read TLP Header and Prefix Logs
> >   * @dev: PCIe device
> >   * @where: PCI Config offset of TLP Header Log
> > + * @where2: PCI Config offset of TLP Prefix Log
> 
> Is it worth giving it a more specific name than where2?
> Possibly renaming where as well!

Sure, why not.

-- 
 i.

> > + * @tlp_len: TLP Log length (Header Log + TLP Prefix Log in DWORDs)
> >   * @log: TLP Log structure to fill
> >   *
> >   * Fill @log from TLP Header Log registers, e.g., AER or DPC.
> >   *
> >   * Return: 0 on success and filled TLP Log structure, <0 on error.
> >   */
> > -int pcie_read_tlp_log(struct pci_dev *dev, int where,
> > -		      struct pcie_tlp_log *log)
> > +int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
> > +		      unsigned int tlp_len, struct pcie_tlp_log *log)
> >  {

Re: [PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Ilpo Järvinen]

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

On Thu, 12 Dec 2024, Ilpo Järvinen wrote:

> On Wed, 11 Dec 2024, Jonathan Cameron wrote:
> 
> > On Fri, 13 Sep 2024 17:36:30 +0300
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > 
> > > pcie_read_tlp_log() handles only 4 Header Log DWORDs but TLP Prefix Log
> > > (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
> > > 
> > > Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
> > > TLP Prefix Log. The relevant registers are formatted identically in AER
> > > and DPC Capability, but has these variations:
> > > 
> > > a) The offsets of TLP Prefix Log registers vary.
> > > b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.
> > > 
> > > Therefore callers must pass the offset of the TLP Prefix Log register
> > > and the entire length to pcie_read_tlp_log() to be able to read the
> > > correct number of TLP Prefix DWORDs from the correct offset.
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > Trivial comments below
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Would have been nice if they'd just made the formats have the
> > same sized holes etc!
> 
> That's not even the worst problem.
> 
> They managed to copy-paste most of the stuff into DPC (copy-paste is 
> really obvious because the text still refers to AER in a DPC section :-)) 
> but forgot to add a few capability fields into the DPC capability, most 
> importantly, the bit that indicates whether TLP was logged in Flit mode
> or not And now we get to keep the pieces how to interpret the Log 
> Registers (relates to the follow up series). :-(
> 
> > > diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> > > index 65ac7b5d8a87..def9dd7b73e8 100644
> > > --- a/drivers/pci/pcie/tlp.c
> > > +++ b/drivers/pci/pcie/tlp.c
> > > @@ -11,26 +11,65 @@
> > 
> > >  /**
> > >   * pcie_read_tlp_log - read TLP Header Log
> > Maybe update this to read TLP Header and Prefix Logs
> > >   * @dev: PCIe device
> > >   * @where: PCI Config offset of TLP Header Log
> > > + * @where2: PCI Config offset of TLP Prefix Log
> > 
> > Is it worth giving it a more specific name than where2?
> > Possibly renaming where as well!
> 
> Sure, why not.

Hi again,

After doing this rename, I rebased the Flit mode series on top of it and
realized there's one small problem with naming where2. It will be 
overloaded between TLP Prefix Log (Non-Flit mode) and extended TLP Header 
Log (DW5+) (Flit mode) so I'm not sure if there's really going to be a 
good name for it.

-- 
 i.

Re: [PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Jonathan Cameron]

On Thu, 12 Dec 2024 20:48:38 +0200 (EET)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 12 Dec 2024, Ilpo Järvinen wrote:
> 
> > On Wed, 11 Dec 2024, Jonathan Cameron wrote:
> >   
> > > On Fri, 13 Sep 2024 17:36:30 +0300
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > >   
> > > > pcie_read_tlp_log() handles only 4 Header Log DWORDs but TLP Prefix Log
> > > > (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
> > > > 
> > > > Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
> > > > TLP Prefix Log. The relevant registers are formatted identically in AER
> > > > and DPC Capability, but has these variations:
> > > > 
> > > > a) The offsets of TLP Prefix Log registers vary.
> > > > b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.
> > > > 
> > > > Therefore callers must pass the offset of the TLP Prefix Log register
> > > > and the entire length to pcie_read_tlp_log() to be able to read the
> > > > correct number of TLP Prefix DWORDs from the correct offset.
> > > > 
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>  
> > > 
> > > Trivial comments below
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Would have been nice if they'd just made the formats have the
> > > same sized holes etc!  
> > 
> > That's not even the worst problem.
> > 
> > They managed to copy-paste most of the stuff into DPC (copy-paste is 
> > really obvious because the text still refers to AER in a DPC section :-)) 
> > but forgot to add a few capability fields into the DPC capability, most 
> > importantly, the bit that indicates whether TLP was logged in Flit mode
> > or not And now we get to keep the pieces how to interpret the Log 
> > Registers (relates to the follow up series). :-(
> >   
> > > > diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> > > > index 65ac7b5d8a87..def9dd7b73e8 100644
> > > > --- a/drivers/pci/pcie/tlp.c
> > > > +++ b/drivers/pci/pcie/tlp.c
> > > > @@ -11,26 +11,65 @@  
> > >   
> > > >  /**
> > > >   * pcie_read_tlp_log - read TLP Header Log  
> > > Maybe update this to read TLP Header and Prefix Logs  
> > > >   * @dev: PCIe device
> > > >   * @where: PCI Config offset of TLP Header Log
> > > > + * @where2: PCI Config offset of TLP Prefix Log  
> > > 
> > > Is it worth giving it a more specific name than where2?
> > > Possibly renaming where as well!  
> > 
> > Sure, why not.  
> 
> Hi again,
> 
> After doing this rename, I rebased the Flit mode series on top of it and
> realized there's one small problem with naming where2. It will be 
> overloaded between TLP Prefix Log (Non-Flit mode) and extended TLP Header 
> Log (DW5+) (Flit mode) so I'm not sure if there's really going to be a 
> good name for it.
> 
Ah. I should read that series ;)

Absolutely fine - sometime generic naming is the best we can do!

Jonathan

[PATCH v6 7/8] PCI: Create helper to print TLP Header and Prefix Log

[Ilpo Järvinen]

Add pcie_print_tlp_log() helper to print TLP Header and Prefix Log.
Print End-End Prefixes only if they are non-zero.

Consolidate the few places which currently print TLP using custom
formatting.

The first attempt used pr_cont() instead of building a string first but
it turns out pr_cont() is not compatible with pci_err() and prints on a
separate line. When I asked about this, Andy Shevchenko suggested
pr_cont() should not be used in the first place (to eventually get rid
of it) so pr_cont() is now replaced with building the string first.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.h      |  2 ++
 drivers/pci/pcie/aer.c | 10 ++--------
 drivers/pci/pcie/dpc.c |  5 +----
 drivers/pci/pcie/tlp.c | 31 +++++++++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b47844b97428..cd6ada5d5980 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -486,6 +486,8 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
 int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 		      unsigned int tlp_len, struct pcie_tlp_log *log);
 unsigned int aer_tlp_log_len(struct pci_dev *dev);
+void pcie_print_tlp_log(const struct pci_dev *dev,
+			const struct pcie_tlp_log *log, const char *pfx);
 #endif	/* CONFIG_PCIEAER */
 
 #ifdef CONFIG_PCIEPORTBUS
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 651d0c72802a..6484e3a66a41 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -664,12 +664,6 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
 	}
 }
 
-static void __print_tlp_header(struct pci_dev *dev, struct pcie_tlp_log *t)
-{
-	pci_err(dev, "  TLP Header: %08x %08x %08x %08x\n",
-		t->dw[0], t->dw[1], t->dw[2], t->dw[3]);
-}
-
 static void __aer_print_error(struct pci_dev *dev,
 			      struct aer_err_info *info)
 {
@@ -724,7 +718,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 	__aer_print_error(dev, info);
 
 	if (info->tlp_header_valid)
-		__print_tlp_header(dev, &info->tlp);
+		pcie_print_tlp_log(dev, &info->tlp, dev_fmt("  "));
 
 out:
 	if (info->id && info->error_dev_num > 1 && info->id == id)
@@ -796,7 +790,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 			aer->uncor_severity);
 
 	if (tlp_header_valid)
-		__print_tlp_header(dev, &aer->header_log);
+		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
 
 	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
 			aer_severity, tlp_header_valid, &aer->header_log);
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7933b3cedb59..86ae1cf88787 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -220,10 +220,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 	pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
 			  cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
 			  dpc_tlp_log_len(pdev), &tlp_log);
-	pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
-		tlp_log.dw[0], tlp_log.dw[1], tlp_log.dw[2], tlp_log.dw[3]);
-	for (i = 0; i < pdev->dpc_rp_log_size - 5; i++)
-		pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, tlp_log.prefix[i]);
+	pcie_print_tlp_log(pdev, &tlp_log, dev_fmt(""));
 
 	if (pdev->dpc_rp_log_size < 5)
 		goto clear_status;
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index def9dd7b73e8..097ac8514e96 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/aer.h>
+#include <linux/array_size.h>
 #include <linux/pci.h>
 #include <linux/string.h>
 
@@ -76,3 +77,33 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 
 	return 0;
 }
+
+/**
+ * pcie_print_tlp_log - Print TLP Header / Prefix Log contents
+ * @dev: PCIe device
+ * @log: TLP Log structure
+ * @pfx: String prefix (for print out indentation)
+ *
+ * Prints TLP Header and Prefix Log information held by @log.
+ */
+void pcie_print_tlp_log(const struct pci_dev *dev,
+			const struct pcie_tlp_log *log, const char *pfx)
+{
+	char buf[(10 + 1) * (4 + ARRAY_SIZE(log->prefix)) + 14 + 1];
+	unsigned int i;
+	int len;
+
+	len = scnprintf(buf, sizeof(buf), "%#010x %#010x %#010x %#010x",
+			log->dw[0], log->dw[1], log->dw[2], log->dw[3]);
+
+	if (log->prefix[0])
+		len += scnprintf(buf + len, sizeof(buf) - len, " E-E Prefixes:");
+	for (i = 0; i < ARRAY_SIZE(log->prefix); i++) {
+		if (!log->prefix[i])
+			break;
+		len += scnprintf(buf + len, sizeof(buf) - len,
+				 " %#010x", log->prefix[i]);
+	}
+
+	pci_err(dev, "%sTLP Header: %s\n", pfx, buf);
+}
-- 
2.39.2

Re: [PATCH v6 7/8] PCI: Create helper to print TLP Header and Prefix Log

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:31 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Add pcie_print_tlp_log() helper to print TLP Header and Prefix Log.
> Print End-End Prefixes only if they are non-zero.
> 
> Consolidate the few places which currently print TLP using custom
> formatting.
> 
> The first attempt used pr_cont() instead of building a string first but
> it turns out pr_cont() is not compatible with pci_err() and prints on a
> separate line. When I asked about this, Andy Shevchenko suggested
> pr_cont() should not be used in the first place (to eventually get rid
> of it) so pr_cont() is now replaced with building the string first.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

A couple of trivial things inline but looks good to me either way.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index def9dd7b73e8..097ac8514e96 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/aer.h>
> +#include <linux/array_size.h>
>  #include <linux/pci.h>
>  #include <linux/string.h>
>  
> @@ -76,3 +77,33 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>  
>  	return 0;
>  }
> +
> +/**
> + * pcie_print_tlp_log - Print TLP Header / Prefix Log contents
> + * @dev: PCIe device
> + * @log: TLP Log structure
> + * @pfx: String prefix (for print out indentation)

Code doesn't care if it is indentation or ponies.  So does it make
sense to say anything beyond String prefix?

> + *
> + * Prints TLP Header and Prefix Log information held by @log.
> + */
> +void pcie_print_tlp_log(const struct pci_dev *dev,
> +			const struct pcie_tlp_log *log, const char *pfx)
> +{
> +	char buf[(10 + 1) * (4 + ARRAY_SIZE(log->prefix)) + 14 + 1];

Can we associate the 14 with the prefixes string by having that as a
const char * and using strlen()  It was a tiny bit irritating to count
the characters whilst reviewing this ;)



> +	unsigned int i;
> +	int len;
> +
> +	len = scnprintf(buf, sizeof(buf), "%#010x %#010x %#010x %#010x",
> +			log->dw[0], log->dw[1], log->dw[2], log->dw[3]);
> +
> +	if (log->prefix[0])
> +		len += scnprintf(buf + len, sizeof(buf) - len, " E-E Prefixes:");
> +	for (i = 0; i < ARRAY_SIZE(log->prefix); i++) {
> +		if (!log->prefix[i])
> +			break;
> +		len += scnprintf(buf + len, sizeof(buf) - len,
> +				 " %#010x", log->prefix[i]);
> +	}
> +
> +	pci_err(dev, "%sTLP Header: %s\n", pfx, buf);
> +}

[PATCH v6 8/8] PCI/AER: Add prefixes to printouts

[Ilpo Järvinen]

Only part of the AER diagnostic printouts use "AER:" prefix because
they use low-level pci_printk() directly to allow selecting level.

Add "AER:" prefix to lines that are printed with pci_printk().

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

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 6484e3a66a41..c4ba4396a8dc 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -685,8 +685,8 @@ static void __aer_print_error(struct pci_dev *dev,
 		if (!errmsg)
 			errmsg = "Unknown Error Bit";
 
-		pci_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
-				info->first_error == i ? " (First)" : "");
+		pci_printk(level, dev, "%s   [%2d] %-22s%s\n", dev_fmt(""), i,
+			   errmsg, info->first_error == i ? " (First)" : "");
 	}
 	pci_dev_aer_stats_incr(dev, info);
 }
@@ -708,12 +708,12 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 
 	level = (info->severity == AER_CORRECTABLE) ? KERN_WARNING : KERN_ERR;
 
-	pci_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
-		   aer_error_severity_string[info->severity],
+	pci_printk(level, dev, "%sPCIe Bus Error: severity=%s, type=%s, (%s)\n",
+		   dev_fmt(""), aer_error_severity_string[info->severity],
 		   aer_error_layer[layer], aer_agent_string[agent]);
 
-	pci_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
-		   dev->vendor, dev->device, info->status, info->mask);
+	pci_printk(level, dev, "%s  device [%04x:%04x] error status/mask=%08x/%08x\n",
+		   dev_fmt(""), dev->vendor, dev->device, info->status, info->mask);
 
 	__aer_print_error(dev, info);
 
-- 
2.39.2

Re: [PATCH v6 8/8] PCI/AER: Add prefixes to printouts

[Jonathan Cameron]

On Fri, 13 Sep 2024 17:36:32 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Only part of the AER diagnostic printouts use "AER:" prefix because
> they use low-level pci_printk() directly to allow selecting level.
> 
> Add "AER:" prefix to lines that are printed with pci_printk().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Makes sense.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v6 0/8] PCI: Consolidate TLP Log reading and printing

[Ilpo Järvinen]

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

On Fri, 13 Sep 2024, Ilpo Järvinen wrote:

> This series has the remaining patches of the AER & DPC TLP Log handling
> consolidation and now includes a few minor improvements to the earlier
> accepted TLP Logging code.
> 
> v6:
> - Preserve "AER:"/"DPC:" prefix on the printed TLP line
> - New patch to add "AER:" also  on other lines of the AER error dump

Hi Bjorn,

A small reminder this series exists.

-- 
 i.

> v5:
> - Fix build with AER=y and DPC=n
> - Match kerneldoc and function parameter name
> 
> v4:
> - Added patches:
> 	- Remove EXPORT of pcie_read_tlp_log()
> 	- Moved code to pcie/tlp.c and build only with AER enabled
> 	- Match variables in prototype and function
> 	- int -> unsigned int conversion
> 	- eetlp_prefix_max into own patch
> - struct pcie_tlp_log param consistently called "log" within tlp.c
> - Moved function prototypes into drivers/pci/pci.h
> - Describe AER/DPC differences more clearly in one commit message
> 
> v3:
> - Small rewording in a commit message
> 
> v2:
> - Don't add EXPORT()s
> - Don't include igxbe changes
> - Don't use pr_cont() as it's incompatible with pci_err() and according
>   to Andy Shevchenko should not be used in the first place
> 
> Ilpo Järvinen (8):
>   PCI: Don't expose pcie_read_tlp_log() outside of PCI subsystem
>   PCI: Move TLP Log handling to own file
>   PCI: Make pcie_read_tlp_log() signature same
>   PCI: Use unsigned int i in pcie_read_tlp_log()
>   PCI: Store # of supported End-End TLP Prefixes
>   PCI: Add TLP Prefix reading into pcie_read_tlp_log()
>   PCI: Create helper to print TLP Header and Prefix Log
>   PCI/AER: Add prefixes to printouts
> 
>  drivers/pci/ats.c             |   2 +-
>  drivers/pci/pci.c             |  28 ---------
>  drivers/pci/pci.h             |   9 +++
>  drivers/pci/pcie/Makefile     |   2 +-
>  drivers/pci/pcie/aer.c        |  26 ++++----
>  drivers/pci/pcie/dpc.c        |  14 ++---
>  drivers/pci/pcie/tlp.c        | 109 ++++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c           |  14 +++--
>  include/linux/aer.h           |   3 +-
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |   2 +
>  11 files changed, 149 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/pci/pcie/tlp.c
> 
> 

Re: [PATCH v6 0/8] PCI: Consolidate TLP Log reading and printing

[Ilpo Järvinen]

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

On Wed, 23 Oct 2024, Ilpo Järvinen wrote:

> On Fri, 13 Sep 2024, Ilpo Järvinen wrote:
> 
> > This series has the remaining patches of the AER & DPC TLP Log handling
> > consolidation and now includes a few minor improvements to the earlier
> > accepted TLP Logging code.
> > 
> > v6:
> > - Preserve "AER:"/"DPC:" prefix on the printed TLP line
> > - New patch to add "AER:" also  on other lines of the AER error dump
> 
> Hi Bjorn,
> 
> A small reminder this series exists.

Another reminder about this series. :-)

-- 
 i.