linux-kernel - Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

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