linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware
@ 2023-01-20  3:15 David E. Box
  2023-01-20  3:15 ` [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state() David E. Box
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: David E. Box @ 2023-01-20  3:15 UTC (permalink / raw)
  To: david.e.box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel

This series adds a work around for enabling PCIe ASPM and for setting PCIe
LTR values on VMD reserved root ports on select platforms. While
configuration of these capabilities is usually done by BIOS, on these
platforms these capabilities will not be configured because the ports are
not visible to BIOS. This was part of an initial design that expected the
driver to completely handle the ports, including power management. However
on Linux those ports are still managed by the PCIe core, which has the
expectation that they adhere to device standards including BIOS
configuration, leading to this problem.

The target platforms are Tiger Lake, Alder Lake, and Raptor Lake though the
latter has already implemented support for configuring the LTR values.
Meteor Lake is expected add BIOS ASPM support, eliminating the future need
for this work around.

Note, the driver programs the LTRs because BIOS would also normally do this
for devices that do not set them by default. Without this, SoC power
management would be blocked on those platform. This SoC specific value is
the maximum latency required to allow the SoC to enter the deepest power
state.

This patch addresses the following open bugzillas on VMD enabled laptops
that cannot enter low power states.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717

David E. Box (3):
  PCI: vmd: Use PCI_VDEVICE in device list
  PCI: vmd: Create feature grouping for client products
  PCI: vmd: Add quirk to configure PCIe ASPM and LTR

Michael Bottini (1):
  PCI/ASPM: Add pci_enable_link_state()

 drivers/pci/controller/vmd.c | 97 ++++++++++++++++++++++++++----------
 drivers/pci/pcie/aspm.c      | 54 ++++++++++++++++++++
 include/linux/pci.h          |  7 +++
 3 files changed, 132 insertions(+), 26 deletions(-)


base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4
-- 
2.34.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state()
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
@ 2023-01-20  3:15 ` David E. Box
  2023-01-20  3:15 ` [PATCH V10 2/4] PCI: vmd: Use PCI_VDEVICE in device list David E. Box
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: David E. Box @ 2023-01-20  3:15 UTC (permalink / raw)
  To: david.e.box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel

From: Michael Bottini <michael.a.bottini@linux.intel.com>

Add pci_enable_link_state() to allow devices to change the default BIOS
configured states. Clears the BIOS default settings then sets the new
states and reconfigures the link under the semaphore. Also add
PCIE_LINK_STATE_ALL macro for convenience for callers that want to enable
all link states.

Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 V10
  - No change
 V9
  - No change
 V8
  - No change

 V7
  - Fix description as suggested by Bjorn
  - Rename function to pci_enable_link_state

 V6
  - No change
 V5
  - Rename to pci_enable_default_link_state and model after
    pci_disable_link_state() as suggested by Bjorn.
  - Add helper PCIE_LINK_STATE_ALL which sets bits for all links states and
    clock pm.
  - Clarify commit language to indicate the function changes the default
    link states (not ASPM policy).
 V4
  - Refactor vmd_enable_apsm() to exit early, making the lines shorter
    and more readable. Suggested by Christoph.
 V3
  - No changes
 V2
  - Use return status to print pci_info message if ASPM cannot be enabled.
  - Add missing static declaration, caught by lkp@intel.com

 drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  7 ++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 53a1fa306e1e..339c686a5094 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1181,6 +1181,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
 }
 EXPORT_SYMBOL(pci_disable_link_state);
 
+/**
+ * pci_enable_link_state - Clear and set the default device link state so that
+ * the link may be allowed to enter the specified states. Note that if the
+ * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
+ * touch the LNKCTL register. Also note that this does not enable states
+ * disabled by pci_disable_link_state(). Return 0 or a negative errno.
+ *
+ * @pdev: PCI device
+ * @state: Mask of ASPM link states to enable
+ */
+int pci_enable_link_state(struct pci_dev *pdev, int state)
+{
+	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
+
+	if (!link)
+		return -EINVAL;
+	/*
+	 * A driver requested that ASPM be enabled on this device, but
+	 * if we don't have permission to manage ASPM (e.g., on ACPI
+	 * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
+	 * the _OSC method), we can't honor that request.
+	 */
+	if (aspm_disabled) {
+		pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
+		return -EPERM;
+	}
+
+	down_read(&pci_bus_sem);
+	mutex_lock(&aspm_lock);
+	link->aspm_default = 0;
+	if (state & PCIE_LINK_STATE_L0S)
+		link->aspm_default |= ASPM_STATE_L0S;
+	if (state & PCIE_LINK_STATE_L1)
+		/* L1 PM substates require L1 */
+		link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
+	if (state & PCIE_LINK_STATE_L1_1)
+		link->aspm_default |= ASPM_STATE_L1_1;
+	if (state & PCIE_LINK_STATE_L1_2)
+		link->aspm_default |= ASPM_STATE_L1_2;
+	if (state & PCIE_LINK_STATE_L1_1_PCIPM)
+		link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
+	if (state & PCIE_LINK_STATE_L1_2_PCIPM)
+		link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
+	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+
+	link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
+	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	mutex_unlock(&aspm_lock);
+	up_read(&pci_bus_sem);
+
+	return 0;
+}
+EXPORT_SYMBOL(pci_enable_link_state);
+
 static int pcie_aspm_set_policy(const char *val,
 				const struct kernel_param *kp)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index adffd65e84b4..ea601e6fbbda 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1685,10 +1685,15 @@ extern bool pcie_ports_native;
 #define PCIE_LINK_STATE_L1_2		BIT(4)
 #define PCIE_LINK_STATE_L1_1_PCIPM	BIT(5)
 #define PCIE_LINK_STATE_L1_2_PCIPM	BIT(6)
+#define PCIE_LINK_STATE_ALL		(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
+					 PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
+					 PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
+					 PCIE_LINK_STATE_L1_2_PCIPM)
 
 #ifdef CONFIG_PCIEASPM
 int pci_disable_link_state(struct pci_dev *pdev, int state);
 int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
+int pci_enable_link_state(struct pci_dev *pdev, int state);
 void pcie_no_aspm(void);
 bool pcie_aspm_support_enabled(void);
 bool pcie_aspm_enabled(struct pci_dev *pdev);
@@ -1697,6 +1702,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
 { return 0; }
 static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
 { return 0; }
+static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
+{ return 0; }
 static inline void pcie_no_aspm(void) { }
 static inline bool pcie_aspm_support_enabled(void) { return false; }
 static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V10 2/4] PCI: vmd: Use PCI_VDEVICE in device list
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
  2023-01-20  3:15 ` [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state() David E. Box
@ 2023-01-20  3:15 ` David E. Box
  2023-01-20  3:15 ` [PATCH V10 3/4] PCI: vmd: Create feature grouping for client products David E. Box
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: David E. Box @ 2023-01-20  3:15 UTC (permalink / raw)
  To: david.e.box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel

Use PCI_VDEVICE to simply the device table.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 V10 - Changed commit message.

 V9 - No change

 V8 - No change

 V7 - New Patch. Separate patch suggested by Lorenzo

 drivers/pci/controller/vmd.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 769eedeb8802..1fc3f2174359 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -1017,33 +1017,33 @@ static int vmd_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
 
 static const struct pci_device_id vmd_ids[] = {
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
+	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
+	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x467f),
+	{PCI_VDEVICE(INTEL, 0x467f),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x4c3d),
+	{PCI_VDEVICE(INTEL, 0x4c3d),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa77f),
+	{PCI_VDEVICE(INTEL, 0xa77f),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x7d0b),
+	{PCI_VDEVICE(INTEL, 0x7d0b),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xad0b),
+	{PCI_VDEVICE(INTEL, 0xad0b),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
+	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_OFFSET_FIRST_VECTOR,},
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V10 3/4] PCI: vmd: Create feature grouping for client products
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
  2023-01-20  3:15 ` [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state() David E. Box
  2023-01-20  3:15 ` [PATCH V10 2/4] PCI: vmd: Use PCI_VDEVICE in device list David E. Box
