Re: [PATCH] PCI: disable nv_msi_ht_cap_quirk_leaf quirk on arm/arm64

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]

On Thu, Apr 18, 2019 at 06:17:04PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 16, 2019 at 01:35:04PM +0200, Thierry Reding wrote:
> > On Mon, Apr 15, 2019 at 08:48:13AM -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 15, 2019 at 11:25:37AM +0200, Nicolas Chauvet wrote:
> > > > This patch disable the use of nv_msi_ht_cap_quirk_leaf quirk on arm and
> > > > arm64 NVIDIA devices such as Tegra
> > > > 
> > > > This fixes the following output:
> > > > "pci 0000:00:01.0: nv_msi_ht_cap_quirk didn't locate host bridge"
> > > > as experienced on a Trimslice device with PCI host bridge enabled
> > > > 
> > > > v3: exclude the quirk for arm and arm64 instead of only for x86
> > > > 
> > > > v2: use __maybe_unused to avoid a warning on nv_msi_ht_cap_quirk_leaf
> > > > 
> > > > Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
> > > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > > > Acked-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  drivers/pci/quirks.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > index a59ad09ce911..1ac65f09d7ee 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -2811,12 +2811,15 @@ static void nv_msi_ht_cap_quirk_all(struct pci_dev *dev)
> > > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all);
> > > >  DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AL, PCI_ANY_ID, nv_msi_ht_cap_quirk_all);
> > > >  
> > > > -static void nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev)
> > > > +static void __maybe_unused nv_msi_ht_cap_quirk_leaf(struct pci_dev *dev)
> > > >  {
> > > >  	return __nv_msi_ht_cap_quirk(dev, 0);
> > > >  }
> > > > +/* HyperTransport is not relevant on theses arches */
> > > > +#if !IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)
> > > 
> > > I don't see any of the previous versions or discussion on the list, so
> > > I can't tell what led to this #ifdef, but I'd prefer to avoid #ifdefs
> > > like this because they're checking for arch at the wrong level.
> > > 
> > > If you're seeing that warning message, apparently your system contains
> > > an Nvidia device that has an HT_CAPTYPE_MSI_MAPPING capability, so
> > > HyperTransport is at least in the picture, even on arm/arm64.
> > > 
> > > The quirk assumes a host bridge at 00:00.0, which is really a
> > > device-specific assumption, or maybe something specific to
> > > HyperTransport.  It's certainly not required by the PCI specs, which
> > > don't require a host bridge to be visible as a PCI device at all.
> > > 
> > > I'd be tempted to just change that pci_warn() to a pci_info() or maybe
> > > even remove it altogether.  It would be nice to have the complete
> > > "lspci -vv" and dmesg log archived for future reference, since this
> > > quirk has such a broad reach (matching PCI_ANY_ID) and it has a long
> > > and ugly history already.
> > 
> > I've been thinking about this a bit and it seems to me like the
> > intention of the quirk is to make sure that all devices have their HT
> > MSI mapping capabilities disabled if the system doesn't support HT.
> > As you said, the fact that a host bridge may not be present isn't an
> > error, however, it also means that we have to assume that HT is not
> > supported in such cases and make sure the HT MSI mapping capability gets
> > disabled for all devices.
> > 
> > The following patch tries to address this and gets rid of the warning on
> > Tegra systems.
> 
> Also interesting that Nicolas' original patch took this quirk
> completely out of the picture for ARM/ARM64, while your patch actively
> disables HT MSI mapping if there's no HT host bridge.

I should note here that this is just my interpretation of what I think
the quirk was originally meant to be. I don't have any hardware to test
this on, nor do I know much (well, nothing, to be frank) about Hyper-
Transport.

So my thinking was that if there's no HT host bridge, then it doesn't
make any sense to enable HT MSI mapping either, which I think may have
been the intent of this quirk: make sure that if the system is not HT
capable, that none of the PCI devices try to use HT MSI mapping.

Thierry

> If HT MSI Mapping Capability is enabled, the device is supposed to map
> MSIs into HyperTransport interrupt messages (HyperTransport I/O Link
> spec r3.10e, sec 7.12).  I don't know what that even means if the
> device is on a PCIe link instead of an HT chain.
> 
> > --- >8 ---
> > From 69fb3b269cc852964385436694d79765d5c1cab3 Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <treding@nvidia.com>
> > Date: Tue, 16 Apr 2019 13:27:53 +0200
> > Subject: [PATCH] PCI: Be less noisy if no HT host bridge exists
> > 
> > Embedded systems will typically not expose a host bridge's configuration
> > space, so such setups must be assumed not to support HyperTransport, and
> > consequently the HT MSI mapping capability should be disabled for all
> > devices.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/pci/quirks.c | 34 +++++++++++++++++++---------------
> >  1 file changed, 19 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index f9cd4d432e05..705891c88073 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -2792,24 +2792,28 @@ static void __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all)
> >  	 */
> >  	host_bridge = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus), 0,
> >  						  PCI_DEVFN(0, 0));
> > -	if (host_bridge == NULL) {
> > -		pci_warn(dev, "nv_msi_ht_cap_quirk didn't locate host bridge\n");
> > -		return;
> > -	}
> > -
> > -	pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE);
> > -	if (pos != 0) {
> > -		/* Host bridge is to HT */
> > -		if (found == 1) {
> > -			/* it is not enabled, try to enable it */
> > -			if (all)
> > -				ht_enable_msi_mapping(dev);
> > -			else
> > -				nv_ht_enable_msi_mapping(dev);
> > +	if (host_bridge) {
> > +		pos = pci_find_ht_capability(host_bridge, HT_CAPTYPE_SLAVE);
> > +		if (pos != 0) {
> > +			/* Host bridge is to HT */
> > +			if (found == 1) {
> > +				/* it is not enabled, try to enable it */
> > +				if (all)
> > +					ht_enable_msi_mapping(dev);
> > +				else
> > +					nv_ht_enable_msi_mapping(dev);
> > +			}
> > +			goto out;
> >  		}
> > -		goto out;
> >  	}
> >  
> > +	/*
> > +	 * Some systems, such as embedded devices, may not expose the host
> > +	 * bridge's configuration space. Assume such systems to not support
> > +	 * HyperTransport and disable the HT MSI mapping capability for all
> > +	 * devices.
> > +	 */
> > +
> >  	/* HT MSI is not enabled */
> >  	if (found == 1)
> >  		goto out;
> > -- 
> > 2.21.0
> > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]