From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_PASS,T_DKIMWL_WL_HIGH,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18CE7C46471 for ; Tue, 7 Aug 2018 21:41:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B3ACE21945 for ; Tue, 7 Aug 2018 21:41:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="E29vM/L3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B3ACE21945 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726990AbeHGX6H (ORCPT ); Tue, 7 Aug 2018 19:58:07 -0400 Received: from mail.kernel.org ([198.145.29.99]:53626 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726494AbeHGX6H (ORCPT ); Tue, 7 Aug 2018 19:58:07 -0400 Received: from localhost (unknown [69.71.4.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A00B92168C; Tue, 7 Aug 2018 21:41:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1533678103; bh=EnYpABeqf7uPYhwXAQV59kGNh4lDsdgKzcLRArnDB/8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=E29vM/L3GTQhCHiSoltIQTLNJm8szLi6txxpC+nr6Cu9HEUYuUuV76Fw918etixeL Tu6CSRHi66/Yc56qZ4drgfeURoC5afIp9sfH04LvZrZpuaeVZFbnVqmjUlUAK9daYY uR3ln9wPAiQgXtL5BtChHUGT5+D1eaZ1iOgrn+Kw= Date: Tue, 7 Aug 2018 16:41:41 -0500 From: Bjorn Helgaas To: Alexandru Gagniuc Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, jakub.kicinski@netronome.com, keith.busch@intel.com, alex_gagniuc@dellteam.com, austin_bolen@dell.com, shyam_iyer@dell.com, Ariel Elior , everest-linux-l2@cavium.com, "David S. Miller" , Michael Chan , Ganesh Goudar , Jeff Kirsher , Tariq Toukan , Saeed Mahameed , Leon Romanovsky , Dirk van der Merwe , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com Subject: Re: [PATCH v6 1/9] PCI: Check for PCIe downtraining conditions Message-ID: <20180807214141.GA49411@bhelgaas-glaptop.roam.corp.google.com> References: <20180806232600.25694-1-mr.nuke.me@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180806232600.25694-1-mr.nuke.me@gmail.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 06, 2018 at 06:25:35PM -0500, Alexandru Gagniuc wrote: > PCIe downtraining happens when both the device and PCIe port are > capable of a larger bus width or higher speed than negotiated. > Downtraining might be indicative of other problems in the system, and > identifying this from userspace is neither intuitive, nor > straightforward. > > The easiest way to detect this is with pcie_print_link_status(), > since the bottleneck is usually the link that is downtrained. It's not > a perfect solution, but it works extremely well in most cases. After this series, there are no callers of pcie_print_link_status(), which means we *only* print something if a device is capable of more bandwidth than the fabric can deliver. ISTR some desire to have this information for NICs even if the device isn't limited, so I'm just double-checking to make sure the driver guys are OK with this change. There are no callers of __pcie_print_link_status() outside the PCI core, so I would move the declaration from include/linux/pci.h to drivers/pci/pci.h. If we agree that we *never* need to print anything unless a device is constrained by the fabric, I would get rid of the "verbose" flag and keep everything in pcie_print_link_status(). > Signed-off-by: Alexandru Gagniuc > --- > drivers/pci/pci.c | 22 ++++++++++++++++++---- > drivers/pci/probe.c | 21 +++++++++++++++++++++ > include/linux/pci.h | 1 + > 3 files changed, 40 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 316496e99da9..414ad7b3abdb 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5302,14 +5302,15 @@ u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed, > } > > /** > - * pcie_print_link_status - Report the PCI device's link speed and width > + * __pcie_print_link_status - Report the PCI device's link speed and width > * @dev: PCI device to query > + * @verbose: Be verbose -- print info even when enough bandwidth is available. > * > * Report the available bandwidth at the device. If this is less than the > * device is capable of, report the device's maximum possible bandwidth and > * the upstream link that limits its performance to less than that. > */ > -void pcie_print_link_status(struct pci_dev *dev) > +void __pcie_print_link_status(struct pci_dev *dev, bool verbose) > { > enum pcie_link_width width, width_cap; > enum pci_bus_speed speed, speed_cap; > @@ -5319,11 +5320,11 @@ void pcie_print_link_status(struct pci_dev *dev) > bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap); > bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width); > > - if (bw_avail >= bw_cap) > + if (bw_avail >= bw_cap && verbose) > pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n", > bw_cap / 1000, bw_cap % 1000, > PCIE_SPEED2STR(speed_cap), width_cap); > - else > + else if (bw_avail < bw_cap) > pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n", > bw_avail / 1000, bw_avail % 1000, > PCIE_SPEED2STR(speed), width, > @@ -5331,6 +5332,19 @@ void pcie_print_link_status(struct pci_dev *dev) > bw_cap / 1000, bw_cap % 1000, > PCIE_SPEED2STR(speed_cap), width_cap); > } > + > +/** > + * pcie_print_link_status - Report the PCI device's link speed and width > + * @dev: PCI device to query > + * > + * Report the available bandwidth at the device. If this is less than the > + * device is capable of, report the device's maximum possible bandwidth and > + * the upstream link that limits its performance to less than that. > + */ > +void pcie_print_link_status(struct pci_dev *dev) > +{ > + __pcie_print_link_status(dev, true); > +} > EXPORT_SYMBOL(pcie_print_link_status); > > /** > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 611adcd9c169..1c8c26dd2cb2 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2205,6 +2205,24 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn) > return dev; > } > > +static void pcie_check_upstream_link(struct pci_dev *dev) > +{ > + if (!pci_is_pcie(dev)) > + return; > + > + /* Look from the device up to avoid downstream ports with no devices. */ > + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT) && > + (pci_pcie_type(dev) != PCI_EXP_TYPE_LEG_END) && > + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM)) > + return; > + > + /* Multi-function PCIe share the same link/status. */ > + if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) > + return; > + > + __pcie_print_link_status(dev, false); > +} > + > static void pci_init_capabilities(struct pci_dev *dev) > { > /* Enhanced Allocation */ > @@ -2240,6 +2258,9 @@ static void pci_init_capabilities(struct pci_dev *dev) > /* Advanced Error Reporting */ > pci_aer_init(dev); > > + /* Check link and detect downtrain errors */ > + pcie_check_upstream_link(dev); > + > if (pci_probe_reset_function(dev) == 0) > dev->reset_fn = 1; > } > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c133ccfa002e..d212de231259 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1087,6 +1087,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps); > u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, > enum pci_bus_speed *speed, > enum pcie_link_width *width); > +void __pcie_print_link_status(struct pci_dev *dev, bool verbose); > void pcie_print_link_status(struct pci_dev *dev); > int pcie_flr(struct pci_dev *dev); > int __pci_reset_function_locked(struct pci_dev *dev); > -- > 2.17.1 >