@ 2023-01-20  3:15 ` David E. Box
  2023-01-20  3:15 ` [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR David E. Box
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: David E. Box @ 2023-01-20  3:15 UTC (permalink / raw)
  To: david.e.box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel

Simplify the device ID list by creating a grouping of features shared by
client products.

Suggested-by: Jon Derrick <jonathan.derrick@linux.dev>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 V10 - No change.

 V9 - No change.

 V8 - New patch.

 drivers/pci/controller/vmd.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 1fc3f2174359..47fa3e5f2dc5 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -68,6 +68,10 @@ enum vmd_features {
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
 };
 
+#define VMD_FEATS_CLIENT	(VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |	\
+				 VMD_FEAT_HAS_BUS_RESTRICTIONS |	\
+				 VMD_FEAT_OFFSET_FIRST_VECTOR)
+
 static DEFINE_IDA(vmd_instance_ida);
 
 /*
@@ -1024,29 +1028,17 @@ static const struct pci_device_id vmd_ids[] = {
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
 	{PCI_VDEVICE(INTEL, 0x467f),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0x4c3d),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0xa77f),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0x7d0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0xad0b),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
-		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
-				VMD_FEAT_HAS_BUS_RESTRICTIONS |
-				VMD_FEAT_OFFSET_FIRST_VECTOR,},
+		.driver_data = VMD_FEATS_CLIENT,},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
                   ` (2 preceding siblings ...)
  2023-01-20  3:15 ` [PATCH V10 3/4] PCI: vmd: Create feature grouping for client products David E. Box
@ 2023-01-20  3:15 ` David E. Box
  2023-03-20 22:56   ` Ville Syrjälä
  2023-01-20  5:28 ` [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware Sathyanarayanan Kuppuswamy
  2023-02-02 15:07 ` Lorenzo Pieralisi
  5 siblings, 1 reply; 11+ messages in thread
From: David E. Box @ 2023-01-20  3:15 UTC (permalink / raw)
  To: david.e.box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel

PCIe ports reserved for VMD use are not visible to BIOS and therefore not
configured to enable PCIe ASPM or LTR values (which BIOS will configure if
they are not set). Lack of this programming results in high power
consumption on laptops as reported in bugzilla.  For affected products use
pci_enable_link_state to set the allowed link states for devices on the
root ports. Also set the LTR value to the maximum value needed for the SoC.

This is a workaround for products from Rocket Lake through Alder Lake.
Raptor Lake, the latest product at this time, has already implemented LTR
configuring in BIOS. Future products will move ASPM configuration back to
BIOS as well.  As this solution is intended for laptops, support is not
added for hotplug or for devices downstream of a switch on the root port.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717

Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Jon Derrick <jonathan.derrick@linux.dev>
Reviewed-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 V10
  - No change
 V9
  - Added BIOS quirk flag to VMD_FEATS_CLIENT flag, suggested by Sathya.
 V8
  - Removed struct vmd_device_data patch. Instead use #define for the LTR
    value which is the same across all products needing the quirk.
 V7
  - No change
 V6
  - Set ASPM first before setting LTR. This is needed because some
    devices may only have LTR set by BIOS and not ASPM
  - Skip setting the LTR if the current LTR in non-zero.
 V5
  - Provide the LTR value as driver data.
  - Use DWORD for the config space write to avoid PCI WORD access bug.
  - Set ASPM links firsts, enabling all link states, before setting a
    default LTR if the capability is present
  - Add kernel message that VMD is setting the device LTR.
 V4
  - Refactor vmd_enable_apsm() to exit early, making the lines shorter
    and more readable. Suggested by Christoph.
 V3
  - No changes
 V2
  - Use return status to print pci_info message if ASPM cannot be enabled.
  - Add missing static declaration, caught by lkp@intel.com

 drivers/pci/controller/vmd.c | 55 +++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 47fa3e5f2dc5..990630ec57c6 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -66,11 +66,22 @@ enum vmd_features {
 	 * interrupt handling.
 	 */
 	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
