Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Tue, Nov 03 2020 at 11:41, Marc Zyngier wrote: > On 2020-11-03 10:31, Thomas Gleixner wrote: > 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. Bah, you're right. What a mess.
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
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(>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 4000.pci: host bridge /pci ranges: [0.374045] pci-host-generic 4000.pci: IO 0x00..0x00 -> 0x00 [0.375458] pci-host-generic 4000.pci: MEM 0x004100..0x007fff -> 0x004100 [0.376848] pci-host-generic 4000.pci: ECAM at [mem 0x4000-0x40ff] for [bus 00] [0.378204] pci-host-generic 4000.pci: PCI host bridge to bus :00 [0.379316] pci_bus :00: root bus resource [bus 00] [0.380146] pci_bus :00: root bus resource [io 0x-0x] [0.381131] pci_bus :00: root bus resource [mem 0x4100-0x7fff] [0.382267] pci :00:00.0: [1af4:1009] type 00 class 0xff [0.383369] pci :00:00.0: reg 0x10: [io 0x6200-0x62ff] [0.384286] pci :00:00.0: reg 0x14: [mem 0x4100-0x41ff] [0.385324] pci :00:00.0: reg 0x18: [mem 0x41000200-0x410003ff] [0.386680] pci :00:01.0: [1af4:1009] type 00 class 0xff [0.387778] pci :00:01.0: reg 0x10: [io 0x6300-0x63ff] [0.388696] pci :00:01.0: reg 0x14: [mem 0x41000400-0x410004ff] [0.389730] pci :00:01.0: reg 0x18: [mem 0x41000600-0x410007ff] [0.391070] pci :00:02.0: [1af4:1000] type 00 class 0x02 [0.392212] pci :00:02.0: reg 0x10: [io 0x6400-0x64ff] [0.393137] pci :00:02.0: reg 0x14: [mem 0x41000800-0x410008ff] [0.394163] pci :00:02.0: reg 0x18: [mem 0x41000a00-0x41000bff] [0.395678] pci :00:00.0: BAR 2: assigned [mem 0x4100-0x410001ff] [0.396762] pci :00:01.0: BAR 2: assigned [mem
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
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? Thanks, tglx
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-03 10:16, Thomas Gleixner wrote: On Tue, Nov 03 2020 at 09:54, Marc Zyngier wrote: On 2020-11-02 22:18, Thomas Gleixner wrote: On Mon, Nov 02 2020 at 17:16, Thomas Gleixner wrote: On Mon, Nov 02 2020 at 11:30, Marc Zyngier wrote: --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) d = pci_host_bridge_msi_domain(b); dev_set_msi_domain(>dev, d); + if (!d) + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; Hrm, that might break legacy setups (no irqdomain support). I'd rather prefer to explicitly tell the pci core at host registration time. s/might break/ breaks / Just validated :) For my own edification, can you point me to the failing case? Any architecture which selects PCI_MSI_ARCH_FALLBACKS and does not have irqdomain support runs into: if (!d) bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; which in turn makes pci_msi_supported() return 0 and consequently makes pci_enable_msi[x]() fail. I pointer that out in [1], together with a potential fix. Not sure if anything else breaks though. Thanks, M. [1] https://lore.kernel.org/r/336d6588567949029c52ecfbb8766...@kernel.org/ -- Jazz is not dead. It just smells funny...
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Tue, Nov 03 2020 at 09:54, Marc Zyngier wrote: > On 2020-11-02 22:18, Thomas Gleixner wrote: >> On Mon, Nov 02 2020 at 17:16, Thomas Gleixner wrote: >>> On Mon, Nov 02 2020 at 11:30, Marc Zyngier wrote: --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) d = pci_host_bridge_msi_domain(b); dev_set_msi_domain(>dev, d); + if (!d) + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; >>> >>> Hrm, that might break legacy setups (no irqdomain support). I'd rather >>> prefer to explicitly tell the pci core at host registration time. >> >> s/might break/ breaks / Just validated :) > > For my own edification, can you point me to the failing case? Any architecture which selects PCI_MSI_ARCH_FALLBACKS and does not have irqdomain support runs into: if (!d) bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; which in turn makes pci_msi_supported() return 0 and consequently makes pci_enable_msi[x]() fail. Thanks, tglx
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-02 22:18, Thomas Gleixner wrote: On Mon, Nov 02 2020 at 17:16, Thomas Gleixner wrote: On Mon, Nov 02 2020 at 11:30, Marc Zyngier wrote: --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) d = pci_host_bridge_msi_domain(b); dev_set_msi_domain(>dev, d); + if (!d) + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; Hrm, that might break legacy setups (no irqdomain support). I'd rather prefer to explicitly tell the pci core at host registration time. s/might break/ breaks / Just validated :) For my own edification, can you point me to the failing case? 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. 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. I'll have a think. M. -- Jazz is not dead. It just smells funny...
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Mon, Nov 02 2020 at 17:16, Thomas Gleixner wrote: > On Mon, Nov 02 2020 at 11:30, Marc Zyngier wrote: >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus >> *bus) >> d = pci_host_bridge_msi_domain(b); >> >> dev_set_msi_domain(>dev, d); >> +if (!d) >> +bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; > > Hrm, that might break legacy setups (no irqdomain support). I'd rather > prefer to explicitly tell the pci core at host registration time. s/might break/ breaks / Just validated :) 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. 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. Thanks, tglx
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Mon, Nov 02 2020 at 11:30, Marc Zyngier wrote: > On 2020-11-01 22:27, Thomas Gleixner wrote: > The following patch makes it work for me (GICv3 guest without an ITS)by > checking for the presence of an MSI domain at the point where we > actually > perform this association, and before starting to scan for endpoints. > > I *think* this should work for the MTK thingy, but someone needs to > go and check. > > Thanks, > > M. > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 4289030b0fff..bb363eb103a2 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus > *bus) > d = pci_host_bridge_msi_domain(b); > > dev_set_msi_domain(>dev, d); > + if (!d) > + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; Hrm, that might break legacy setups (no irqdomain support). I'd rather prefer to explicitly tell the pci core at host registration time. Bjorn? Thanks, tglx
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-01 22:27, Thomas Gleixner wrote: On Sun, Nov 01 2020 at 21:47, Marc Zyngier wrote: On Sun, 01 Nov 2020 18:27:13 +, Frank Wunderlich wrote: Thinking of it a bit more, I think this is the wrong solution. PCI MSIs are optional, and not a requirement. I can trivially spin a VM with PCI devices and yet no MSI capability (yes, it is more difficult with real HW), and this results in a bunch of warning, none of which are actually indicative of anything being wrong. Well. No. The problem is that the device enumerates MSI capability, but the host bridge is not proving support for MSI. The host bridge fails to mark the bus with PCI_BUS_FLAGS_NO_MSI. That's the reason why this runs into this issue. Right, that's the piece I was missing, thanks for that. However, that doesn't really address the issue when the host bridge and the MSI widget are two separate entities, oblivious of each other (which is a pretty common thing on the ARM side). In this configuration, you can't really decide whether you have a MSI domain in the host bridge driver (the association is done in the code PCI code, after you have registered it with the core code), and by the time you get a pointer to the bus, the endpoints have already been probed. The following patch makes it work for me (GICv3 guest without an ITS)by checking for the presence of an MSI domain at the point where we actually perform this association, and before starting to scan for endpoints. I *think* this should work for the MTK thingy, but someone needs to go and check. Thanks, M. diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 4289030b0fff..bb363eb103a2 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -871,6 +871,8 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) d = pci_host_bridge_msi_domain(b); dev_set_msi_domain(>dev, d); + if (!d) + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; } static int pci_register_host_bridge(struct pci_host_bridge *bridge) -- Jazz is not dead. It just smells funny...
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Sun, Nov 01 2020 at 21:47, Marc Zyngier wrote:
> On Sun, 01 Nov 2020 18:27:13 +,
> Frank Wunderlich wrote:
> Thinking of it a bit more, I think this is the wrong solution.
>
> PCI MSIs are optional, and not a requirement. I can trivially spin a
> VM with PCI devices and yet no MSI capability (yes, it is more
> difficult with real HW), and this results in a bunch of warning, none
> of which are actually indicative of anything being wrong.
Well. No.
The problem is that the device enumerates MSI capability, but the host
bridge is not proving support for MSI.
The host bridge fails to mark the bus with PCI_BUS_FLAGS_NO_MSI. That's
the reason why this runs into this issue.
Something like the uncompiled hack below. I haven't found a way to hand
that down to the probe function.
Thanks,
tglx
---
diff --git a/drivers/pci/controller/pcie-mediatek.c
b/drivers/pci/controller/pcie-mediatek.c
index cf4c18f0c25a..d91bdfea7329 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -143,6 +143,7 @@ struct mtk_pcie_port;
* struct mtk_pcie_soc - differentiate between host generations
* @need_fix_class_id: whether this host's class ID needed to be fixed or not
* @need_fix_device_id: whether this host's device ID needed to be fixed or not
+ * @no_msi: Bridge has no MSI support
* @device_id: device ID which this host need to be fixed
* @ops: pointer to configuration access functions
* @startup: pointer to controller setting functions
@@ -151,6 +152,7 @@ struct mtk_pcie_port;
struct mtk_pcie_soc {
bool need_fix_class_id;
bool need_fix_device_id;
+ bool no_msi;
unsigned int device_id;
struct pci_ops *ops;
int (*startup)(struct mtk_pcie_port *port);
@@ -1089,6 +1091,9 @@ static int mtk_pcie_probe(struct platform_device *pdev)
if (err)
goto put_resources;
+ if (!pcie->soc->no_msi)
+ host->bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+
return 0;
put_resources:
@@ -1173,6 +1178,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
};
static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
+ .no_msi = true,
.ops = _pcie_ops,
.startup = mtk_pcie_startup_port,
};
Re: Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Sun, 01 Nov 2020 18:27:13 +, Frank Wunderlich wrote: > > > Gesendet: Sonntag, 01. November 2020 um 18:54 Uhr > > Von: "Ryder Lee" > > > Yea, mt7623 (mtk_pcie_soc_v1) does not support MSI, so that's a way to > > handle it. > > > > @Frank, could you help to test it? > > > > Ryder > > compiles clean for mt7623/armhf and mt7622/aarch64 so far > > at least bananapi-r2/mt7623 booting is clean now - no warning pcie > and sata/ahci seems still working as expected. I have a mt7615 card > in pcie-slot (firmware-load and init without errors) and hdd > connected to outer sata port (can access partitions witout errors) > > booted r64 too, still see no warning, but have not yet connected > hdd/pcie-device, but i guess this should not break anything here > > so Marc, if you post the patch separately, you can add my tested-by > ;) thank you for this Thinking of it a bit more, I think this is the wrong solution. PCI MSIs are optional, and not a requirement. I can trivially spin a VM with PCI devices and yet no MSI capability (yes, it is more difficult with real HW), and this results in a bunch of warning, none of which are actually indicative of anything being wrong. I think the real fix is to get rid of the warnings altogether. If we could detect that there should be an MSI domain associated with the device and that it wasn't there, that'd be a good reason to scream. But on its own, the absence of a MSI domain is not an indication of anything being amiss. M. -- Without deviation from the norm, progress is not possible.
