From: Bjorn Helgaas <helgaas@kernel.org>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: me@adhityamohan.in, kw@linux.com, lorenzo.pieralisi@arm.com,
robh@kernel.org, linux-pci@vger.kernel.org,
intel-gfx@lists.freedesktop.org, rafael@kernel.org,
linux-kernel@vger.kernel.org, hch@infradead.org,
jonathan.derrick@linux.dev, bhelgaas@google.com,
nirmal.patel@linux.intel.com, michael.a.bottini@intel.com
Subject: Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
Date: Fri, 9 Jun 2023 17:46:37 -0500 [thread overview]
Message-ID: <20230609224637.GA1267887@bhelgaas> (raw)
In-Reply-To: <fd3212e6f74dee60c66dee821f601e9440c5ae67.camel@linux.intel.com>
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
next prev parent reply other threads:[~2023-06-09 22:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-06-10 0:09 ` David E. Box
2023-10-24 17:08 ` Ville Syrjälä
2023-10-25 8:38 ` Ilpo Järvinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230609224637.GA1267887@bhelgaas \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=david.e.box@linux.intel.com \
--cc=hch@infradead.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jonathan.derrick@linux.dev \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=me@adhityamohan.in \
--cc=michael.a.bottini@intel.com \
--cc=nirmal.patel@linux.intel.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).