+
+	/*
+	 * Enable ASPM on the PCIE root ports and set the default LTR of the
+	 * storage devices on platforms where these values are not configured by
+	 * BIOS. This is needed for laptops, which require these settings for
+	 * proper power management of the SoC.
+	 */
+	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
 };
 
+#define VMD_BIOS_PM_QUIRK_LTR	0x1003	/* 3145728 ns */
+
 #define VMD_FEATS_CLIENT	(VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |	\
 				 VMD_FEAT_HAS_BUS_RESTRICTIONS |	\
-				 VMD_FEAT_OFFSET_FIRST_VECTOR)
+				 VMD_FEAT_OFFSET_FIRST_VECTOR |		\
+				 VMD_FEAT_BIOS_PM_QUIRK)
 
 static DEFINE_IDA(vmd_instance_ida);
 
@@ -713,6 +724,46 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
 	vmd_bridge->native_dpc = root_bridge->native_dpc;
 }
 
+/*
+ * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
+ */
+static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
+{
+	unsigned long features = *(unsigned long *)userdata;
+	u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
+	u32 ltr_reg;
+	int pos;
+
+	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
+		return 0;
+
+	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return 0;
+
+	/*
+	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
+	 * so the LTR quirk is not needed.
+	 */
+	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
+	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
+		return 0;
+
+	/*
+	 * Set the default values to the maximum required by the platform to
+	 * allow the deepest power management savings. Write as a DWORD where
+	 * the lower word is the max snoop latency and the upper word is the
+	 * max non-snoop latency.
+	 */
+	ltr_reg = (ltr << 16) | ltr;
+	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
+	pci_info(pdev, "VMD: Default LTR value set by driver\n");
+
+	return 0;
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -885,6 +936,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
+	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
+
 	/*
 	 * VMD root buses are virtual and don't return true on pci_is_pcie()
 	 * and will fail pcie_bus_configure_settings() early. It can instead be
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
                   ` (3 preceding siblings ...)
  2023-01-20  3:15 ` [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR David E. Box
@ 2023-01-20  5:28 ` Sathyanarayanan Kuppuswamy
  2023-02-02 15:07 ` Lorenzo Pieralisi
  5 siblings, 0 replies; 11+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2023-01-20  5:28 UTC (permalink / raw)
  To: David E. Box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
	hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me
  Cc: linux-pci, linux-kernel



On 1/19/23 7:15 PM, David E. Box wrote:
> This series adds a work around for enabling PCIe ASPM and for setting PCIe
> LTR values on VMD reserved root ports on select platforms. While
> configuration of these capabilities is usually done by BIOS, on these
> platforms these capabilities will not be configured because the ports are
> not visible to BIOS. This was part of an initial design that expected the
> driver to completely handle the ports, including power management. However
> on Linux those ports are still managed by the PCIe core, which has the
> expectation that they adhere to device standards including BIOS
> configuration, leading to this problem.
> 
> The target platforms are Tiger Lake, Alder Lake, and Raptor Lake though the
> latter has already implemented support for configuring the LTR values.
> Meteor Lake is expected add BIOS ASPM support, eliminating the future need
> for this work around.
> 
> Note, the driver programs the LTRs because BIOS would also normally do this
> for devices that do not set them by default. Without this, SoC power
> management would be blocked on those platform. This SoC specific value is
> the maximum latency required to allow the SoC to enter the deepest power
> state.
> 
> This patch addresses the following open bugzillas on VMD enabled laptops
> that cannot enter low power states.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717
> 

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> David E. Box (3):
>   PCI: vmd: Use PCI_VDEVICE in device list
>   PCI: vmd: Create feature grouping for client products
>   PCI: vmd: Add quirk to configure PCIe ASPM and LTR
> 
> Michael Bottini (1):
>   PCI/ASPM: Add pci_enable_link_state()
> 
>  drivers/pci/controller/vmd.c | 97 ++++++++++++++++++++++++++----------
>  drivers/pci/pcie/aspm.c      | 54 ++++++++++++++++++++
>  include/linux/pci.h          |  7 +++
>  3 files changed, 132 insertions(+), 26 deletions(-)
> 
> 
> base-commit: 5dc4c995db9eb45f6373a956eb1f69460e69e6d4

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware
  2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
                   ` (4 preceding siblings ...)
  2023-01-20  5:28 ` [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware Sathyanarayanan Kuppuswamy
@ 2023-02-02 15:07 ` Lorenzo Pieralisi
  5 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Pieralisi @ 2023-02-02 15:07 UTC (permalink / raw)
  To: rafael, robh, bhelgaas, michael.a.bottini, hch, nirmal.patel, kw,
	me, David E. Box, jonathan.derrick, Lorenzo Pieralisi
  Cc: linux-kernel, linux-pci

On Thu, 19 Jan 2023 19:15:18 -0800, David E. Box wrote:
> This series adds a work around for enabling PCIe ASPM and for setting PCIe
> LTR values on VMD reserved root ports on select platforms. While
> configuration of these capabilities is usually done by BIOS, on these
> platforms these capabilities will not be configured because the ports are
> not visible to BIOS. This was part of an initial design that expected the
> driver to completely handle the ports, including power management. However
> on Linux those ports are still managed by the PCIe core, which has the
> expectation that they adhere to device standards including BIOS
> configuration, leading to this problem.
> 
> [...]

Applied to pci/controller/vmd, thanks!

[1/4] PCI/ASPM: Add pci_enable_link_state()
      https://git.kernel.org/pci/pci/c/de82f60f9c86
[2/4] PCI: vmd: Use PCI_VDEVICE in device list
      https://git.kernel.org/pci/pci/c/cca0dfecdba3
[3/4] PCI: vmd: Create feature grouping for client products
      https://git.kernel.org/pci/pci/c/14d2079af648
[4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
      https://git.kernel.org/pci/pci/c/f492edb40b54

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
  2023-01-20  3:15 ` [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR David E. Box
@ 2023-03-20 22:56   ` Ville Syrjälä
  2023-03-21  2:24     ` David E. Box
  0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2023-03-20 22:56 UTC (permalink / raw)
  To: David E. Box
  Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, hch, kw, robh,
	bhelgaas, michael.a.bottini, rafael, me, linux-pci, linux-kernel,
	intel-gfx

On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> +/*
> + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> + */
> +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> +{
> +	unsigned long features = *(unsigned long *)userdata;
> +	u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> +	u32 ltr_reg;
> +	int pos;
> +
> +	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> +		return 0;
> +
> +	pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);

Hi,

This is tripping lockdep on one our CI ADL machines.

https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt

<4>[   13.815380] ============================================
<4>[   13.815382] WARNING: possible recursive locking detected
<4>[   13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
<4>[   13.815386] --------------------------------------------
<4>[   13.815387] swapper/0/1 is trying to acquire lock:
<4>[   13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at: pci_enable_link_state+0x69/0x1d0
<4>[   13.815396] 
                  but task is already holding lock:
<4>[   13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at: pci_walk_bus+0x24/0x90
<4>[   13.815403] 
                  other info that might help us debug this:
<4>[   13.815404]  Possible unsafe locking scenario:

<4>[   13.815406]        CPU0
<4>[   13.815407]        ----
<4>[   13.815408]   lock(pci_bus_sem);
<4>[   13.815410]   lock(pci_bus_sem);
<4>[   13.815411] 
                   *** DEADLOCK ***

<4>[   13.815413]  May be due to missing lock nesting notation

<4>[   13.815414] 2 locks held by swapper/0/1:
<4>[   13.815416]  #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at: __driver_attach+0xab/0x180
<4>[   13.815422]  #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at: pci_walk_bus+0x24/0x90
<4>[   13.815426] 
                  stack backtrace:
<4>[   13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1
<4>[   13.815431] Hardware name: Intel Corporation Alder Lake Client Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419 03/25/2022
<4>[   13.815434] Call Trace:
<4>[   13.815436]  <TASK>
<4>[   13.815437]  dump_stack_lvl+0x64/0xb0
<4>[   13.815443]  __lock_acquire+0x9b5/0x2550
<4>[   13.815461]  lock_acquire+0xd7/0x330
<4>[   13.815463]  ? pci_enable_link_state+0x69/0x1d0
<4>[   13.815466]  down_read+0x3d/0x180
<4>[   13.815480]  ? pci_enable_link_state+0x69/0x1d0
<4>[   13.815482]  pci_enable_link_state+0x69/0x1d0
<4>[   13.815485]  ? __pfx_vmd_pm_enable_quirk+0x10/0x10
<4>[   13.815488]  vmd_pm_enable_quirk+0x49/0xb0
<4>[   13.815490]  pci_walk_bus+0x6d/0x90
<4>[   13.815492]  vmd_probe+0x75f/0x9d0
<4>[   13.815495]  pci_device_probe+0x95/0x120
<4>[   13.815498]  really_probe+0x164/0x3c0
<4>[   13.815500]  ? __pfx___driver_attach+0x10/0x10
<4>[   13.815503]  __driver_probe_device+0x73/0x170
<4>[   13.815506]  driver_probe_device+0x19/0xa0
<4>[   13.815508]  __driver_attach+0xb6/0x180
<4>[   13.815511]  ? __pfx___driver_attach+0x10/0x10
<4>[   13.815513]  bus_for_each_dev+0x77/0xd0
<4>[   13.815516]  bus_add_driver+0x114/0x210
<4>[   13.815518]  driver_register+0x5b/0x110
<4>[   13.815520]  ? __pfx_vmd_drv_init+0x10/0x10
<4>[   13.815523]  do_one_initcall+0x57/0x330
<4>[   13.815527]  kernel_init_freeable+0x181/0x3a0
<4>[   13.815529]  ? __pfx_kernel_init+0x10/0x10
<4>[   13.815532]  kernel_init+0x15/0x120
<4>[   13.815534]  ret_from_fork+0x29/0x50
<4>[   13.815537]  </TASK>

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
  2023-03-20 22:56   ` Ville Syrjälä
@ 2023-03-21  2:24     ` David E. Box
  2023-03-21 12:38       ` Ville Syrjälä
  0 siblings, 1 reply; 11+ messages in thread
From: David E. Box @ 2023-03-21  2:24 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, hch, kw, robh,
	bhelgaas, michael.a.bottini, rafael, me, linux-pci, linux-kernel,
	intel-gfx

Hi,

On Tue, 2023-03-21 at 00:56 +0200, Ville Syrjälä wrote:
> On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> > +/*
> > + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > + */
> > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > +{
> > +       unsigned long features = *(unsigned long *)userdata;
> > +       u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > +       u32 ltr_reg;
> > +       int pos;
> > +
> > +       if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > +               return 0;
> > +
> > +       pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);

We call pci_enable_link_state from a callback that's run during pci_walk_bus()
which I see already acquires the semaphore. We've had this patch for well over a
year and I haven't seen this issue before. Is there a particular config needed
to reproduce it?

As far as a solution I think we can copy what __pci_disable_link_state() does
and add a bool argument so that we only do down/up on the semaphore when set to
true. Since we know we will in be the lock during the bus walk we can set it to
false.

David

> 
> Hi,
> 
> This is tripping lockdep on one our CI ADL machines.
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt
> 
> <4>[   13.815380] ============================================
> <4>[   13.815382] WARNING: possible recursive locking detected
> <4>[   13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
> <4>[   13.815386] --------------------------------------------
> <4>[   13.815387] swapper/0/1 is trying to acquire lock:
> <4>[   13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> pci_enable_link_state+0x69/0x1d0
> <4>[   13.815396] 
>                   but task is already holding lock:
> <4>[   13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> pci_walk_bus+0x24/0x90
> <4>[   13.815403] 
>                   other info that might help us debug this:
> <4>[   13.815404]  Possible unsafe locking scenario:
> 
> <4>[   13.815406]        CPU0
> <4>[   13.815407]        ----
> <4>[   13.815408]   lock(pci_bus_sem);
> <4>[   13.815410]   lock(pci_bus_sem);
> <4>[   13.815411] 
>                    *** DEADLOCK ***
> 
> <4>[   13.815413]  May be due to missing lock nesting notation
> 
> <4>[   13.815414] 2 locks held by swapper/0/1:
> <4>[   13.815416]  #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at:
> __driver_attach+0xab/0x180
> <4>[   13.815422]  #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> pci_walk_bus+0x24/0x90
> <4>[   13.815426] 
>                   stack backtrace:
> <4>[   13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-
> CI_DRM_12814-g4753bbc2a817+ #1
> <4>[   13.815431] Hardware name: Intel Corporation Alder Lake Client
> Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419
> 03/25/2022
> <4>[   13.815434] Call Trace:
> <4>[   13.815436]  <TASK>
> <4>[   13.815437]  dump_stack_lvl+0x64/0xb0
> <4>[   13.815443]  __lock_acquire+0x9b5/0x2550
> <4>[   13.815461]  lock_acquire+0xd7/0x330
> <4>[   13.815463]  ? pci_enable_link_state+0x69/0x1d0
> <4>[   13.815466]  down_read+0x3d/0x180
> <4>[   13.815480]  ? pci_enable_link_state+0x69/0x1d0
> <4>[   13.815482]  pci_enable_link_state+0x69/0x1d0
> <4>[   13.815485]  ? __pfx_vmd_pm_enable_quirk+0x10/0x10
> <4>[   13.815488]  vmd_pm_enable_quirk+0x49/0xb0
> <4>[   13.815490]  pci_walk_bus+0x6d/0x90
> <4>[   13.815492]  vmd_probe+0x75f/0x9d0
> <4>[   13.815495]  pci_device_probe+0x95/0x120
> <4>[   13.815498]  really_probe+0x164/0x3c0
> <4>[   13.815500]  ? __pfx___driver_attach+0x10/0x10
> <4>[   13.815503]  __driver_probe_device+0x73/0x170
> <4>[   13.815506]  driver_probe_device+0x19/0xa0
> <4>[   13.815508]  __driver_attach+0xb6/0x180
> <4>[   13.815511]  ? __pfx___driver_attach+0x10/0x10
> <4>[   13.815513]  bus_for_each_dev+0x77/0xd0
> <4>[   13.815516]  bus_add_driver+0x114/0x210
> <4>[   13.815518]  driver_register+0x5b/0x110
> <4>[   13.815520]  ? __pfx_vmd_drv_init+0x10/0x10
> <4>[   13.815523]  do_one_initcall+0x57/0x330
> <4>[   13.815527]  kernel_init_freeable+0x181/0x3a0
> <4>[   13.815529]  ? __pfx_kernel_init+0x10/0x10
> <4>[   13.815532]  kernel_init+0x15/0x120
> <4>[   13.815534]  ret_from_fork+0x29/0x50
> <4>[   13.815537]  </TASK>
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
  2023-03-21  2:24     ` David E. Box
@ 2023-03-21 12:38       ` Ville Syrjälä
  2023-03-21 23:40         ` David E. Box
  0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2023-03-21 12:38 UTC (permalink / raw)
  To: David E. Box
  Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, hch, kw, robh,
	bhelgaas, michael.a.bottini, rafael, me, linux-pci, linux-kernel,
	intel-gfx

On Mon, Mar 20, 2023 at 07:24:16PM -0700, David E. Box wrote:
> Hi,
> 
> On Tue, 2023-03-21 at 00:56 +0200, Ville Syrjälä wrote:
> > On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> > > +/*
> > > + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > > + */
> > > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > +{
> > > +       unsigned long features = *(unsigned long *)userdata;
> > > +       u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > > +       u32 ltr_reg;
> > > +       int pos;
> > > +
> > > +       if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > > +               return 0;
> > > +
> > > +       pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> 
> We call pci_enable_link_state from a callback that's run during pci_walk_bus()
> which I see already acquires the semaphore. We've had this patch for well over a
> year and I haven't seen this issue before. Is there a particular config needed
> to reproduce it?

Not sure what would affect it, beyond the normal PROVE_LOCKING=y.

This is the .config our CI uses:
https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/blob/master/kconfig/debug

> 
> As far as a solution I think we can copy what __pci_disable_link_state() does
> and add a bool argument so that we only do down/up on the semaphore when set to
> true. Since we know we will in be the lock during the bus walk we can set it to
> false.
> 
> David
> 
> > 
> > Hi,
> > 
> > This is tripping lockdep on one our CI ADL machines.
> > 
> > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt
> > 
> > <4>[   13.815380] ============================================
> > <4>[   13.815382] WARNING: possible recursive locking detected
> > <4>[   13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
> > <4>[   13.815386] --------------------------------------------
> > <4>[   13.815387] swapper/0/1 is trying to acquire lock:
> > <4>[   13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_enable_link_state+0x69/0x1d0
> > <4>[   13.815396] 
> >                   but task is already holding lock:
> > <4>[   13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[   13.815403] 
> >                   other info that might help us debug this:
> > <4>[   13.815404]  Possible unsafe locking scenario:
> > 
> > <4>[   13.815406]        CPU0
> > <4>[   13.815407]        ----
> > <4>[   13.815408]   lock(pci_bus_sem);
> > <4>[   13.815410]   lock(pci_bus_sem);
> > <4>[   13.815411] 
> >                    *** DEADLOCK ***
> > 
> > <4>[   13.815413]  May be due to missing lock nesting notation
> > 
> > <4>[   13.815414] 2 locks held by swapper/0/1:
> > <4>[   13.815416]  #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at:
> > __driver_attach+0xab/0x180
> > <4>[   13.815422]  #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[   13.815426] 
> >                   stack backtrace:
> > <4>[   13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-
> > CI_DRM_12814-g4753bbc2a817+ #1
> > <4>[   13.815431] Hardware name: Intel Corporation Alder Lake Client
> > Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419
> > 03/25/2022
> > <4>[   13.815434] Call Trace:
> > <4>[   13.815436]  <TASK>
> > <4>[   13.815437]  dump_stack_lvl+0x64/0xb0
> > <4>[   13.815443]  __lock_acquire+0x9b5/0x2550
> > <4>[   13.815461]  lock_acquire+0xd7/0x330
> > <4>[   13.815463]  ? pci_enable_link_state+0x69/0x1d0
> > <4>[   13.815466]  down_read+0x3d/0x180
> > <4>[   13.815480]  ? pci_enable_link_state+0x69/0x1d0
> > <4>[   13.815482]  pci_enable_link_state+0x69/0x1d0
> > <4>[   13.815485]  ? __pfx_vmd_pm_enable_quirk+0x10/0x10
> > <4>[   13.815488]  vmd_pm_enable_quirk+0x49/0xb0
> > <4>[   13.815490]  pci_walk_bus+0x6d/0x90
> > <4>[   13.815492]  vmd_probe+0x75f/0x9d0
> > <4>[   13.815495]  pci_device_probe+0x95/0x120
> > <4>[   13.815498]  really_probe+0x164/0x3c0
> > <4>[   13.815500]  ? __pfx___driver_attach+0x10/0x10
> > <4>[   13.815503]  __driver_probe_device+0x73/0x170
> > <4>[   13.815506]  driver_probe_device+0x19/0xa0
> > <4>[   13.815508]  __driver_attach+0xb6/0x180
> > <4>[   13.815511]  ? __pfx___driver_attach+0x10/0x10
> > <4>[   13.815513]  bus_for_each_dev+0x77/0xd0
> > <4>[   13.815516]  bus_add_driver+0x114/0x210
> > <4>[   13.815518]  driver_register+0x5b/0x110
> > <4>[   13.815520]  ? __pfx_vmd_drv_init+0x10/0x10
> > <4>[   13.815523]  do_one_initcall+0x57/0x330
> > <4>[   13.815527]  kernel_init_freeable+0x181/0x3a0
> > <4>[   13.815529]  ? __pfx_kernel_init+0x10/0x10
> > <4>[   13.815532]  kernel_init+0x15/0x120
> > <4>[   13.815534]  ret_from_fork+0x29/0x50
> > <4>[   13.815537]  </TASK>
> > 

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
  2023-03-21 12:38       ` Ville Syrjälä
@ 2023-03-21 23:40         ` David E. Box
  0 siblings, 0 replies; 11+ messages in thread
From: David E. Box @ 2023-03-21 23:40 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, hch, kw, robh,
	bhelgaas, michael.a.bottini, rafael, me, linux-pci, linux-kernel,
	intel-gfx

On Tue, 2023-03-21 at 14:38 +0200, Ville Syrjälä wrote:
> On Mon, Mar 20, 2023 at 07:24:16PM -0700, David E. Box wrote:
> > Hi,
> > 
> > On Tue, 2023-03-21 at 00:56 +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> > > > +/*
> > > > + * Enable ASPM and LTR settings on devices that aren't configured by
> > > > BIOS.
> > > > + */
> > > > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > > +{
> > > > +       unsigned long features = *(unsigned long *)userdata;
> > > > +       u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > > > +       u32 ltr_reg;
> > > > +       int pos;
> > > > +
> > > > +       if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > > > +               return 0;
> > > > +
> > > > +       pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > 
> > We call pci_enable_link_state from a callback that's run during
> > pci_walk_bus()
> > which I see already acquires the semaphore. We've had this patch for well
> > over a
> > year and I haven't seen this issue before. Is there a particular config
> > needed
> > to reproduce it?
> 
> Not sure what would affect it, beyond the normal PROVE_LOCKING=y.

Thanks. Did not have this set. I reproduced the issue and have sent a fix.

David

> 
> This is the .config our CI uses:
> https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/blob/master/kconfig/debug
> 
> > 
> > As far as a solution I think we can copy what __pci_disable_link_state()
> > does
> > and add a bool argument so that we only do down/up on the semaphore when set
> > to
> > true. Since we know we will in be the lock during the bus walk we can set it
> > to
> > false.
> > 
> > David
> > 
> > > 
> > > Hi,
> > > 
> > > This is tripping lockdep on one our CI ADL machines.
> > > 
> > > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt
> > > 
> > > <4>[   13.815380] ============================================
> > > <4>[   13.815382] WARNING: possible recursive locking detected
> > > <4>[   13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
> > > <4>[   13.815386] --------------------------------------------
> > > <4>[   13.815387] swapper/0/1 is trying to acquire lock:
> > > <4>[   13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > > pci_enable_link_state+0x69/0x1d0
> > > <4>[   13.815396] 
> > >                   but task is already holding lock:
> > > <4>[   13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > > pci_walk_bus+0x24/0x90
> > > <4>[   13.815403] 
> > >                   other info that might help us debug this:
> > > <4>[   13.815404]  Possible unsafe locking scenario:
> > > 
> > > <4>[   13.815406]        CPU0
> > > <4>[   13.815407]        ----
> > > <4>[   13.815408]   lock(pci_bus_sem);
> > > <4>[   13.815410]   lock(pci_bus_sem);
> > > <4>[   13.815411] 
> > >                    *** DEADLOCK ***
> > > 
> > > <4>[   13.815413]  May be due to missing lock nesting notation
> > > 
> > > <4>[   13.815414] 2 locks held by swapper/0/1:
> > > <4>[   13.815416]  #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at:
> > > __driver_attach+0xab/0x180
> > > <4>[   13.815422]  #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > > pci_walk_bus+0x24/0x90
> > > <4>[   13.815426] 
> > >                   stack backtrace:
> > > <4>[   13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-
> > > CI_DRM_12814-g4753bbc2a817+ #1
> > > <4>[   13.815431] Hardware name: Intel Corporation Alder Lake Client
> > > Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419
> > > 03/25/2022
> > > <4>[   13.815434] Call Trace:
> > > <4>[   13.815436]  <TASK>
> > > <4>[   13.815437]  dump_stack_lvl+0x64/0xb0
> > > <4>[   13.815443]  __lock_acquire+0x9b5/0x2550
> > > <4>[   13.815461]  lock_acquire+0xd7/0x330
> > > <4>[   13.815463]  ? pci_enable_link_state+0x69/0x1d0
> > > <4>[   13.815466]  down_read+0x3d/0x180
> > > <4>[   13.815480]  ? pci_enable_link_state+0x69/0x1d0
> > > <4>[   13.815482]  pci_enable_link_state+0x69/0x1d0
> > > <4>[   13.815485]  ? __pfx_vmd_pm_enable_quirk+0x10/0x10
> > > <4>[   13.815488]  vmd_pm_enable_quirk+0x49/0xb0
> > > <4>[   13.815490]  pci_walk_bus+0x6d/0x90
> > > <4>[   13.815492]  vmd_probe+0x75f/0x9d0
> > > <4>[   13.815495]  pci_device_probe+0x95/0x120
> > > <4>[   13.815498]  really_probe+0x164/0x3c0
> > > <4>[   13.815500]  ? __pfx___driver_attach+0x10/0x10
> > > <4>[   13.815503]  __driver_probe_device+0x73/0x170
> > > <4>[   13.815506]  driver_probe_device+0x19/0xa0
> > > <4>[   13.815508]  __driver_attach+0xb6/0x180
> > > <4>[   13.815511]  ? __pfx___driver_attach+0x10/0x10
> > > <4>[   13.815513]  bus_for_each_dev+0x77/0xd0
> > > <4>[   13.815516]  bus_add_driver+0x114/0x210
> > > <4>[   13.815518]  driver_register+0x5b/0x110
> > > <4>[   13.815520]  ? __pfx_vmd_drv_init+0x10/0x10
> > > <4>[   13.815523]  do_one_initcall+0x57/0x330
> > > <4>[   13.815527]  kernel_init_freeable+0x181/0x3a0
> > > <4>[   13.815529]  ? __pfx_kernel_init+0x10/0x10
> > > <4>[   13.815532]  kernel_init+0x15/0x120
> > > <4>[   13.815534]  ret_from_fork+0x29/0x50
> > > <4>[   13.815537]  </TASK>
> > > 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-03-21 23:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-20  3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
2023-01-20  3:15 ` [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state() David E. Box
2023-01-20  3:15 ` [PATCH V10 2/4] PCI: vmd: Use PCI_VDEVICE in device list David E. Box
2023-01-20  3:15 ` [PATCH V10 3/4] PCI: vmd: Create feature grouping for client products David E. Box
2023-01-20  3:15 ` [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR David E. Box
2023-03-20 22:56   ` Ville Syrjälä
2023-03-21  2:24     ` David E. Box
2023-03-21 12:38       ` Ville Syrjälä
2023-03-21 23:40         ` David E. Box
2023-01-20  5:28 ` [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware Sathyanarayanan Kuppuswamy
2023-02-02 15:07 ` Lorenzo Pieralisi

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).