From owner-svn-src-head@freebsd.org Thu Mar 14 11:07:54 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 85820153D98D; Thu, 14 Mar 2019 11:07:54 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0938B71FF8; Thu, 14 Mar 2019 11:07:54 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Received: from [192.168.1.33] (unknown [81.174.250.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: johalun) by smtp.freebsd.org (Postfix) with ESMTPSA id 3A41711620; Thu, 14 Mar 2019 11:07:53 +0000 (UTC) (envelope-from johalun@FreeBSD.org) Subject: Re: svn commit: r345103 - head/sys/compat/linuxkpi/common/include/linux To: John Baldwin Cc: cem@freebsd.org, Hans Petter Selasky , src-committers , svn-src-all , svn-src-head References: <201903131915.x2DJFbRk002502@repo.freebsd.org> From: Johannes Lundberg Openpgp: preference=signencrypt Autocrypt: addr=johalun0@gmail.com; keydata= mQINBFxFmoIBEADoFO5jY+Fmsg44KiZjufEmpEf4kt7nCOfxNG9SruWpoXUaq0B296F+fIZC hNZqv1v7lGTsfoWRusxJmLd5CQgHHxEyruZbbPpNsQ/JKoDY3GGmrmWfN/SX3y0t0kdB9HsW mJcvZhK7we52f4gxddIVBS9nQoVoONX+hzXf8zwOAa0ik0EPgEwpIKS4j9lLq4bU+mqVKdRR bPeDujEA/qbsCKhaFJkPzXZtzEe6srq4RK1doEztwnKz02b+8gs642TRkWDQeTRZputrAaoN Un4R76A1QpXWyrFG1dQu48IGHi3KbkrvNyq6R1aUBIA0+CG1npIbxmc2mtSjoyvdipmDRbBD +mhECIxmYfBT6818zuj91XjrfOyfVdV2BryBvqFkJLkS3N3QElBIiVdDgdrqiNFWiOlDMxNI tdP16oQBNo8IB27/0YHpnQEw1MafZv5gG5DO0zLtLy88ASAfL7BYf90JP19rT4JIwnxsXxyv kEJnzhsXf0QVObEiAu1MqeFyWfZ8PpunmvEmJ0VChOL+v/kIx1E9cxhhzMZhqiMXfyM4zx2+ BF1FwAwJYPuJLu2B3L0uVBu+M1YvSOmKAbXPDP8PsqPjgSBTYI51MUjuuxN6jSsHDuK6G5k4 pUWR8axa+wafhd6Vz8zVwdTJZ9LdxgLLVg0kprBgccPHhPAZVQARAQABtCZKb2hhbm5lcyBM dW5kYmVyZyA8am9oYWx1bjBAZ21haWwuY29tPokCVAQTAQgAPhYhBIl1Pb3+hI60ivmRSULn yG4BGvSeBQJcRZqCAhsjBQkJZgGABQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEELnyG4B GvSe9O0P/RzeQAu1R37RlONZTXNn+qIAHvHbZEhzrCibzaZnwYdC31wGrYmXNDyiQIqOngFf QJuufQtH/+95OESJsjR+42L/pNfFdaEWxiI003qE7uCMzLK5UWUXd/5d5vYY0CaPyNCj1tyM ZIq7x4CaR3QLTh/Fw4zMUI/ZPH2S5SxVFGv0ZZFAdNYILD3qCkAS/9HmXsqufBWbfutA8TTf wyJfywmvf7ENjlZ4QOjb242ZY9NndqbmqTgWVAws+PN5e9AT8HkadscCTCSkYnxJyYG2El27 DpAAkekYplb/C0j82KSz2fy9RgwD+tTqt88DJOeFbIbrYt44u7KLHpzaZeqyUtn0reHCkE0W lnKH2kXXbuswFB4sONxI/J5+qSmOsAm5ItO3voyjm/swpmFR1yBlxo4th26gbO5NfBOK9YsY zHKgiRDv6ZdnHo+htphRxcCDHsFPzkQe5jouI25dvMZYl1LaTS/09lwYVwVIB2SFmMtFZ7rB N4NBSzPlpsg+g4dJNqiw6Rfa2Q/wUv+MzTJgLtHjDccXlpm33Nc09UytHFtNn26PO/zrM39r TwzdLu1mg0x2WWEWTIqe4CaczQU9SIg49BSyJNoPSZx3V7nMhTKbOeQKR5aV3dXI66aENw86 pa1tipuUKCPmope/GTJatUgPiD3JkyiD+7c1zQX2UAGmuQINBFxFmoIBEACb55RAkM59huAx 4Ddd8WBjsw25qf7rzxeRKAQ7or/8LvJBYQDPXZy0RhkRiu+P+MjxwGb6HVh+LDyAYDn9d8Mt ZqCP/dOGNcl7pkb6IhfRc3i5neckXCYfbm0cigiX9JkqZSt3KT96zbjCxsFZKyIyEFsMl46q 7wKWK5Irj3zxV/Z51JNTJyMLcIRWhY8G6qlMNFgZkz2Hv63w6BRekKVImOmOdThLAscy5ybq 2CIUeAwPG7lMYG9rgcPdn3tMPeWlLmUmi5pSwOQ3AKg3xFrW3WfegjRHdqpeuXoeTjYPPCW4 gyl59uv6E12a6eivItCxj67vlBXgOr4um+zoPyXG/WfidIFtWaEgyBrlGR1Klk7SIcqjEHUA FdiM+PweY4opHXXKn60NOZCqBJ59K43drOQgRouz8E2T3yEoYg40xAfY3lhJV/Vx5+kSTjmy sT2xotlPn/GzfaAEvNuJDK+Mec3LvfbbDoOWFolNyEvoMQqF5Q3A8eGqYsoVGBPxyzNvF2iY LkymxiXpgrSN0Q/LOK7pFlWwbVC8Z6g5I0J9ecgD55dGLoX2luLir787XX/JxGffzbRnP9NE ifenJGrQmx4CyEaz/CHQqSbROm5Uo/YFUX9J7OfUO4mtu90j773j32I3psey/Fz3EC/A2PHv Ghb0KsWYpS3Pj5TV1gGyswARAQABiQI8BBgBCAAmFiEEiXU9vf6EjrSK+ZFJQufIbgEa9J4F AlxFmoICGwwFCQlmAYAACgkQQufIbgEa9J7qOQ//YG/4e69YTSjtiYLXzBI8tRU2Sx+NFByx zx+C/r0EBThLtgRwCqEUZRB7iIDSO8aZ0Qa3vwWRohlD1tn/LBdDFfMmuQkNVdLIrjBoGBB9 B5xHdZJ9xnTZEwpTtk6IWolT4j+8rpGemGKKiFo3X6l02On4Qb4iM7h6rcDb76mfwooNYzB3 8PPcLvyOWb/9iCXAb5N7doo5zmOl15DVwvIF04eXU0q1FFj/iS1zNmtZ5Got82O1TQFV+de4 Rb3YA80IZhhhCiHHJqkMKeKQogRqU+UNDBARUBxfUtKsJtQzTQ2JUGwkb6X6bx53FTLP6O9q hDoODVweE1LdB1k1H5Nn+gawPdRMBqj43Y2amK7KEgoTBrwU04CLpKiaAC0S+EcJFfJcwtpK k3F+uTtP/hnhFnWbn8SgRkHKXKWqSCt63NstXhMzAJut1gEzV+CcPNKqa/sFgQaYEvzCS5Kl F/PXj0++f3TIFqT+2ZNNp8Bz8dT7gh8RPPg5oYQiCHH8K1RAmq7gKqmwyg0qgOazHnped+od X4f3qx320JAP6NP9wglDm6eht48NJzb0sffN8z34wrP66oz8oPKtS5CFV0m/384hEg0lmi3W wo2Hno7rA1etTPJX0dI6/GLlQDtNTHvKQ077HQdWVOMQVWC9j7YH7Zr9NjtOvxcNVRX3fxpJ 6CE= X-Tagtoolbar-Keys: D20190314110750797 Message-ID: <0521e2af-03ae-4642-f88c-60d8b8ede968@FreeBSD.org> Date: Thu, 14 Mar 2019 11:07:50 +0000 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US X-Rspamd-Queue-Id: 0938B71FF8 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.96 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.96)[-0.961,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Mar 2019 11:07:54 -0000 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 >> 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