svn commit: r345103 - head/sys/compat/linuxkpi/common/include/linux

[John Baldwin]

On 3/13/19 1:36 PM, Conrad Meyer wrote:
> Hi,
> 
> A lot of the information about PCIe devices is read by PCI probe and
> cached on the (BSD) device.  You could access it out of
> device_get_ivars(bsddev)->cfg.pcie and avoid the MMIO latency.

Agreed when possible, though I'm not sure caching lnkcap is really
all that useful.

> On Wed, Mar 13, 2019 at 12:15 PM Hans Petter Selasky
> <hselasky at freebsd.org> wrote:
>> +static inline enum pci_bus_speed
>> +pcie_get_speed_cap(struct pci_dev *dev)
>> +{
>> +       device_t root;
>> +       uint32_t lnkcap, lnkcap2;
>> +       int error, pos;
>> +
>> +       root = device_get_parent(dev->dev.bsddev);
>> +       if (root == NULL)
>> +               return (PCI_SPEED_UNKNOWN);
>> +       root = device_get_parent(root);
>> +       if (root == NULL)
>> +               return (PCI_SPEED_UNKNOWN);
>> +       root = device_get_parent(root);
>> +       if (root == NULL)
>> +               return (PCI_SPEED_UNKNOWN);
> 
> What is this mechanism trying to accomplish?  It seems incredibly
> fragile.  Looking for pci0? pcib0?

It does seem broken, or maybe that it only works for DRM drivers and nothing else.
For non-DRM drivers, 'bsddev' is a PCI-e endpoint.  You would then go up two layers
(pciX and pcibX) to get to the parent bridge.  However, this is assuming a DRM layout
where it has to go drm0 -> vgapci0 -> pciX -> pcibX due to the vgapci pseudo-driver.
As a result, this function can't possibly work on anything other than a DRM driver.
Since it would try to use pci ivars on some PCI bus instance (or worse), it's likely
to cause random panics if used from a non-DRM driver.

Furthermore, it's not at all clear to me why you can't just read lnkcap/lnkcap2 from
the endpoint device directly vs heading up to the parent bridge though.  Hmm, I guess
it's trying to infer the capabilities of the "slot", so that's why it needs the
grandparent.  You will have to do something less fragile to find the grandparent.
The simplest way may be to walk up to the first "pcib" device you see and then
stop.  You will still want to see if it's a "real" PCI device by seeing if it's
parent is a "pci" device (meaning that the "pcib" device will have valid PCI IVARs
and PCI config registers you can read).  pci_find_pcie_root_port has some similar
code for this, but that function would walk too far up for what you want here, so you
can't reuse it as-is.

>> +       if ((error = pci_find_cap(root, PCIY_EXPRESS, &pos)) != 0)
>> +               return (PCI_SPEED_UNKNOWN);
> 
> Cached as non-zero cfg.pcie.pcie_location value in ivars.
> 
>> +       lnkcap2 = pci_read_config(root, pos + PCIER_LINK_CAP2, 4);
> 
> This one we don't cache, unfortunately, but could.  Ditto LINK_CAP
> below.  Usually "pos + PCIEfoo" is spelled "pcie_read_config(...,
> PCIEfoo...)."

Yes, pcie_read_config is what you normally would use.  That uses the cached
pcie_location for you.

     pcie_read_config(dev->dev.bsddev, PCIER_LINK_CAP2, 2);

But also, you should be checking the PCIe version to see if this register
exists instead of reading it unconditionally.  Specifically, you should read
the version field (PCIEM_FLAGS_VERSION) from PCIER_FLAGS.  LINK_CAP2 only
exists if the version >= 2.

>> +
>> +       if (lnkcap2) {  /* PCIe r3.0-compliant */
>> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_2_5GB)
>> +                       return (PCIE_SPEED_2_5GT);
> 
> Seems like these definitions would be better suited as native
> PCIEM_LINK_CAP2_foo definitions in pcireg.h

I feel less strongly about those since we have to provide the constants
anyway for consumers to use.

>> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_5_0GB)
>> +                       return (PCIE_SPEED_5_0GT);
>> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_8_0GB)
>> +                       return (PCIE_SPEED_8_0GT);
>> +               if (lnkcap2 & PCI_EXP_LNKCAP2_SLS_16_0GB)
>> +                       return (PCIE_SPEED_16_0GT);
>> +       } else {        /* pre-r3.0 */
>> +               lnkcap = pci_read_config(root, pos + PCIER_LINK_CAP, 4);
>> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
>> +                       return (PCIE_SPEED_2_5GT);
>> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
>> +                       return (PCIE_SPEED_5_0GT);
>> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
>> +                       return (PCIE_SPEED_8_0GT);
>> +               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
>> +                       return (PCIE_SPEED_16_0GT);
>> +       }
>> +       return (PCI_SPEED_UNKNOWN);
>> +}
>> +
>> +static inline enum pcie_link_width
>> +pcie_get_width_cap(struct pci_dev *dev)
>> +{
>> +       uint32_t lnkcap;
>> +
>> +       pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> 
> Better spelled as PCIER_LINK_CAP.
> 
>> +       if (lnkcap)
>> +               return ((lnkcap & PCI_EXP_LNKCAP_MLW) >> 4);
> 
> And PCIEM_LINK_CAP_MAX_WIDTH.

Given that this is using a linuxkpi API (pcie_capability_read_dword
instead of pcie_read_config()) then the rest of this function is
written in the Linux KPI, so I think using Linux' native constants
is actually more consistent for this function.

-- 
John Baldwin