Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h

From: Marc Zyngier
Date: Tue Nov 03 2020 - 06:41:28 EST

Next message: Peilin Ye: "Re: [PATCH v2 1/1] Fonts: Replace discarded const qualifier"
Previous message: Maxime Ripard: "Re: [PATCH v1 1/1] ARM: dts: sun8i: h3: Add initial NanoPi R1 support"
In reply to: Thomas Gleixner: "Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h"
Next in thread: Thomas Gleixner: "Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2020-11-03 10:31, Thomas Gleixner wrote:

On Tue, Nov 03 2020 at 09:54, Marc Zyngier wrote:

On 2020-11-02 22:18, Thomas Gleixner wrote:

So we really need some other solution and removing the warning is not
an option. If MSI is enabled then we want to get a warning when a PCI
device has no MSI domain associated. Explicitly expressing the PCIE
brigde misfeature of not supporting MSI is way better than silently
returning an error code which is swallowed anyway.

I don't disagree here, though the PCI_MSI_ARCH_FALLBACKS mechanism
makes it more difficult to establish.

Only for the few leftovers which implement msi_controller, i.e.

drivers/pci/controller/pci-hyperv.c
drivers/pci/controller/pci-tegra.c
drivers/pci/controller/pcie-rcar-host.c
drivers/pci/controller/pcie-xilinx.c

The architectures which select PCI_MSI_ARCH_FALLBACKS are:

arch/ia64/Kconfig: select PCI_MSI_ARCH_FALLBACKS if PCI_MSI
arch/mips/Kconfig: select PCI_MSI_ARCH_FALLBACKS if PCI_MSI
arch/powerpc/Kconfig: select PCI_MSI_ARCH_FALLBACKS if PCI_MSI
arch/s390/Kconfig: select PCI_MSI_ARCH_FALLBACKS if PCI_MSI
arch/sparc/Kconfig: select PCI_MSI_ARCH_FALLBACKS if PCI_MSI

implement arch_setup_msi_irq() which makes it magically work :)

Whatever the preferred way is via flags at host probe time or flagging
it post probe I don't care much as long as it is consistent.

Host probe time is going to require some changes in the core PCI api,
as everything that checks for a MSI domain is based on the pci_bus
structure, which is only allocated much later.

Yeah, it's nasty. One possible solution is to add flags or a callback to
pci_ops, but it's not pretty either.

I think we should go with the 'mark it after pci_host_probe()' hack for
5.10-rc. The real fix will be larger and go into 5.11.

Thoughts?

We can do that, although I worried that it isn't 100% reliable:

pci_host_probe() ends up calling pci_add_device(), and will start
probing devices if the endpoint drivers have already registered
with the core code, long before the flag gets set.

Here's what I've hacked together for a guest that doesn't have
any MSI capability:

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 6ce34a1deecb..7dd5145cd38d 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -55,6 +55,7 @@ int pci_host_common_probe(struct platform_device *pdev)
struct pci_host_bridge *bridge;
struct pci_config_window *cfg;
const struct pci_ecam_ops *ops;
+ int ret;

ops = of_device_get_match_data(&pdev->dev);
if (!ops)
@@ -80,7 +81,10 @@ int pci_host_common_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, bridge);

- return pci_host_probe(bridge);
+ ret = pci_host_probe(bridge);
+ bridge->bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+
+ return ret;
}
EXPORT_SYMBOL_GPL(pci_host_common_probe);

(plus another hack to get the host controller to initialise a bit
later, though building it as a module will achieve the same thing):

