Re: [PATCH 3/4] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

2024-03-22 Thread Bjorn Helgaas
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.

> +++ 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, _log);
> + ret = pcie_read_tlp_log(pdev, pos + PCI_ERR_HEADER_LOG,
> + pos + PCI_ERR_PREFIX_LOG,
> + aer_tlp_log_len(pdev), _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


[PATCH 3/4] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

2024-02-06 Thread Ilpo Järvinen
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.

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().

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.

Signed-off-by: Ilpo Järvinen 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  4 +-
 drivers/pci/ats.c |  2 +-
 drivers/pci/pci.c | 37 ---
 drivers/pci/pcie/aer.c|  4 +-
 drivers/pci/pcie/dpc.c| 22 +++
 drivers/pci/probe.c   | 14 ---
 include/linux/aer.h   |  5 ++-
 include/linux/pci.h   |  2 +-
 include/uapi/linux/pci_regs.h |  2 +
 9 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5fdf37968b2d..6ce720726a1a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ 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, _log);
+   ret = pcie_read_tlp_log(pdev, pos + PCI_ERR_HEADER_LOG,
+   pos + PCI_ERR_PREFIX_LOG,
+   aer_tlp_log_len(pdev), _log);
if (ret < 0) {
ixgbe_check_cfg_remove(hw, pdev);
goto skip_bad_vf_detection;
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/pci.c b/drivers/pci/pci.c
index 0152f0144eec..268a5b9f1dff 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1068,23 +1068,48 @@ static void pci_enable_acs(struct pci_dev *dev)
 }
 
 /**
- * pcie_read_tlp_log - Reads TLP Header Log
+ * aer_tlp_log_len - Calculates 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;
+}
+EXPORT_SYMBOL_GPL(aer_tlp_log_len);
+
+/**
+ * pcie_read_tlp_log - Reads TLP Header and Prefix 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 (in DWORDs)
  * @tlp_log:   TLP Log structure to fill
  *
- * Fills @tlp_log from TLP Header Log registers.
+ * Fills @tlp_log from TLP Header and Prefix Log registers.
  *
  * 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 pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
+ unsigned int tlp_len, struct pcie_tlp_log *tlp_log)
 {
-   int i, ret;
+   unsigned int i;
+   int off, ret;
+   u32 *to;
 
memset(tlp_log, 0, sizeof(*tlp_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) {
+   to = _log->dw[i];
+   off = where + i * 4;
+   } else {
+   to = _log->prefix[i - 4];
+   off = where2 + (i - 4) * 4;
+   }
+
+   ret = pci_read_config_dword(dev, off, to);
if (ret)
return pcibios_err_to_errno(ret);
}
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ac6293c24976..ecc1dea5a208 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, 
>tlp);
+   pcie_read_tlp_log(dev, aer + PCI_ERR_HEADER_LOG,
+ aer +