Re: [PATCH 3/4] PCI: Add TLP Prefix reading into pcie_read_tlp_log()
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2024-03-22 19:30:13
Also in:
intel-wired-lan, linux-edac, linux-efi, linux-pci, linuxppc-dev, lkml
On Tue, Feb 06, 2024 at 03:57:16PM +0200, Ilpo Järvinen wrote:
pcie_read_tlp_log() handles only 4 TLP Header Log DWORDs but TLP Prefix Log (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
s/TLP Header Log/Header Log/ to match spec terminology (also below)
Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also TLP Prefix Log. The layout of relevant registers in AER and DPC Capability is not identical but the offsets of TLP Header Log and TLP Prefix Log vary so the callers must pass the offsets to pcie_read_tlp_log().
s/is not identical but/is identical, but/ ? The spec is a little obtuse about Header Log Size.
Convert eetlp_prefix_path into integer called eetlp_prefix_max and make is available also when CONFIG_PCI_PASID is not configured to be able to determine the number of E-E Prefixes.
I think this eetlp_prefix_path piece is right, but would be nice in a separate patch since it's a little bit different piece to review.
quoted hunk ↗ jump to hunk
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c@@ -11336,7 +11336,9 @@ static pci_ers_result_t ixgbe_io_error_detected(struct pci_dev *pdev, if (!pos) goto skip_bad_vf_detection; - ret = pcie_read_tlp_log(pdev, pos + PCI_ERR_HEADER_LOG, &tlp_log); + ret = pcie_read_tlp_log(pdev, pos + PCI_ERR_HEADER_LOG, + pos + PCI_ERR_PREFIX_LOG, + aer_tlp_log_len(pdev), &tlp_log); if (ret < 0) { ixgbe_check_cfg_remove(hw, pdev); goto skip_bad_vf_detection;
We applied the patch to export pcie_read_tlp_log(), but I'm having
second thoughts about it. I don't think drivers really have any
business here, and I'd rather not expose either pcie_read_tlp_log() or
aer_tlp_log_len().
This part of ixgbe_io_error_detected() was added by 83c61fa97a7d
("ixgbe: Add protection from VF invalid target DMA"), and to me it
looks like debug code that probably doesn't need to be there as long
as the PCI core does the appropriate logging.
Bjorn