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 > > Signed-off-by: David E. Box > > --- > > > > 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 > >