[ 0.369114] 9pnet: Installing 9P2000 support
[ 0.369807] mpls_gso: MPLS GSO support
[ 0.370512] registered taskstats version 1
[ 0.371204] Loading compiled-in X.509 certificates
[ 0.371988] zswap: loaded using pool lzo/zbud
[ 0.373041] pci-host-generic 40000000.pci: host bridge /pci ranges:
[ 0.374045] pci-host-generic 40000000.pci: IO 0x0000000000..0x000000ffff -> 0x0000000000
[ 0.375458] pci-host-generic 40000000.pci: MEM 0x0041000000..0x007fffffff -> 0x0041000000
[ 0.376848] pci-host-generic 40000000.pci: ECAM at [mem 0x40000000-0x40ffffff] for [bus 00]
[ 0.378204] pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00
[ 0.379316] pci_bus 0000:00: root bus resource [bus 00]
[ 0.380146] pci_bus 0000:00: root bus resource [io 0x0000-0xffff]
[ 0.381131] pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff]
[ 0.382267] pci 0000:00:00.0: [1af4:1009] type 00 class 0xff0000
[ 0.383369] pci 0000:00:00.0: reg 0x10: [io 0x6200-0x62ff]
[ 0.384286] pci 0000:00:00.0: reg 0x14: [mem 0x41000000-0x410000ff]
[ 0.385324] pci 0000:00:00.0: reg 0x18: [mem 0x41000200-0x410003ff]
[ 0.386680] pci 0000:00:01.0: [1af4:1009] type 00 class 0xff0000
[ 0.387778] pci 0000:00:01.0: reg 0x10: [io 0x6300-0x63ff]
[ 0.388696] pci 0000:00:01.0: reg 0x14: [mem 0x41000400-0x410004ff]
[ 0.389730] pci 0000:00:01.0: reg 0x18: [mem 0x41000600-0x410007ff]
[ 0.391070] pci 0000:00:02.0: [1af4:1000] type 00 class 0x020000
[ 0.392212] pci 0000:00:02.0: reg 0x10: [io 0x6400-0x64ff]
[ 0.393137] pci 0000:00:02.0: reg 0x14: [mem 0x41000800-0x410008ff]
[ 0.394163] pci 0000:00:02.0: reg 0x18: [mem 0x41000a00-0x41000bff]
[ 0.395678] pci 0000:00:00.0: BAR 2: assigned [mem 0x41000000-0x410001ff]
[ 0.396762] pci 0000:00:01.0: BAR 2: assigned [mem 0x41000200-0x410003ff]
[ 0.397851] pci 0000:00:02.0: BAR 2: assigned [mem 0x41000400-0x410005ff]
[ 0.398934] pci 0000:00:00.0: BAR 0: assigned [io 0x1000-0x10ff]
[ 0.400014] pci 0000:00:00.0: BAR 1: assigned [mem 0x41000600-0x410006ff]
[ 0.401105] pci 0000:00:01.0: BAR 0: assigned [io 0x1100-0x11ff]
[ 0.402080] pci 0000:00:01.0: BAR 1: assigned [mem 0x41000700-0x410007ff]
[ 0.403185] pci 0000:00:02.0: BAR 0: assigned [io 0x1200-0x12ff]
[ 0.404156] pci 0000:00:02.0: BAR 1: assigned [mem 0x41000800-0x410008ff]
[ 0.405344] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
[ 0.406569] ------------[ cut here ]------------
[ 0.407347] WARNING: CPU: 1 PID: 1 at include/linux/msi.h:213 __pci_enable_msix_range+0x680/0x720
[ 0.408884] Modules linked in:
[ 0.409429] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc2-dirty #2117
[ 0.410646] Hardware name: linux,dummy-virt (DT)
[ 0.411463] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[ 0.412505] pc : __pci_enable_msix_range+0x680/0x720
[ 0.413380] lr : __pci_enable_msix_range+0x394/0x720
[ 0.414244] sp : ffffffc0119ab3d0
[ 0.414830] x29: ffffffc0119ab3d0 x28: 0000000000000002
[ 0.415766] x27: ffffff804107b0b8 x26: ffffff804107b000
[ 0.416689] x25: ffffff80410acb00 x24: 0000000000000000
[ 0.417623] x23: ffffff80410ac480 x22: ffffff804107b000
[ 0.418549] x21: ffffff804107b2e8 x20: 0000000000000014
[ 0.419482] x19: 0000000000000002 x18: 00000000fffffffc
[ 0.420408] x17: 000000002ce4d3e3 x16: 00000000a4f09d66
[ 0.421342] x15: 0000000000000020 x14: ffffffffffffffff
[ 0.422268] x13: 0000000041001000 x12: ffffffc0119cf000
[ 0.423201] x11: ffffffc010000000 x10: ffffffc0119cd000
[ 0.424127] x9 : ffffffc010628ad4 x8 : 0000000000000000
[ 0.425063] x7 : 0000000000000000 x6 : 000000000000003f
[ 0.425989] x5 : 0000000000000040 x4 : ffffff804107b2e8
[ 0.426914] x3 : 0000000000000004 x2 : 0000000000000000
[ 0.427855] x1 : 0000000000000000 x0 : 0000000000000000
[ 0.428788] Call trace:
[ 0.429226] __pci_enable_msix_range+0x680/0x720
[ 0.430033] pci_alloc_irq_vectors_affinity+0xcc/0x144
[ 0.430932] vp_find_vqs_msix+0xdc/0x414
[ 0.431631] vp_find_vqs+0x54/0x1c0
[ 0.432245] p9_virtio_probe+0xa8/0x374

This is admittedly a contrived example, but I'm not convinced it is
completely unlikely.

M.
--
Jazz is not dead. It just smells funny...

Next message: Peilin Ye: "Re: [PATCH v2 1/1] Fonts: Replace discarded const qualifier"
Previous message: Maxime Ripard: "Re: [PATCH v1 1/1] ARM: dts: sun8i: h3: Add initial NanoPi R1 support"
In reply to: Thomas Gleixner: "Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h"
Next in thread: Thomas Gleixner: "Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]