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

[Johannes Lundberg]

On 3/13/19 11:39 PM, John Baldwin wrote:
> 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.

Hi

Thanks for the feedback.

This code used to exist in the drm driver but was moved into kernel pci
code on Linux. It's more or less as it existed in the drm driver with
FreeBSD patches having roots probably going back to drm2 from base. If
this function won't be compatible with anything but drm drivers, we
could just as well move it to linuxkpi in ports instead.

I'll try to implement the suggested improvements. Keep on eye out for
updates here https://reviews.freebsd.org/D19565

/Johannes