* [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
@ 2023-04-11 21:33 David E. Box
2023-06-08 20:52 ` [Intel-gfx] " Bjorn Helgaas
2023-10-24 17:08 ` Ville Syrjälä
0 siblings, 2 replies; 7+ messages in thread
From: David E. Box @ 2023-04-11 21:33 UTC (permalink / raw)
To: david.e.box, ville.syrjala, nirmal.patel, jonathan.derrick,
lorenzo.pieralisi, hch, kw, robh, bhelgaas, michael.a.bottini,
rafael
Cc: me, linux-pci, linux-kernel, intel-gfx
In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
LTR") the VMD driver calls pci_enabled_link_state as a callback from
pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
DECLARE_PCI_FIXUP_FINAL.
Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
V2 - Instead of adding a lock flag argument to pci_enabled_link_state, move
the fix to quirks.c
drivers/pci/controller/vmd.c | 55 +--------------------------
drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++++++++++++
2 files changed, 73 insertions(+), 54 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 990630ec57c6..47fa3e5f2dc5 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -66,22 +66,11 @@ 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_BIOS_PM_QUIRK)
+ VMD_FEAT_OFFSET_FIRST_VECTOR)
static DEFINE_IDA(vmd_instance_ida);
@@ -724,46 +713,6 @@ 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, <r_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;
@@ -936,8 +885,6 @@ 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
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44cab813bf95..2d86623f96e3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6023,3 +6023,75 @@ 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
+
+#ifdef CONFIG_VMD
+/*
+ * Enable ASPM on the PCIE root ports under VMD 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.
+ */
+#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
+static void quirk_intel_vmd(struct pci_dev *pdev)
+{
+ struct pci_dev *parent;
+ u16 ltr = VMD_DEVICE_LTR;
+ u32 ltr_reg;
+ int pos;
+
+ /* Check in VMD domain */
+ if (pci_domain_nr(pdev->bus) < 0x10000)
+ return;
+
+ /* Get Root Port */
+ parent = pci_upstream_bridge(pdev);
+ if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
+ return;
+
+ /* Get VMD Host Bridge */
+ parent = to_pci_dev(parent->dev.parent);
+ if (!parent)
+ return;
+
+ /* Get RAID controller */
+ parent = to_pci_dev(parent->dev.parent);
+ if (!parent)
+ return;
+
+ switch (parent->device) {
+ case 0x467f:
+ case 0x4c3d:
+ case 0xa77f:
+ case 0x7d0b:
+ case 0xad0b:
+ case 0x9a0b:
+ break;
+ default:
+ return;
+ }
+
+ pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
+
+ pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+ if (!pos)
+ return;
+
+ /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it */
+ pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
+ if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
+ return;
+
+ /*
+ * Set the LTR 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, "LTR set by VMD PCI quick\n");
+
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
+ PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
+#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-04-11 21:33 [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk David E. Box
@ 2023-06-08 20:52 ` Bjorn Helgaas
2023-06-09 22:09 ` David E. Box
2023-10-24 17:08 ` Ville Syrjälä
1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2023-06-08 20:52 UTC (permalink / raw)
To: David E. Box
Cc: ville.syrjala, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me, linux-pci,
intel-gfx, linux-kernel
On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> LTR") the VMD driver calls pci_enabled_link_state as a callback from
> pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> DECLARE_PCI_FIXUP_FINAL.
s/pci_enabled_link_state/pci_enable_link_state/
Add "()" after pci_enable_link_state() and pci_bus_walk() to make it
obvious they're functions.
> ...
> +++ b/drivers/pci/quirks.c
> @@ -6023,3 +6023,75 @@ 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
> +
> +#ifdef CONFIG_VMD
> +/*
> + * Enable ASPM on the PCIE root ports under VMD 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.
s/PCIE/PCIe/ to match spec usage.
> + */
> +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
It would be nice to know how this value was derived. But I know we
had this hard-coded value before, so it's not new with this patch.
> +static void quirk_intel_vmd(struct pci_dev *pdev)
I think this quirk could possibly stay in
drivers/pci/controller/vmd.c, couldn't it? It has a lot of
VMD-specific knowledge that it would nice to contain in vmd.c.
> +{
> + struct pci_dev *parent;
> + u16 ltr = VMD_DEVICE_LTR;
I don't think "ltr" is an improvement over using "VMD_DEVICE_LTR"
below.
> + u32 ltr_reg;
> + int pos;
> +
> + /* Check in VMD domain */
> + if (pci_domain_nr(pdev->bus) < 0x10000)
> + return;
If in vmd.c, maybe could identify devices under a VMD by checking
pdev->bus->ops as vmd_acpi_find_companion() does?
> + /* Get Root Port */
> + parent = pci_upstream_bridge(pdev);
> + if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
> + return;
> +
> + /* Get VMD Host Bridge */
> + parent = to_pci_dev(parent->dev.parent);
> + if (!parent)
> + return;
> +
> + /* Get RAID controller */
> + parent = to_pci_dev(parent->dev.parent);
> + if (!parent)
> + return;
> +
> + switch (parent->device) {
> + case 0x467f:
> + case 0x4c3d:
> + case 0xa77f:
> + case 0x7d0b:
> + case 0xad0b:
> + case 0x9a0b:
> + break;
> + default:
> + return;
> + }
> +
> + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
Seems like you would want to set LTR *before* enabling the link
states?
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> + if (!pos)
> + return;
> +
> + /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it */
> + pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> + if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> + return;
> +
> + /*
> + * Set the LTR 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.
This comment suggests that the LTR value is platform-dependent, which
is what I would expect, but the code and the hard-coded VMD_DEVICE_LTR
value don't have any platform dependencies. Again, nothing new in
*this* patch; that's true in the current tree, too.
> + ltr_reg = (ltr << 16) | ltr;
> + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> + pci_info(pdev, "LTR set by VMD PCI quick\n");
s/quick/quirk/
> +
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> + PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
> +#endif
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-06-08 20:52 ` [Intel-gfx] " Bjorn Helgaas
@ 2023-06-09 22:09 ` David E. Box
2023-06-09 22:46 ` Bjorn Helgaas
0 siblings, 1 reply; 7+ messages in thread
From: David E. Box @ 2023-06-09 22:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: ville.syrjala, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
hch, kw, robh, bhelgaas, michael.a.bottini, rafael, me, linux-pci,
intel-gfx, linux-kernel
Hi Bjorn,
On Thu, 2023-06-08 at 15:52 -0500, Bjorn Helgaas wrote:
> On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> > In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> > LTR") the VMD driver calls pci_enabled_link_state as a callback from
> > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> > warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> > DECLARE_PCI_FIXUP_FINAL.
>
> s/pci_enabled_link_state/pci_enable_link_state/
>
> Add "()" after pci_enable_link_state() and pci_bus_walk() to make it
> obvious they're functions.
>
> > ...
> > +++ b/drivers/pci/quirks.c
> > @@ -6023,3 +6023,75 @@ 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
> > +
> > +#ifdef CONFIG_VMD
> > +/*
> > + * Enable ASPM on the PCIE root ports under VMD 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.
>
> s/PCIE/PCIe/ to match spec usage.
>
> > + */
> > +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
>
> It would be nice to know how this value was derived. But I know we
> had this hard-coded value before, so it's not new with this patch.
Do you mean to show the multiplier that determines that value or to say why this
particular number was chosen? For the latter, it the largest that could be set
(given the multipier options) that will allow the SoC to get to it's lowest
power state. And it's the same so far on all the SoCs covered by the VMD driver.
>
> > +static void quirk_intel_vmd(struct pci_dev *pdev)
>
> I think this quirk could possibly stay in
> drivers/pci/controller/vmd.c, couldn't it? It has a lot of
> VMD-specific knowledge that it would nice to contain in vmd.c.
I may have misunderstood your comment on V1 then. But you suggested that this
would be typically done as PCI_FIXUP so that the PCI core could call it and we
could avoid the locking issue that was seen while walking the bus in vmd.c.
https://lore.kernel.org/lkml/ab9bf3032ed46fc0586e089edc5aac6e71b331d8.camel@linux.intel.com/T/#m09dc05ef56b8d9f104f91594f582251b6088d24d
>
> > +{
> > + struct pci_dev *parent;
> > + u16 ltr = VMD_DEVICE_LTR;
>
> I don't think "ltr" is an improvement over using "VMD_DEVICE_LTR"
> below.
>
> > + u32 ltr_reg;
> > + int pos;
> > +
> > + /* Check in VMD domain */
> > + if (pci_domain_nr(pdev->bus) < 0x10000)
> > + return;
>
> If in vmd.c, maybe could identify devices under a VMD by checking
> pdev->bus->ops as vmd_acpi_find_companion() does?
>
> > + /* Get Root Port */
> > + parent = pci_upstream_bridge(pdev);
> > + if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
> > + return;
> > +
> > + /* Get VMD Host Bridge */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + /* Get RAID controller */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + switch (parent->device) {
> > + case 0x467f:
> > + case 0x4c3d:
> > + case 0xa77f:
> > + case 0x7d0b:
> > + case 0xad0b:
> > + case 0x9a0b:
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
>
> Seems like you would want to set LTR *before* enabling the link
> states?
Yes that would be better. We'll still want to check if the LTR is set first
though because if it is then we don't need to do either.
David
>
> > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > + if (!pos)
> > + return;
> > +
> > + /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> > */
> > + pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> > + if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> > + return;
> > +
> > + /*
> > + * Set the LTR 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.
>
> This comment suggests that the LTR value is platform-dependent, which
> is what I would expect, but the code and the hard-coded VMD_DEVICE_LTR
> value don't have any platform dependencies. Again, nothing new in
> *this* patch; that's true in the current tree, too.
>
> > + ltr_reg = (ltr << 16) | ltr;
> > + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> > + pci_info(pdev, "LTR set by VMD PCI quick\n");
>
> s/quick/quirk/
>
> > +
> > +}
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> > + PCI_CLASS_STORAGE_EXPRESS, 0,
> > quirk_intel_vmd);
> > +#endif
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-06-09 22:09 ` David E. Box
@ 2023-06-09 22:46 ` Bjorn Helgaas
2023-06-10 0:09 ` David E. Box
0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2023-06-09 22:46 UTC (permalink / raw)
To: David E. Box
Cc: me, kw, lorenzo.pieralisi, robh, linux-pci, intel-gfx, rafael,
linux-kernel, hch, jonathan.derrick, bhelgaas, nirmal.patel,
michael.a.bottini
On Fri, Jun 09, 2023 at 03:09:26PM -0700, David E. Box wrote:
> Hi Bjorn,
>
> On Thu, 2023-06-08 at 15:52 -0500, Bjorn Helgaas wrote:
> > On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> > > In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> > > LTR") the VMD driver calls pci_enabled_link_state as a callback from
> > > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> > > warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> > > DECLARE_PCI_FIXUP_FINAL.
> > > +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
> >
> > It would be nice to know how this value was derived. But I know we
> > had this hard-coded value before, so it's not new with this patch.
>
> Do you mean to show the multiplier that determines that value or to
> say why this particular number was chosen? For the latter, it the
> largest that could be set (given the multipier options) that will
> allow the SoC to get to it's lowest power state. And it's the same
> so far on all the SoCs covered by the VMD driver.
Oh, sorry, I meant "why this number was chosen". PCIe r6.0, sec
7.8.2, says this capability allows software to provide "platform
latency information," so I assume this is somehow dependent on
platform, but I really don't understand the details of how LTR works,
and we didn't have an explanation before, so this was just a "if you
happen to know, it might be useful here" comment.
> > > +static void quirk_intel_vmd(struct pci_dev *pdev)
> >
> > I think this quirk could possibly stay in
> > drivers/pci/controller/vmd.c, couldn't it? It has a lot of
> > VMD-specific knowledge that it would nice to contain in vmd.c.
>
> I may have misunderstood your comment on V1 then. But you suggested
> that this would be typically done as PCI_FIXUP so that the PCI core
> could call it and we could avoid the locking issue that was seen
> while walking the bus in vmd.c.
Right, I think it makes sense to be a DECLARE_PCI_FIXUP_CLASS_FINAL(),
but I was thinking that it could be implemented in vmd.c and still be
called by the PCI core.
But now I'm uncertain since vmd.c can be compiled as a module, and I'm
not sure how that could work, since pci_fixup_device() calls things in
the __start_pci_fixups_final[] table, and I don't see how loading a
module would insert the fixup entry into that table.
So maybe it needs to be in quirks.c after all.
I think my only remaining questions here are about how to identify
devices below VMD and the order of enabling ASPM states vs setting
LTR.
Bjorn
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-06-09 22:46 ` Bjorn Helgaas
@ 2023-06-10 0:09 ` David E. Box
0 siblings, 0 replies; 7+ messages in thread
From: David E. Box @ 2023-06-10 0:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: me, kw, lorenzo.pieralisi, robh, linux-pci, intel-gfx, rafael,
linux-kernel, hch, jonathan.derrick, bhelgaas, nirmal.patel,
michael.a.bottini
On Fri, 2023-06-09 at 17:46 -0500, Bjorn Helgaas wrote:
> On Fri, Jun 09, 2023 at 03:09:26PM -0700, David E. Box wrote:
> > Hi Bjorn,
> >
> > On Thu, 2023-06-08 at 15:52 -0500, Bjorn Helgaas wrote:
> > > On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> > > > In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> > > > LTR") the VMD driver calls pci_enabled_link_state as a callback from
> > > > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a
> > > > lockdep
> > > > warning. Instead of doing the pci_bus_walk, move the fix to quirks.c
> > > > using
> > > > DECLARE_PCI_FIXUP_FINAL.
>
> > > > +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
> > >
> > > It would be nice to know how this value was derived. But I know we
> > > had this hard-coded value before, so it's not new with this patch.
> >
> > Do you mean to show the multiplier that determines that value or to
> > say why this particular number was chosen? For the latter, it the
> > largest that could be set (given the multipier options) that will
> > allow the SoC to get to it's lowest power state. And it's the same
> > so far on all the SoCs covered by the VMD driver.
>
> Oh, sorry, I meant "why this number was chosen". PCIe r6.0, sec
> 7.8.2, says this capability allows software to provide "platform
> latency information," so I assume this is somehow dependent on
> platform, but I really don't understand the details of how LTR works,
> and we didn't have an explanation before, so this was just a "if you
> happen to know, it might be useful here" comment.
Sure.
>
> > > > +static void quirk_intel_vmd(struct pci_dev *pdev)
> > >
> > > I think this quirk could possibly stay in
> > > drivers/pci/controller/vmd.c, couldn't it? It has a lot of
> > > VMD-specific knowledge that it would nice to contain in vmd.c.
> >
> > I may have misunderstood your comment on V1 then. But you suggested
> > that this would be typically done as PCI_FIXUP so that the PCI core
> > could call it and we could avoid the locking issue that was seen
> > while walking the bus in vmd.c.
>
> Right, I think it makes sense to be a DECLARE_PCI_FIXUP_CLASS_FINAL(),
> but I was thinking that it could be implemented in vmd.c and still be
> called by the PCI core.
>
> But now I'm uncertain since vmd.c can be compiled as a module, and I'm
> not sure how that could work, since pci_fixup_device() calls things in
> the __start_pci_fixups_final[] table, and I don't see how loading a
> module would insert the fixup entry into that table.
>
> So maybe it needs to be in quirks.c after all.
Okay.
>
> I think my only remaining questions here are about how to identify
> devices below VMD and the order of enabling ASPM states vs setting
> LTR.
Agree on setting the LTR first. I'll also look at other ways to identify devices
below VMD.
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-04-11 21:33 [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk David E. Box
2023-06-08 20:52 ` [Intel-gfx] " Bjorn Helgaas
@ 2023-10-24 17:08 ` Ville Syrjälä
2023-10-25 8:38 ` Ilpo Järvinen
1 sibling, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2023-10-24 17:08 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 Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> LTR") the VMD driver calls pci_enabled_link_state as a callback from
> pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> DECLARE_PCI_FIXUP_FINAL.
What happened to this patch? We're still carrying a local fix
for this in drm-tip...
>
> Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>
> V2 - Instead of adding a lock flag argument to pci_enabled_link_state, move
> the fix to quirks.c
>
> drivers/pci/controller/vmd.c | 55 +--------------------------
> drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 73 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 990630ec57c6..47fa3e5f2dc5 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -66,22 +66,11 @@ 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_BIOS_PM_QUIRK)
> + VMD_FEAT_OFFSET_FIRST_VECTOR)
>
> static DEFINE_IDA(vmd_instance_ida);
>
> @@ -724,46 +713,6 @@ 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, <r_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;
> @@ -936,8 +885,6 @@ 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
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..2d86623f96e3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6023,3 +6023,75 @@ 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
> +
> +#ifdef CONFIG_VMD
> +/*
> + * Enable ASPM on the PCIE root ports under VMD 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.
> + */
> +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
> +static void quirk_intel_vmd(struct pci_dev *pdev)
> +{
> + struct pci_dev *parent;
> + u16 ltr = VMD_DEVICE_LTR;
> + u32 ltr_reg;
> + int pos;
> +
> + /* Check in VMD domain */
> + if (pci_domain_nr(pdev->bus) < 0x10000)
> + return;
> +
> + /* Get Root Port */
> + parent = pci_upstream_bridge(pdev);
> + if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
> + return;
> +
> + /* Get VMD Host Bridge */
> + parent = to_pci_dev(parent->dev.parent);
> + if (!parent)
> + return;
> +
> + /* Get RAID controller */
> + parent = to_pci_dev(parent->dev.parent);
> + if (!parent)
> + return;
> +
> + switch (parent->device) {
> + case 0x467f:
> + case 0x4c3d:
> + case 0xa77f:
> + case 0x7d0b:
> + case 0xad0b:
> + case 0x9a0b:
> + break;
> + default:
> + return;
> + }
> +
> + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> + if (!pos)
> + return;
> +
> + /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it */
> + pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> + if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> + return;
> +
> + /*
> + * Set the LTR 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, "LTR set by VMD PCI quick\n");
> +
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> + PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
> +#endif
> --
> 2.34.1
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
2023-10-24 17:08 ` Ville Syrjälä
@ 2023-10-25 8:38 ` Ilpo Järvinen
0 siblings, 0 replies; 7+ messages in thread
From: Ilpo Järvinen @ 2023-10-25 8:38 UTC (permalink / raw)
To: Ville Syrjälä
Cc: David E. Box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi,
hch, kw, robh, bhelgaas, michael.a.bottini, Rafael J. Wysocki, me,
linux-pci, LKML, intel-gfx
[-- Attachment #1: Type: text/plain, Size: 7047 bytes --]
On Tue, 24 Oct 2023, Ville Syrjälä wrote:
> On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> > In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> > LTR") the VMD driver calls pci_enabled_link_state as a callback from
> > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> > warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> > DECLARE_PCI_FIXUP_FINAL.
>
> What happened to this patch? We're still carrying a local fix
> for this in drm-tip...
>
> >
> > Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >
> > V2 - Instead of adding a lock flag argument to pci_enabled_link_state, move
> > the fix to quirks.c
> >
> > drivers/pci/controller/vmd.c | 55 +--------------------------
> > drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 73 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 990630ec57c6..47fa3e5f2dc5 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -66,22 +66,11 @@ 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_BIOS_PM_QUIRK)
> > + VMD_FEAT_OFFSET_FIRST_VECTOR)
> >
> > static DEFINE_IDA(vmd_instance_ida);
> >
> > @@ -724,46 +713,6 @@ 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, <r_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;
> > @@ -936,8 +885,6 @@ 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
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 44cab813bf95..2d86623f96e3 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6023,3 +6023,75 @@ 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
> > +
> > +#ifdef CONFIG_VMD
> > +/*
> > + * Enable ASPM on the PCIE root ports under VMD 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.
> > + */
> > +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
This should be defined using FIELD_PREP()s. It is better to construct both
snoop and nosnoop registers here and not do the shift below at all.
There are new defines for the nosnoop reg in pci/field-get branch for the
nosnoop reg fields.
--
i.
> > +static void quirk_intel_vmd(struct pci_dev *pdev)
> > +{
> > + struct pci_dev *parent;
> > + u16 ltr = VMD_DEVICE_LTR;
> > + u32 ltr_reg;
> > + int pos;
> > +
> > + /* Check in VMD domain */
> > + if (pci_domain_nr(pdev->bus) < 0x10000)
> > + return;
> > +
> > + /* Get Root Port */
> > + parent = pci_upstream_bridge(pdev);
> > + if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
> > + return;
> > +
> > + /* Get VMD Host Bridge */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + /* Get RAID controller */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + switch (parent->device) {
> > + case 0x467f:
> > + case 0x4c3d:
> > + case 0xa77f:
> > + case 0x7d0b:
> > + case 0xad0b:
> > + case 0x9a0b:
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > +
> > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > + if (!pos)
> > + return;
> > +
> > + /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it */
> > + pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> > + if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> > + return;
> > +
> > + /*
> > + * Set the LTR 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, "LTR set by VMD PCI quick\n");
> > +
> > +}
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> > + PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
> > +#endif
> > --
> > 2.34.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-10-25 8:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-11 21:33 [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk David E. Box
2023-06-08 20:52 ` [Intel-gfx] " Bjorn Helgaas
2023-06-09 22:09 ` David E. Box
2023-06-09 22:46 ` Bjorn Helgaas
2023-06-10 0:09 ` David E. Box
2023-10-24 17:08 ` Ville Syrjälä
2023-10-25 8:38 ` Ilpo Järvinen
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 read-only IMAP folder(s) and NNTP newsgroup(s).