Message ID | 20230929115723.7864-7-ilpo.jarvinen@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp3978125vqu; Fri, 29 Sep 2023 05:12:18 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF3fhXDSGNiib2yIvV/NFWIih31TlWDkeCrsZgDnrnGDYMjbYrwcoHvPfEGmYOVVaD/Ngh5 X-Received: by 2002:a05:6a21:7746:b0:14e:b4d5:782e with SMTP id bc6-20020a056a21774600b0014eb4d5782emr3230432pzc.29.1695989537903; Fri, 29 Sep 2023 05:12:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695989537; cv=none; d=google.com; s=arc-20160816; b=SWjoDJZBLVQngJMXRfjN56Z6/dljzQ5kKO6fNDMZUjso1IhNJ3RllUSfdNt7t9w9Hc bDTSIxmn8KhmHO1lgDGo8ivmxd4O8XuGyFtLqKig9SR7cT7Y4YwRKBU1uK+J750f7VUM ENTFijed/uxTuLcg/iliQNC7UCpHzJrMZL05oLKn9ISs5jTfAkli8VyVRfBWKLg0lld+ +P16x9UGq1tkqvMoMw0ivtS+Qd18grWyqQ0avHcCymvlWnQihw2CC8Sdc9awnfujO9tv s/PRLKqNIRrSZOKhio5jcHWsTZhbMBS+e1/Hitx12/JdWFbtOXPMsZyWARh0wRJeuJ0Q kClg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=YnYbGtxlEyv5gg06gqhhjGtZbz2oF6HoowlGwJBpkrA=; fh=fw9CINBXChmOzbpG8z130R6wLR+N0uJHzfDIs0RDLlQ=; b=st+qH6SPmMRNlihs1veF/zuUChgOKXmV3qu+TMWDC8qqGU0pkYUmJGPjkIQLl5Zgwb cfggU2uZt/zGvqejsDBnuwwOMINH0nkMDVdhwaCb/ZW6uvFx4B79vEp7CL7T08tKAeL/ cJPtYNhLONLg04vG/GdW3LJuJr9M8kYrCQQEn0xgWxZoJIfk4SiFPY3sgPsXu9yCvbKd Am+Y3+jt2rnv6tvJbjf3IyDv+nRkCLN2kMPlsMc4sAxQfkiIt2BWpo1K/T3vMnFdF7cd i30Ru+NfinsHu222LleSdSDqBQwW64v/eCAR67CDHJmAWz8RiauIGCiU7pmEhvVqLUHx 8LaQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KtarcQXN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from groat.vger.email (groat.vger.email. [2620:137:e000::3:5]) by mx.google.com with ESMTPS id z7-20020a056a001d8700b0068e26ca7f00si20142109pfw.39.2023.09.29.05.12.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Sep 2023 05:12:17 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=KtarcQXN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 5092A838192C; Fri, 29 Sep 2023 04:59:42 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233151AbjI2L7K (ORCPT <rfc822;pwkd43@gmail.com> + 20 others); Fri, 29 Sep 2023 07:59:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55140 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233214AbjI2L7G (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 29 Sep 2023 07:59:06 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 726CF1B0; Fri, 29 Sep 2023 04:59:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1695988743; x=1727524743; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=5GI9De0NedLS8Fb+1iCt8QAfte6ia8Vm/9fJLfKqyBE=; b=KtarcQXNP/o65oNQiIK/akayDJM7rPqashxAuwRz1JWkR2MqiisLCzQ1 MKxFkVV+bzsSeqZgMrF1nYS7QJqVy0m01v+UPZ3eDFzI239PYue681jTM zTsHMcLPiLFZsKfxoDkROuE3Arkzi/VBJwVyrFE5slkSNOX2Ra3+0Jnb+ uVu8o7oa8tUB31Py7t9OaLavF1rcBBfXjg+qVVHiM1svY4z3nB9YDtpeh ejDapAvBN6fw0YJbMOW8vRP9KYYjd0oXlk3rGFgKs537MfiMWLEJNAmJj CRt30mLtqa95Q3Y4jpnvLd80Qgga+Y0lWn4bPof3syC9kPEvp+fUNk1cI g==; X-IronPort-AV: E=McAfee;i="6600,9927,10847"; a="362528079" X-IronPort-AV: E=Sophos;i="6.03,187,1694761200"; d="scan'208";a="362528079" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2023 04:59:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10847"; a="815581136" X-IronPort-AV: E=Sophos;i="6.03,187,1694761200"; d="scan'208";a="815581136" Received: from valeks2x-mobl.ger.corp.intel.com (HELO localhost) ([10.252.53.242]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2023 04:58:54 -0700 From: =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com> To: linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Rob Herring <robh@kernel.org>, =?utf-8?q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>, Lukas Wunner <lukas@wunner.de>, Alexandru Gagniuc <mr.nuke.me@gmail.com>, Krishna chaitanya chundru <quic_krichai@quicinc.com>, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>, linux-kernel@vger.kernel.org Cc: Alex Deucher <alexdeucher@gmail.com>, Daniel Lezcano <daniel.lezcano@linaro.org>, Amit Kucheria <amitk@kernel.org>, Zhang Rui <rui.zhang@intel.com>, =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com> Subject: [PATCH v3 06/10] PCI: Cache PCIe device's Supported Speed Vector Date: Fri, 29 Sep 2023 14:57:19 +0300 Message-Id: <20230929115723.7864-7-ilpo.jarvinen@linux.intel.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230929115723.7864-1-ilpo.jarvinen@linux.intel.com> References: <20230929115723.7864-1-ilpo.jarvinen@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Fri, 29 Sep 2023 04:59:42 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778373925795331989 X-GMAIL-MSGID: 1778373925795331989 |
Series |
Add PCIe Bandwidth Controller
|
|
Commit Message
Ilpo Järvinen
Sept. 29, 2023, 11:57 a.m. UTC
The Supported Link Speeds Vector in the Link Capabilities Register 2
corresponds to the bus below on Root Ports and Downstream Ports,
whereas it corresponds to the bus above on Upstream Ports and
Endpoints. Only the former is currently cached in pcie_bus_speeds in
the struct pci_bus. The link speeds that are supported is the
intersection of these two.
Store the device's Supported Link Speeds Vector into the struct pci_bus
when the Function 0 is enumerated (the Multi-Function Devices must have
same speeds the same for all Functions) to be easily able to calculate
the intersection of Supported Link Speeds.
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
drivers/pci/probe.c | 10 ++++++++++
drivers/pci/remove.c | 2 ++
include/linux/pci.h | 1 +
3 files changed, 13 insertions(+)
Comments
On Fri, Sep 29, 2023 at 02:57:19PM +0300, Ilpo Järvinen wrote: > The Supported Link Speeds Vector in the Link Capabilities Register 2 > corresponds to the bus below on Root Ports and Downstream Ports, > whereas it corresponds to the bus above on Upstream Ports and > Endpoints. It would be good to add a pointer to the spec here. I think the relevant section is PCIe r6.1 sec 7.5.3.18 which says: "Supported Link Speeds Vector - This field indicates the supported Link speed(s) of the associated Port." ^^^^^^^^^^^^^^^ Obviously the associated port is upstream on a Switch Upstream Port or Endpoint, whereas it is downstream on a Switch Downstream Port or Root Port. Come to think of it, what about edge cases such as RCiEPs? > Only the former is currently cached in pcie_bus_speeds in > the struct pci_bus. The link speeds that are supported is the > intersection of these two. I'm wondering if caching both is actually necessary. Why not cache just the intersection? Do we need either of the two somewhere? > Store the device's Supported Link Speeds Vector into the struct pci_bus > when the Function 0 is enumerated (the Multi-Function Devices must have > same speeds the same for all Functions) to be easily able to calculate > the intersection of Supported Link Speeds. Might want to add an explanation what you're going to need this for, I assume it's accessed frequently by the bandwidth throttling driver in a subsequent patch? Thanks, Lukas
On Sat, 30 Dec 2023, Lukas Wunner wrote: > On Fri, Sep 29, 2023 at 02:57:19PM +0300, Ilpo Järvinen wrote: > > The Supported Link Speeds Vector in the Link Capabilities Register 2 > > corresponds to the bus below on Root Ports and Downstream Ports, > > whereas it corresponds to the bus above on Upstream Ports and > > Endpoints. > > It would be good to add a pointer to the spec here. I think the > relevant section is PCIe r6.1 sec 7.5.3.18 which says: > > "Supported Link Speeds Vector - This field indicates the supported > Link speed(s) of the associated Port." > ^^^^^^^^^^^^^^^ > > Obviously the associated port is upstream on a Switch Upstream Port > or Endpoint, whereas it is downstream on a Switch Downstream Port > or Root Port. > > Come to think of it, what about edge cases such as RCiEPs? On real HW I've seen, RCiEPs don't seem to have these speeds at all (PCIe r6.1, sec 7.5.3): "The Link Capabilities, Link Status, and Link Control registers are required for all Root Ports, Switch Ports, Bridges, and Endpoints that are not RCiEPs. For Functions that do not implement the Link Capabilities, Link Status, and Link Contro registers, these spaces must be hardwired to 0. Link Capabilities 2, Link Status 2, and Link Control 2 registers are required for all Root Ports, Switch Ports, Bridges, and Endpoints (except for RCiEPs) that implement capabilities requiring those registers. For Functions that do not implement the Link Capabilities 2, Link Status 2, and Link Control 2 registers, these spaces must be hardwired to 0b." > > Only the former is currently cached in pcie_bus_speeds in > > the struct pci_bus. The link speeds that are supported is the > > intersection of these two. > > I'm wondering if caching both is actually necessary. Why not cache > just the intersection? Do we need either of the two somewhere? Intersection is enough at least for bwctrl. The only downside that is barely worth mentioning is that the bus SLSV has to be re-read when function 0 sets the intersection. I can think of somebody wanting to expose the list of both supported speed to userspace though sysfs (not done by this patch series), but they could be read from the registers in that case so that use case doesn't really matter much, IMO. > > Store the device's Supported Link Speeds Vector into the struct pci_bus > > when the Function 0 is enumerated (the Multi-Function Devices must have > > same speeds the same for all Functions) to be easily able to calculate > > the intersection of Supported Link Speeds. > > Might want to add an explanation what you're going to need this for, > I assume it's accessed frequently by the bandwidth throttling driver > in a subsequent patch? Yes. I tend to try to avoid forward references because some maintainers complain about them (leading to minimal changes where true motivations have to be hidden because "future" cannot be used to motivate a change even if that's often the truest motivation within a patch series). But I'll add a fwd ref here to make it more obvious. :-)
On Mon, Jan 01, 2024 at 08:31:39PM +0200, Ilpo Järvinen wrote: > On Sat, 30 Dec 2023, Lukas Wunner wrote: > > On Fri, Sep 29, 2023 at 02:57:19PM +0300, Ilpo Järvinen wrote: > > > Only the former is currently cached in pcie_bus_speeds in > > > the struct pci_bus. The link speeds that are supported is the > > > intersection of these two. > > > > I'm wondering if caching both is actually necessary. Why not cache > > just the intersection? Do we need either of the two somewhere? > > Intersection is enough at least for bwctrl. The only downside that is > barely worth mentioning is that the bus SLSV has to be re-read when > function 0 sets the intersection. > > I can think of somebody wanting to expose the list of both supported speed > to userspace though sysfs (not done by this patch series), but they could > be read from the registers in that case so that use case doesn't really > matter much, IMO. Yes, that would be a reasonable argument to keep both values instead of storing just the intersection. > > > Store the device's Supported Link Speeds Vector into the struct pci_bus > > > when the Function 0 is enumerated (the Multi-Function Devices must have > > > same speeds the same for all Functions) to be easily able to calculate > > > the intersection of Supported Link Speeds. > > > > Might want to add an explanation what you're going to need this for, > > I assume it's accessed frequently by the bandwidth throttling driver > > in a subsequent patch? > > Yes. I tend to try to avoid forward references because some maintainers > complain about them (leading to minimal changes where true motivations > have to be hidden because "future" cannot be used to motivate a change > even if that's often the truest motivation within a patch series). But > I'll add a fwd ref here to make it more obvious. :-) Bjorn has used phrases such as "We're about to ..." a couple of times in commit messages to convey that a particular change in the present patch will be taken advantage of by a subsequent patch. I've used the same phrase but got criticized (in other subsystems) for using "we". So I use phrases such as: "An upcoming commit will create DOE mailboxes upon device enumeration by the PCI core. Their lifetime shall not be limited by a driver. Therefore rework..." (see 022b66f38195) Can also reference the past: "The PCI core has just been amended to create a pci_doe_mb struct for every DOE instance on device enumeration. [...] That leaves [...] without any callers, so drop them." (see 74e491e5d1bc) If someone finds your commit e.g. through git blame, it may help them enormously if you provide context in the commit message. If maintainers in other subsystem tell you otherwise, they're wrong. ;) Thanks, Lukas
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ca1d797a30cb..a9408f2420e5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2564,6 +2564,7 @@ static void pci_set_msi_domain(struct pci_dev *dev) void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) { + u8 dev_speeds = 0; int ret; pci_configure_device(dev); @@ -2590,11 +2591,20 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) pci_init_capabilities(dev); + if (pci_is_pcie(dev) && PCI_FUNC(dev->devfn) == 0) { + u32 linkcap, linkcap2; + + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap); + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &linkcap2); + dev_speeds = pcie_get_supported_speeds(linkcap, linkcap2); + } /* * Add the device to our list of discovered devices * and the bus list for fixup functions, etc. */ down_write(&pci_bus_sem); + if (dev_speeds) + bus->pcie_dev_speeds = dev_speeds; list_add_tail(&dev->bus_list, &bus->devices); up_write(&pci_bus_sem); diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index d749ea8250d6..656784cfb291 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -36,6 +36,8 @@ static void pci_destroy_dev(struct pci_dev *dev) device_del(&dev->dev); down_write(&pci_bus_sem); + if (pci_is_pcie(dev) && PCI_FUNC(dev->devfn) == 0) + dev->bus->pcie_dev_speeds = 0; list_del(&dev->bus_list); up_write(&pci_bus_sem); diff --git a/include/linux/pci.h b/include/linux/pci.h index cb03f3ff9d23..b8bd3dc92032 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -665,6 +665,7 @@ struct pci_bus { unsigned char max_bus_speed; /* enum pci_bus_speed */ unsigned char cur_bus_speed; /* enum pci_bus_speed */ u8 pcie_bus_speeds;/* Supported Link Speeds Vector (+ reserved 0 at LSB) */ + u8 pcie_dev_speeds;/* Device's Supported Link Speeds Vector (+ 0 at LSB) */ #ifdef CONFIG_PCI_DOMAINS_GENERIC int domain_nr; #endif