From owner-svn-src-head@freebsd.org Wed Mar 13 23:39:46 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 0D01D1544976; Wed, 13 Mar 2019 23:39:46 +0000 (UTC) (envelope-from jhb@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 8B3F27522E; Wed, 13 Mar 2019 23:39:45 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-3.local (ralph.baldwin.cx [66.234.199.215]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 76281CB27; Wed, 13 Mar 2019 23:39:44 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: svn commit: r345103 - head/sys/compat/linuxkpi/common/include/linux To: cem@freebsd.org, Hans Petter Selasky , Johannes Lundberg Cc: src-committers , svn-src-all , svn-src-head References: <201903131915.x2DJFbRk002502@repo.freebsd.org> From: John Baldwin Openpgp: preference=signencrypt Autocrypt: addr=jhb@FreeBSD.org; keydata= mQGiBETQ+XcRBADMFybiq69u+fJRy/0wzqTNS8jFfWaBTs5/OfcV7wWezVmf9sgwn8TW0Dk0 c9MBl0pz+H01dA2ZSGZ5fXlmFIsee1WEzqeJzpiwd/pejPgSzXB9ijbLHZ2/E0jhGBcVy5Yo /Tw5+U/+laeYKu2xb0XPvM0zMNls1ah5OnP9a6Ql6wCgupaoMySb7DXm2LHD1Z9jTsHcAQMD /1jzh2BoHriy/Q2s4KzzjVp/mQO5DSm2z14BvbQRcXU48oAosHA1u3Wrov6LfPY+0U1tG47X 1BGfnQH+rNAaH0livoSBQ0IPI/8WfIW7ub4qV6HYwWKVqkDkqwcpmGNDbz3gfaDht6nsie5Z pcuCcul4M9CW7Md6zzyvktjnbz61BADGDCopfZC4of0Z3Ka0u8Wik6UJOuqShBt1WcFS8ya1 oB4rc4tXfSHyMF63aPUBMxHR5DXeH+EO2edoSwViDMqWk1jTnYza51rbGY+pebLQOVOxAY7k do5Ordl3wklBPMVEPWoZ61SdbcjhHVwaC5zfiskcxj5wwXd2E9qYlBqRg7QeSm9obiBCYWxk d2luIDxqaGJARnJlZUJTRC5vcmc+iGAEExECACAFAkTQ+awCGwMGCwkIBwMCBBUCCAMEFgID AQIeAQIXgAAKCRBy3lIGd+N/BI6RAJ9S97fvbME+3hxzE3JUyUZ6vTewDACdE1stFuSfqMvM jomvZdYxIYyTUpC5Ag0ERND5ghAIAPwsO0B7BL+bz8sLlLoQktGxXwXQfS5cInvL17Dsgnr3 1AKa94j9EnXQyPEj7u0d+LmEe6CGEGDh1OcGFTMVrof2ZzkSy4+FkZwMKJpTiqeaShMh+Goj XlwIMDxyADYvBIg3eN5YdFKaPQpfgSqhT+7El7w+wSZZD8pPQuLAnie5iz9C8iKy4/cMSOrH YUK/tO+Nhw8Jjlw94Ik0T80iEhI2t+XBVjwdfjbq3HrJ0ehqdBwukyeJRYKmbn298KOFQVHO EVbHA4rF/37jzaMadK43FgJ0SAhPPF5l4l89z5oPu0b/+5e2inA3b8J3iGZxywjM+Csq1tqz hltEc7Q+E08AAwUIAL+15XH8bPbjNJdVyg2CMl10JNW2wWg2Q6qdljeaRqeR6zFus7EZTwtX sNzs5bP8y51PSUDJbeiy2RNCNKWFMndM22TZnk3GNG45nQd4OwYK0RZVrikalmJY5Q6m7Z16 4yrZgIXFdKj2t8F+x613/SJW1lIr9/bDp4U9tw0V1g3l2dFtD3p3ZrQ3hpoDtoK70ioIAjjH aIXIAcm3FGZFXy503DOA0KaTWwvOVdYCFLm3zWuSOmrX/GsEc7ovasOWwjPn878qVjbUKWwx Q4QkF4OhUV9zPtf9tDSAZ3x7QSwoKbCoRCZ/xbyTUPyQ1VvNy/mYrBcYlzHodsaqUDjHuW+I SQQYEQIACQUCRND5ggIbDAAKCRBy3lIGd+N/BCO8AJ9j1dWVQWxw/YdTbEyrRKOY8YZNwwCf afMAg8QvmOWnHx3wl8WslCaXaE8= Message-ID: Date: Wed, 13 Mar 2019 16:39:42 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:60.0) Gecko/20100101 Thunderbird/60.5.3 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 8B3F27522E X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.89 / 15.00]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.89)[-0.885,0]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; 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: Wed, 13 Mar 2019 23:39:46 -0000 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 = 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