linux-kernel - Re: [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200504141521.GA265122@bjorn-Precision-5520>
Date:   Mon, 4 May 2020 09:15:21 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc:     bhelgaas@...gle.com,
        "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: Enable ASPM L1 on TI PCIe-to-PCI bridge

On Mon, May 04, 2020 at 03:02:59PM +0800, Kai-Heng Feng wrote:
> The TI PCIe-to-PCI bridge prevents the Intel SoC from entering power
> state deeper than PC3, consumes lots of unnecessary power.
> 
> On Windows ASPM L1 is enabled on the device and its upstream bridge,
> so it can make the Intel SoC reach PC8 or PC10 to save lots of power.

Does this work around some kind of hardware or firmware defect?  If
ASPM on this device works correctly, the generic code in aspm.c should
enable L1 automatically as it does for all other devices.

I don't think we should add quirks to set the ASPM configuration
directly for random devices.

> So enable ASPM L1 like Windows does, to save additional power.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207571
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> ---
>  drivers/pci/quirks.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index ca9ed5774eb1..ac7eccf34f87 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2330,6 +2330,27 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
>  
> +static void quirk_enable_aspm_l1(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge = pci_upstream_bridge(dev);
> +	u16 lnkctl;
> +
> +	pci_info(dev, "Enabling L1\n");
> +	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnkctl);
> +	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
> +		pcie_capability_write_word(dev, PCI_EXP_LNKCTL,
> +					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
> +
> +	if (!bridge)
> +		return;
> +
> +	pcie_capability_read_word(bridge, PCI_EXP_LNKCTL, &lnkctl);
> +	if (!(lnkctl & PCI_EXP_LNKCTL_ASPM_L1))
> +		pcie_capability_write_word(bridge, PCI_EXP_LNKCTL,
> +					   lnkctl | PCI_EXP_LNKCTL_ASPM_L1);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_TI, 0x8240, quirk_enable_aspm_l1);
> +
>  /*
>   * Some Pericom PCIe-to-PCI bridges in reverse mode need the PCIe Retrain
>   * Link bit cleared after starting the link retrain process to allow this
> -- 
> 2.17.1
>