Message-ID: <fd3212e6f74dee60c66dee821f601e9440c5ae67.camel@linux.intel.com>
Date:   Fri, 09 Jun 2023 15:09:26 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     ville.syrjala@...ux.intel.com, nirmal.patel@...ux.intel.com,
        jonathan.derrick@...ux.dev, lorenzo.pieralisi@....com,
        hch@...radead.org, kw@...ux.com, robh@...nel.org,
        bhelgaas@...gle.com, michael.a.bottini@...el.com,
        rafael@...nel.org, me@...ityamohan.in, linux-pci@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk

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, &ltr_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
> > 

Powered by blists - more mailing lists