I was really hopeful this would have handled it, but no joy.
I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD"
and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately
on top of the below quirk patch to no avail.
The only thing that works is the aggregate patch I've added.
> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
> VMD_FEATS_CLIENT. In fact, they're not included in vmd_ids[] at all,
> so I'd be surprised if VMD worked for those devices unless BIOS set
> up the VMD itself.
Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten
"Enable LTR for endpoints behind VMD" to print if the BIOS already does
that for you, and sent the patch here (since I wasn't seeing the message
printed when the fixup was being done and wanted to know why).
I'd REALLY like to get this into mainline, so if there's anything I can
do to help, LMK.
Thanks,
-Kenny
On 12/13/24 08:43, Bjorn Helgaas wrote:
[+cc David, Nirmal, linux-pci]
On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
On 12/12/24 12:56, Bjorn Helgaas wrote:
On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
So we're on 6.13-rc2 and the patches are getting closer and
closer, but they still need to be manually added.
The good news is now only (variants of) "PCI/ASPM: Enable LTR
for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
under VMD domain" are needed.
...
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.
This looks equivalent in spirit to upstream
https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
0xa77f, 0xad0b, 0xb06f, 0xb60b.
However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
VMD_FEATS_CLIENT. In fact, they're not included in vmd_ids[] at all,
so I'd be surprised if VMD worked for those devices unless BIOS set
up the VMD itself.
Maybe David or Nirmal can comment on this?
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
IDs 0x9a09 and 0xa0b0.
This looks like it should also be handled by upstream f492edb40b54
("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
"pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
But again, the Device IDs mentioned in the Ubuntu commit are NOT
included in the upstream VMD_FEATS_CLIENT list.
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
below any VMD claimed by the "vmd" driver, not just VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.
I think the only thing that's missing is that the upstream vmd_ids[]
needs to be updated with some new VMD Device IDs that are tagged with
VMD_FEATS_CLIENT.
I don't know what the vmd_ids[] strategy is, but Kenneth, you might
try an upstream patch like the one below. If that resolves the
standby/low-power issues, maybe David or Nirmal can figure out the
"right" way to do this.
Bjorn
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9d9596947350..4de7ff3bbf23 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
.driver_data = VMD_FEATS_CLIENT,},
{PCI_VDEVICE(INTEL, 0xb06f),
.driver_data = VMD_FEATS_CLIENT,},
+ {PCI_VDEVICE(INTEL, 0x9a09),
+ .driver_data = VMD_FEATS_CLIENT,},
+ {PCI_VDEVICE(INTEL, 0xa0b0),
+ .driver_data = VMD_FEATS_CLIENT,},
+ {PCI_VDEVICE(INTEL, 0xa0bc),
+ .driver_data = VMD_FEATS_CLIENT,},
{0,}
};
MODULE_DEVICE_TABLE(pci, vmd_ids);
--
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
County CA
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..6cec8ed1a726 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -846,7 +846,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
}
/* Save default state */
- link->aspm_default = link->aspm_enabled;
+ link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+ PCIE_LINK_STATE_ASPM_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 76f4df75b08a..c5145e74df73 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6301,3 +6301,70 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
#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 db9b47ce3eef..9bd8234f1d39 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 {
