Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f3debbc0956a6a83511b0d90ffc67611cd912b3.camel@linux.intel.com>
Subject: Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     Bjorn Helgaas <helgaas@kernel.org>
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
Date:   Fri, 09 Jun 2023 17:09:52 -0700
In-Reply-To: <20230609224637.GA1267887@bhelgaas>
References: <20230609224637.GA1267887@bhelgaas>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Precedence: bulk

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,
> >=20
> > 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 fro=
m
> > > > 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.
>=20
> > > > +#define VMD_DEVICE_LTR=C2=A00x1003=C2=A0=C2=A0/* 3145728 ns */
> > >=20
> > > It would be nice to know how this value was derived.=C2=A0 But I know=
 we
> > > had this hard-coded value before, so it's not new with this patch.
> >=20
> > 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.
>=20
> Oh, sorry, I meant "why this number was chosen".=C2=A0 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.

>=20
> > > > +static void quirk_intel_vmd(struct pci_dev *pdev)
> > >=20
> > > I think this quirk could possibly stay in
> > > drivers/pci/controller/vmd.c, couldn't it?=C2=A0 It has a lot of
> > > VMD-specific knowledge that it would nice to contain in vmd.c.
> >=20
> > 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.
>=20
> 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.
>=20
> 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.
>=20
> So maybe it needs to be in quirks.c after all.

Okay.

>=20
> 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 de=
vices
below VMD.

David