Date: Thu, 14 Mar 2019 11:07:50 +0000 From: Johannes Lundberg <johalun@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: cem@freebsd.org, Hans Petter Selasky <hselasky@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r345103 - head/sys/compat/linuxkpi/common/include/linux Message-ID: <0521e2af-03ae-4642-f88c-60d8b8ede968@FreeBSD.org> In-Reply-To: <fcfcbb36-a41d-6dda-0940-1e492d6dc676@FreeBSD.org> References: <201903131915.x2DJFbRk002502@repo.freebsd.org> <CAG6CVpU0feif5Z99By7R93cVnNi-MjCsoGh_t9j3xhqhNR%2B%2BQA@mail.gmail.com> <fcfcbb36-a41d-6dda-0940-1e492d6dc676@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 =3D device_get_parent(dev->dev.bsddev);
>>> + if (root =3D=3D NULL)
>>> + return (PCI_SPEED_UNKNOWN);
>>> + root =3D device_get_parent(root);
>>> + if (root =3D=3D NULL)
>>> + return (PCI_SPEED_UNKNOWN);
>>> + root =3D device_get_parent(root);
>>> + if (root =3D=3D 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 no=
thing else.
> For non-DRM drivers, 'bsddev' is a PCI-e endpoint. You would then go u=
p two layers
> (pciX and pcibX) to get to the parent bridge. However, this is assumin=
g a DRM layout
> where it has to go drm0 -> vgapci0 -> pciX -> pcibX due to the vgapci p=
seudo-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 n=
eeds the
> grandparent. You will have to do something less fragile to find the gr=
andparent.
> The simplest way may be to walk up to the first "pcib" device you see a=
nd 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 vali=
d PCI IVARs
> and PCI config registers you can read). pci_find_pcie_root_port has so=
me similar
> code for this, but that function would walk too far up for what you wan=
t here, so you
> can't reuse it as-is.
>
>>> + if ((error =3D pci_find_cap(root, PCIY_EXPRESS, &pos)) !=3D 0=
)
>>> + return (PCI_SPEED_UNKNOWN);
>> Cached as non-zero cfg.pcie.pcie_location value in ivars.
>>
>>> + lnkcap2 =3D 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 ca=
ched
> 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 regist=
er
> exists instead of reading it unconditionally. Specifically, you should=
read
> the version field (PCIEM_FLAGS_VERSION) from PCIER_FLAGS. LINK_CAP2 on=
ly
> exists if the version >=3D 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 =3D 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0521e2af-03ae-4642-f88c-60d8b8ede968>