| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPpJ_edNHfK-LAiFPxXsuxeH93W=a=G+5FMYnE3VKWfF=6jmrg@mail.gmail.com>
Date: Wed, 31 Jan 2024 15:37:53 +0800
From: Jian-Hong Pan <jhp@...lessos.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>,
David Box <david.e.box@...ux.intel.com>, Damien Le Moal <dlemoal@...nel.org>,
Niklas Cassel <cassel@...nel.org>, Nirmal Patel <nirmal.patel@...ux.intel.com>,
Jonathan Derrick <jonathan.derrick@...ux.dev>, linux-ide@...r.kernel.org,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, linux@...lessos.org,
Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH 2/2] PCI: vmd: enable PCI PM's L1 substates of remapped
PCIe port and NVMe
Bjorn Helgaas <helgaas@...nel.org> 於 2024年1月31日 週三 上午1:28寫道：
>
> [+cc Johan since qcom has similar code]
>
> On Tue, Jan 30, 2024 at 10:35:19AM -0600, Bjorn Helgaas wrote:
> > On Tue, Jan 30, 2024 at 06:00:51PM +0800, Jian-Hong Pan wrote:
> > > The remmapped PCIe port and NVMe have PCI PM L1 substates capability on
> > > ASUS B1400CEAE, but they are disabled originally:
> > >
> > > Capabilities: [900 v1] L1 PM Substates
> > > L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
> > > PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
> > > L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> > > T_CommonMode=0us LTR1.2_Threshold=0ns
> > > L1SubCtl2: T_PwrOn=10us
> > >
> > > Power on all of the VMD remapped PCI devices before enable PCI-PM L1 PM
> > > Substates by following "Section 5.5.4 of PCIe Base Spec Revision 5.0
> > > Version 0.1". Then, PCI PM's L1 substates control are enabled
> > > accordingly.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> > > Signed-off-by: Jian-Hong Pan <jhp@...lessos.org>
> > > ---
> > > drivers/pci/controller/vmd.c | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index 87b7856f375a..b1bbe8e6075a 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -738,6 +738,12 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
> > > vmd_bridge->native_dpc = root_bridge->native_dpc;
> > > }
> > >
> > > +static int vmd_power_on_pci_device(struct pci_dev *pdev, void *userdata)
> > > +{
> > > + pci_set_power_state(pdev, PCI_D0);
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > > */
> > > @@ -928,6 +934,13 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > vmd_acpi_begin();
> > >
> > > pci_scan_child_bus(vmd->bus);
> > > +
> > > + /*
> > > + * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
> > > + * Section 5.5.4 of PCIe Base Spec Revision 5.0 Version 0.1
> > > + */
> > > + pci_walk_bus(vmd->bus, vmd_power_on_pci_device, NULL);
> >
> > Sec 5.5.4 indeed says "If setting either or both of the enable bits
> > for PCI-PM L1 PM Substates, both ports must be configured ... while in
> > D0."
> >
> > This applies to *all* PCIe devices, not just those below a VMD bridge,
> > so I'm not sure this is the right place to do this. Is there anything
> > that prevents a similar issue for non-VMD hierarchies?
> >
> > I guess the bridges (Root Ports and Switch Ports) must already be in
> > D0, or we wouldn't be able to enumerate anything below them, right?
> >
> > It would be nice to connect this more closely with the L1 PM Substates
> > configuration. I don't quite see the connection here. The only path
> > I see for L1SS configuration is this:
> >
> > pci_scan_slot
> > pcie_aspm_init_link_state
> > pcie_aspm_cap_init
> > aspm_l1ss_init
> >
> > which of course is inside pci_scan_child_bus(), which happens *before*
> > this patch puts the devices in D0. Where does the L1SS configuration
> > happen after this vmd_power_on_pci_device()?
>
> I think I found it; a more complete call tree is like this, where the
> L1SS configuration is inside the pci_enable_link_state_locked() done
> by the vmd_pm_enable_quirk() bus walk:
>
> vmd_enable_domain
> pci_scan_child_bus
> ...
> pci_scan_slot
> pcie_aspm_init_link_state
> pcie_aspm_cap_init
> aspm_l1ss_init
> + pci_walk_bus(vmd->bus, vmd_power_on_pci_device, NULL)
> ...
> pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features)
> vmd_pm_enable_quirk
> pci_enable_link_state_locked(PCIE_LINK_STATE_ALL)
> __pci_enable_link_state
> link->aspm_default |= ...
> pcie_config_aspm_link
> pcie_config_aspm_l1ss
> pci_clear_and_set_config_dword(PCI_L1SS_CTL1) # <--
> pcie_config_aspm_dev
> pcie_capability_clear_and_set_word(PCI_EXP_LNKCTL)
>
> qcom_pcie_enable_aspm() does a similar pci_set_power_state(PCI_D0)
> before calling pci_enable_link_state_locked(). I would prefer to
> avoid the D0 precondition, but at the very least, I think we should
> add a note to the pci_enable_link_state_locked() kernel-doc saying the
> caller must ensure the device is in D0.
>
> I think vmd_pm_enable_quirk() is also problematic because it
> configures the LTR Capability *after* calling
> pci_enable_link_state_locked(PCIE_LINK_STATE_ALL).
>
> The pci_enable_link_state_locked() will enable ASPM L1.2, but sec
> 5.5.4 says LTR_L1.2_THRESHOLD must be programmed *before* ASPM L1.2
> Enable is set.
Thanks! This inspires me why LTR1.2_Threshold is 0ns here.
Jian-Hong Pan
Powered by blists - more mailing lists