On 16-Apr-19 5:05 PM, 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.
>
> Thoughts?
>
> Thierry
> --- >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;
I verified this patch on Tegra210.
Reviewed-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx <mailto:mmaddireddy@xxxxxxxxxx>> Tested-by: Manikanta Maddireddy <mmaddireddy@xxxxxxxxxx <mailto:mmaddireddy@xxxxxxxxxx>>
