From: Bjorn Helgaas <helgaas@kernel.org>
To: "Derrick, Jonathan" <jonathan.derrick@intel.com>
Cc: "wangxiongfeng2@huawei.com" <wangxiongfeng2@huawei.com>,
"kw@linux.com" <kw@linux.com>,
"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
"kai.heng.feng@canonical.com" <kai.heng.feng@canonical.com>,
"refactormyself@gmail.com" <refactormyself@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mika.westerberg@linux.intel.com"
<mika.westerberg@linux.intel.com>,
"Mario.Limonciello@dell.com" <Mario.Limonciello@dell.com>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"bhelgaas@google.com" <bhelgaas@google.com>,
"Wysocki, Rafael J" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH] PCI/ASPM: Enable ASPM for links under VMD domain
Date: Thu, 17 Sep 2020 12:20:10 -0500 [thread overview]
Message-ID: <20200917172010.GA1710481@bjorn-Precision-5520> (raw)
In-Reply-To: <4db0fbba635cd1ff5a3c1529d3c7fa08d0729756.camel@intel.com>
On Thu, Sep 10, 2020 at 07:51:05PM +0000, Derrick, Jonathan wrote:
> On Thu, 2020-09-10 at 14:17 -0500, Bjorn Helgaas wrote:
> > On Thu, Sep 10, 2020 at 06:52:48PM +0000, Derrick, Jonathan wrote:
> > > On Thu, 2020-09-10 at 12:38 -0500, Bjorn Helgaas wrote:
> > > > On Thu, Sep 10, 2020 at 04:33:39PM +0000, Derrick, Jonathan wrote:
> > > > > On Wed, 2020-09-09 at 20:55 -0500, Bjorn Helgaas wrote:
> > > > > > On Fri, Aug 21, 2020 at 08:32:20PM +0800, Kai-Heng Feng wrote:
> > > > > > > New Intel laptops with VMD cannot reach deeper power saving state,
> > > > > > > renders very short battery time.
> > > > > > >
> > > > > > > As BIOS may not be able to program the config space for devices under
> > > > > > > VMD domain, ASPM needs to be programmed manually by software. This is
> > > > > > > also the case under Windows.
> > > > > > >
> > > > > > > The VMD controller itself is a root complex integrated endpoint that
> > > > > > > doesn't have ASPM capability, so we can't propagate the ASPM settings to
> > > > > > > devices under it. Hence, simply apply ASPM_STATE_ALL to the links under
> > > > > > > VMD domain, unsupported states will be cleared out anyway.
> > > > > > >
> > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > > > > ---
> > > > > > > drivers/pci/pcie/aspm.c | 3 ++-
> > > > > > > drivers/pci/quirks.c | 11 +++++++++++
> > > > > > > include/linux/pci.h | 2 ++
> > > > > > > 3 files changed, 15 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > > > > > index 253c30cc1967..dcc002dbca19 100644
> > > > > > > --- a/drivers/pci/pcie/aspm.c
> > > > > > > +++ b/drivers/pci/pcie/aspm.c
> > > > > > > @@ -624,7 +624,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> > > > > > > aspm_calc_l1ss_info(link, &upreg, &dwreg);
> > > > > > >
> > > > > > > /* Save default state */
> > > > > > > - link->aspm_default = link->aspm_enabled;
> > > > > > > + link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> > > > > > > + ASPM_STATE_ALL : link->aspm_enabled;
> > > > > >
> > > > > > This function is ridiculously complicated already, and I really don't
> > > > > > want to make it worse.
> > > > > >
> > > > > > What exactly is the PCIe topology here? Apparently the VMD controller
> > > > > > is a Root Complex Integrated Endpoint, so it's a Type 0 (non-bridge)
> > > > > > device. And it has no Link, hence no Link Capabilities or Control and
> > > > > > hence no ASPM-related bits. Right?
> > > > >
> > > > > That's correct. VMD is the Type 0 device providing config/mmio
> > > > > apertures to another segment and MSI/X remapping. No link and no ASPM
> > > > > related bits.
> > > > >
> > > > > Hierarchy is usually something like:
> > > > >
> > > > > Segment 0 | VMD segment
> > > > > Root Complex -> VMD | Type 0 (RP/Bridge; physical slot) - Type 1
> > > > > | Type 0 (RP/Bridge; physical slot) - Type 1
> > > > >
> > > > > > And the devices under the VMD controller? I guess they are regular
> > > > > > PCIe Endpoints, Switch Ports, etc? Obviously there's a Link involved
> > > > > > somewhere. Does the VMD controller have some magic, non-architected
> > > > > > Port on the downstream side?
> > > > >
> > > > > Correct: Type 0 and Type 1 devices, and any number of Switch ports as
> > > > > it's usually pinned out to physical slot.
> > > > >
> > > > > > Does this patch enable ASPM on this magic Link between VMD and the
> > > > > > next device? Configuring ASPM correctly requires knowledge and knobs
> > > > > > from both ends of the Link, and apparently we don't have those for the
> > > > > > VMD end.
> > > > >
> > > > > VMD itself doesn't have the link to it's domain. It's really just the
> > > > > config/mmio aperture and MSI/X remapper. The PCIe link is between the
> > > > > Type 0 and Type 1 devices on the VMD domain. So fortunately the VMD
> > > > > itself is not the upstream part of the link.
> > > > >
> > > > > > Or is it for Links deeper in the hierarchy? I assume those should
> > > > > > just work already, although there might be issues with latency
> > > > > > computation, etc., because we may not be able to account for the part
> > > > > > of the path above VMD.
> > > > >
> > > > > That's correct. This is for the links within the domain itself, such as
> > > > > between a type 0 and NVMe device.
> > > >
> > > > OK, great. So IIUC, below the VMD, there is a Root Port, and the Root
> > > > Port has a link to some Endpoint or Switch, e.g., an NVMe device. And
> > > > we just want to enable ASPM on that link.
> > > >
> > > > That should not be a special case; we should be able to make this so
> > > > it Just Works. Based on this patch, I guess the reason it doesn't
> > > > work is because link->aspm_enabled for that link isn't set correctly.
> > > >
> > > > So is this just a consequence of us depending on the initial Link
> > > > Control value from BIOS? That seems like something we shouldn't
> > > > really depend on.
> Seems like a good idea, that it should instead be quirked if ASPM is
> found unusable on a link. Though I'm not aware of how many platforms
> would require a quirk..
>
> > > >
> > > That's the crux. There's always pcie_aspm=force.
> > > Something I've wondered is if there is a way we could 'discover' if the
> > > link is ASPM safe?
> >
> > Sure. Link Capabilities is supposed to tell us that. If aspm.c
> > depends on the BIOS settings, I think that's a design mistake.
> >
> > But what CONFIG_PCIEASPM_* setting are you using? The default
> > is CONFIG_PCIEASPM_DEFAULT, which literally means "Use the BIOS
> > defaults". If you're using that, and BIOS doesn't enable ASPM below
> > VMD, I guess aspm.c will leave it disabled, and that seems like it
> > would be the expected behavior.
> >
> > Does "pcie_aspm=force" really help you? I don't see any uses of it
> > that should apply to your situation.
> >
> > Bjorn
>
> No you're right. I don't think we need pcie_aspm=force, just the policy
> configuration.
I'm not sure where we're at here.
If the kernel is built with CONFIG_PCIEASPM_DEFAULT=y (which means
"use the BIOS defaults"), and the BIOS doesn't enable ASPM on these
links below VMD, then Linux will leave things alone. I think that's
working as intended.
If desired, we should be able to enable ASPM using sysfs in that case.
We have a pci_disable_link_state() kernel interface that drivers can
use to *disable* ASPM for their device. But I don't think there's any
corresponding interface for drivers to *enable* ASPM. Maybe that's an
avenue to explore?
Bjorn
next prev parent reply other threads:[~2020-09-17 18:38 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <0f902d555deb423ef1c79835b23c917be2633162.camel@intel.com>
2020-09-10 19:17 ` [PATCH] PCI/ASPM: Enable ASPM for links under VMD domain Bjorn Helgaas
2020-09-10 19:51 ` Derrick, Jonathan
2020-09-17 17:20 ` Bjorn Helgaas [this message]
2020-09-23 14:29 ` Kai-Heng Feng
2020-08-21 12:32 Kai-Heng Feng
2020-08-24 13:04 ` Mika Westerberg
2020-08-25 6:23 ` Christoph Hellwig
2020-08-25 6:39 ` Kai Heng Feng
2020-08-25 6:56 ` Christoph Hellwig
2020-08-26 5:53 ` Kai-Heng Feng
2020-09-02 19:48 ` David Fugate
2020-09-02 22:54 ` Keith Busch
2020-08-26 21:43 ` Derrick, Jonathan
2020-08-27 6:34 ` hch
2020-08-27 16:13 ` Derrick, Jonathan
2020-08-27 16:23 ` hch
2020-08-27 16:45 ` Derrick, Jonathan
2020-08-27 16:50 ` hch
2020-08-27 21:33 ` Dan Williams
2020-08-29 7:23 ` hch
2020-08-27 17:49 ` Limonciello, Mario
2020-08-29 7:24 ` hch
2020-09-10 1:55 ` Bjorn Helgaas
2020-09-10 16:33 ` Derrick, Jonathan
2020-09-10 17:38 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200917172010.GA1710481@bjorn-Precision-5520 \
--to=helgaas@kernel.org \
--cc=Mario.Limonciello@dell.com \
--cc=bhelgaas@google.com \
--cc=hkallweit1@gmail.com \
--cc=jonathan.derrick@intel.com \
--cc=kai.heng.feng@canonical.com \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=rafael.j.wysocki@intel.com \
--cc=refactormyself@gmail.com \
--cc=wangxiongfeng2@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).