On Thu, Jan 09, 2025 at 11:36:17AM +0200, Ilpo Järvinen wrote:

[...]

quoted

quoted

-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);

Could we do two loops? Sorry if this was already discussed.

	for (i = 0; i < min(tlp_len, BASE_NR_TLP); i++, where += 4, tlp_len--) {
		ret = pci_read_config_dword(dev, where, &log->dw[i]);
		if (ret)
			return pcibios_err_to_errno(ret);
	}

	for (i = 0; i < tlp_len; i++, where2 += 4) {
		ret = pci_read_config_dword(dev, where2, &log->prefix[i]);
		if (ret)
			return pcibios_err_to_errno(ret);
	}

I'm not convinced splitting it would be clearly better. After the flit 
mode patch, only variation that will remain is the offset calculation 
(extended ->dw[] entires will be overlapping with ->prefix[] using union 
trickery so I can just use ->dw[i] in the loop).

Ah okay, no problem.

quoted

quoted

 	}

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];

Another place for "BASE_NR_*".

quoted

 };
 
 struct aer_capability_regs {

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 14a6306c4ce1..82866ac0bda7 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h

@@ -790,10 +790,11 @@
 	/* Same bits as above */
 #define PCI_ERR_CAP		0x18	/* Advanced Error Capabilities & Ctrl*/
 #define  PCI_ERR_CAP_FEP(x)	((x) & 0x1f)	/* First Error Pointer */
-#define  PCI_ERR_CAP_ECRC_GENC	0x00000020	/* ECRC Generation Capable */
-#define  PCI_ERR_CAP_ECRC_GENE	0x00000040	/* ECRC Generation Enable */
-#define  PCI_ERR_CAP_ECRC_CHKC	0x00000080	/* ECRC Check Capable */
-#define  PCI_ERR_CAP_ECRC_CHKE	0x00000100	/* ECRC Check Enable */
+#define  PCI_ERR_CAP_ECRC_GENC		0x00000020 /* ECRC Generation Capable */
+#define  PCI_ERR_CAP_ECRC_GENE		0x00000040 /* ECRC Generation Enable */
+#define  PCI_ERR_CAP_ECRC_CHKC		0x00000080 /* ECRC Check Capable */
+#define  PCI_ERR_CAP_ECRC_CHKE		0x00000100 /* ECRC Check Enable */
+#define  PCI_ERR_CAP_PREFIX_LOG_PRESENT	0x00000800 /* TLP Prefix Log Present */

I didn't think to mention this in a previous patch, but could/should we
use GENMASK() for bitmasks updates? I know it's a break from the current
style though.

Bjorn called himself "a dinosaur" when it comes to GENMASK() :-):

https://lore.kernel.org/linux-pci/20231031200312.GA25127@bhelgaas/ (local)

I'll try to remember. :)

Thanks,
Yazen