Fwd: upstream linux cannot achieve package C8 power saving

Fwd: upstream linux cannot achieve package C8 power saving

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

Hi,

I notice a bug report on Bugzilla [1]. Quoting from it:

> v6.5 (and at least v5.15, v5.19 and v6.4 as well) will not go to a higher power saving level than package C3.
> 
> With the inclusion of a patch that combines 3 Ubuntu commits related to VMD ASPM & LTR, package C8 is used.

See Bugzilla for the full thread.

FYI, the attached proposed fix is the same as Brett's another BZ report [2].
I include it for upstreaming.

To Brett: Would you like to submit the proper, formal patch (see
Documentation/process/submitting-patches.rst for details)?

Thanks.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=217841
[2]: https://bugzilla.kernel.org/show_bug.cgi?id=217828

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: proposed.patch --]
[-- Type: text/x-patch, Size: 3661 bytes --]

commit 9491bfa0b1e5f658e09135759e7ebc373d9a72ce
Author: brett.hassall <brett.hassall@gmail.com>
Date:   Thu Aug 24 19:26:36 2023 +1000

    combined commit of: 71315b8e - UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain; d83e6f6e -  UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD; 069d0523 - UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 66d7514ca111..29f2f62aaefa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -682,7 +682,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	aspm_l1ss_init(link);
 
 	/* 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;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c525867760bf..a77e2e38fa6a 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6041,3 +6041,71 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
 #endif
+
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
+
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	if (!pci_fixup_is_vmd_bridge(pdev))
+		return;
+
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_fixup_is_vmd_bridge(parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 60b8772b5bd4..d3f96d111250 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {

Re: upstream linux cannot achieve package C8 power saving

[brett hassall]

Hi

Ok, I will read into working up a formal patch.

Thanks for the reply.

Brett


On Wed, 30 Aug 2023 at 11:11, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> Hi,
>
> I notice a bug report on Bugzilla [1]. Quoting from it:
>
> > v6.5 (and at least v5.15, v5.19 and v6.4 as well) will not go to a higher power saving level than package C3.
> >
> > With the inclusion of a patch that combines 3 Ubuntu commits related to VMD ASPM & LTR, package C8 is used.
>
> See Bugzilla for the full thread.
>
> FYI, the attached proposed fix is the same as Brett's another BZ report [2].
> I include it for upstreaming.
>
> To Brett: Would you like to submit the proper, formal patch (see
> Documentation/process/submitting-patches.rst for details)?
>
> Thanks.
>
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217841
> [2]: https://bugzilla.kernel.org/show_bug.cgi?id=217828
>
> --
> An old man doll... just what I always wanted! - Clara

Re: upstream linux cannot achieve package C8 power saving

[brett hassall]

Hi

I contacted the Ubuntu developers to see if they were ok with using
their patches.

They advised the patches were outdated and further development was
under discussion.

The current patches work and would benefit Linux users until something
better comes along.

Would you like me to proceed with the formal patch still ?

Thanks
Brett


On Wed, 30 Aug 2023 at 11:11, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> Hi,
>
> I notice a bug report on Bugzilla [1]. Quoting from it:
>
> > v6.5 (and at least v5.15, v5.19 and v6.4 as well) will not go to a higher power saving level than package C3.
> >
> > With the inclusion of a patch that combines 3 Ubuntu commits related to VMD ASPM & LTR, package C8 is used.
>
> See Bugzilla for the full thread.
>
> FYI, the attached proposed fix is the same as Brett's another BZ report [2].
> I include it for upstreaming.
>
> To Brett: Would you like to submit the proper, formal patch (see
> Documentation/process/submitting-patches.rst for details)?
>
> Thanks.
>
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217841
> [2]: https://bugzilla.kernel.org/show_bug.cgi?id=217828
>
> --
> An old man doll... just what I always wanted! - Clara

Re: upstream linux cannot achieve package C8 power saving

[brett hassall]

Hi

The Ubuntu devs have reverted their 3 patches and replaced them with 2
new patches.

Given this happened in the last week, I'll hold off submitting a patch
for now and see what Ubuntu does

Thanks
Brett


On Tue, 5 Sept 2023 at 06:25, brett hassall <brett.hassall@gmail.com> wrote:
>
> Hi
>
> I contacted the Ubuntu developers to see if they were ok with using
> their patches.
>
> They advised the patches were outdated and further development was
> under discussion.
>
> The current patches work and would benefit Linux users until something
> better comes along.
>
> Would you like me to proceed with the formal patch still ?
>
> Thanks
> Brett
>
>
> On Wed, 30 Aug 2023 at 11:11, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> >
> > Hi,
> >
> > I notice a bug report on Bugzilla [1]. Quoting from it:
> >
> > > v6.5 (and at least v5.15, v5.19 and v6.4 as well) will not go to a higher power saving level than package C3.
> > >
> > > With the inclusion of a patch that combines 3 Ubuntu commits related to VMD ASPM & LTR, package C8 is used.
> >
> > See Bugzilla for the full thread.
> >
> > FYI, the attached proposed fix is the same as Brett's another BZ report [2].
> > I include it for upstreaming.
> >
> > To Brett: Would you like to submit the proper, formal patch (see
> > Documentation/process/submitting-patches.rst for details)?
> >
> > Thanks.
> >
> > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217841
> > [2]: https://bugzilla.kernel.org/show_bug.cgi?id=217828
> >
> > --
> > An old man doll... just what I always wanted! - Clara