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@xxxxxxxxx>
> > > Reviewed-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx>
> > > Acked-by: Thierry Reding <treding@xxxxxxxxxx>
> > > ---
> > > 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.
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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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
>
