On Wed, 11 Dec 2024, Jonathan Cameron wrote:
> On Fri, 13 Sep 2024 17:36:29 +0300
> Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
