Aw: Re: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
Hi, is there any new state? kernel test robot reports the following problem (i do not see it when compiling for my arm/arm64 devices): ARCH=i386 drivers/pci/probe.c: In function 'pci_register_host_bridge': >> drivers/pci/probe.c:930:39: error: 'struct device' has no member named >> 'msi_domain'; did you mean 'pm_domain'? 930 | (bridge->msi_domain && !bus->dev.msi_domain)) and yes...msi_domain is only member of pci_host_bridge (include/linux/pci.h) why do you check for msi_domain in pci_bus->dev regards Frank
Aw: Re: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
Hi, 5.11-rc1 is also affected, here is the relevant part of trace from my bananapi-r2: [5.792659] mtk-pcie 1a14.pcie: PCI host bridge to bus :00 [5.798879] pci_bus :00: root bus resource [bus 00-ff] [5.804412] pci_bus :00: root bus resource [io 0x-0x] (bus address [0x1a16-0x1a16]) [5.813928] pci_bus :00: root bus resource [mem 0x6000-0x6fff] [5.820876] pci :00:00.0: [14c3:0801] type 01 class 0x060400 [5.826915] pci :00:00.0: reg 0x14: [mem 0x-0x] [5.833294] pci :00:00.0: supports D1 [5.837315] pci :00:00.0: PME# supported from D0 D1 D3hot [5.843550] pci :00:01.0: [14c3:0801] type 01 class 0x060400 [5.849598] pci :00:01.0: reg 0x14: [mem 0x-0x] [5.855995] pci :00:01.0: supports D1 [5.860016] pci :00:01.0: PME# supported from D0 D1 D3hot [5.868688] PCI: bus0: Fast back to back transfers disabled [5.874325] pci :00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring [5.882392] pci :00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring [5.890648] pci :01:00.0: [14c3:7615] type 00 class 0x000280 [5.896700] pci :01:00.0: reg 0x10: [mem 0x-0x000f 64bit] [5.933326] PCI: bus1: Fast back to back transfers disabled [5.938930] pci_bus :01: busn_res: [bus 01-ff] end is updated to 01 [5.945814] pci :02:00.0: [1b21:0611] type 00 class 0x010185 [5.951889] pci :02:00.0: reg 0x10: initial BAR value 0x invalid [5.958949] pci :02:00.0: reg 0x10: [io size 0x0008] [5.964382] pci :02:00.0: reg 0x14: initial BAR value 0x invalid [5.971454] pci :02:00.0: reg 0x14: [io size 0x0004] [5.976869] pci :02:00.0: reg 0x18: initial BAR value 0x invalid [5.983938] pci :02:00.0: reg 0x18: [io size 0x0008] [5.989352] pci :02:00.0: reg 0x1c: initial BAR value 0x invalid [5.996424] pci :02:00.0: reg 0x1c: [io size 0x0004] [6.001854] pci :02:00.0: reg 0x20: initial BAR value 0x invalid [6.008910] pci :02:00.0: reg 0x20: [io size 0x0010] [6.014337] pci :02:00.0: reg 0x24: [mem 0x-0x01ff] [6.020635] pci :02:00.0: reg 0x30: [mem 0x-0x pref] [6.053312] PCI: bus2: Fast back to back transfers disabled [6.058912] pci_bus :02: busn_res: [bus 02-ff] end is updated to 02 [6.065622] pci :00:00.0: BAR 8: assigned [mem 0x6000-0x600f] [6.072448] pci :00:01.0: BAR 8: assigned [mem 0x6010-0x601f] [6.079248] pci :00:01.0: BAR 9: assigned [mem 0x6020-0x602f pref] [6.086500] pci :00:00.0: BAR 1: assigned [mem 0x6030-0x6030] [6.093318] pci :00:01.0: BAR 1: assigned [mem 0x6031-0x6031] [6.100120] pci :00:01.0: BAR 7: assigned [io 0x1000-0x1fff] [6.106255] pci :01:00.0: BAR 0: assigned [mem 0x6000-0x600f 64bit] [6.113609] pci :00:00.0: PCI bridge to [bus 01] [6.118585] pci :00:00.0: bridge window [mem 0x6000-0x600f] [6.125405] pci :02:00.0: BAR 6: assigned [mem 0x6020-0x6020 pref] [6.132656] pci :02:00.0: BAR 5: assigned [mem 0x6010-0x601001ff] [6.139458] pci :02:00.0: BAR 4: assigned [io 0x1000-0x100f] [6.145579] pci :02:00.0: BAR 0: assigned [io 0x1010-0x1017] [6.151699] pci :02:00.0: BAR 2: assigned [io 0x1018-0x101f] [6.157807] pci :02:00.0: BAR 1: assigned [io 0x1020-0x1023] [6.163927] pci :02:00.0: BAR 3: assigned [io 0x1024-0x1027] [6.170034] pci :00:01.0: PCI bridge to [bus 02] [6.175035] pci :00:01.0: bridge window [io 0x1000-0x1fff] [6.181153] pci :00:01.0: bridge window [mem 0x6010-0x601f] [6.187951] pci :00:01.0: bridge window [mem 0x6020-0x602f pref] [6.195427] pcieport :00:00.0: enabling device (0140 -> 0142) [6.201795] pcieport :00:00.0: PME: Signaling with IRQ 258 [6.208053] pcieport :00:01.0: enabling device (0140 -> 0143) [6.214395] pcieport :00:01.0: PME: Signaling with IRQ 252 [6.220784] ahci :02:00.0: version 3.0 [6.220814] ahci :02:00.0: enabling device (0140 -> 0143) [6.226705] [ cut here ] [6.231380] WARNING: CPU: 0 PID: 33 at include/linux/msi.h:252 pci_msi_setup_msi_irqs.constprop.0+0x78/0x80 [6.241167] Modules linked in: [6.244235] CPU: 0 PID: 33 Comm: kworker/0:1 Not tainted 5.11.0-rc1-bpi-r2-wifi #1 [6.251816] Hardware name: Mediatek Cortex-A7 (Device Tree) [6.257395] Workqueue: events deferred_probe_work_func [6.262549] Backtrace: [6.264999] [] (dump_backtrace) from [] (show_stack+0x20/0x24) [6.272598] r7:00fc r6:6013 r5: r4:c15eff8c [6.278258] [] (show_stack) from [] (dump_stack+0xcc/0xe0) [6.285499] []
Aw: Re: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
Hi, any new state here? last fix works, but i have not seen it approved by anyone for merge or sent as separate Patch regards Frank
Re: Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-05 23:00, Thomas Gleixner wrote: On Thu, Nov 05 2020 at 09:20, Marc Zyngier wrote: On 2020-11-04 23:14, Thomas Gleixner wrote: /* Resource alignment requirements */ resource_size_t (*align_resource)(struct pci_dev *dev, If that's the direction of travel, we also need something like this for configuration where the host bridge relies on an external MSI block that uses MSI domains (boot-tested in a GICv3 guest). Some more context would be helpful. Brain fails to decode the logic here. OK, let me try again. The MSI controller, which is the thing that deals with MSIs in the system (GICv2m, GICv3-ITS, and a number of others), is optional, is not part of the host bridge (it has nothing to do with PCI at all), and the bridge driver has absolutely no idea whether: - there is something that provides MSI or not - that something has successfully been initialised or not (which translates into an MSI domain being present or not) This is the case for most ARM systems, and all KVM/arm guests. Booting a VM without MSIs is absolutely trivial, and actually makes sense for some of the smaller guests. In these conditions, your no_msi attribute doesn't work as is: we can't decide on its value at probe time without extracting all of the OF/ACPI logic that deals with MSI domains from the core code, and making it available to the host bridge drivers for systems that follow that model. Using the flow you insist on requires parsing the topology twice: - once to find out whether there is actually a MSI provider registered for the host bridge in order to set the no_msi flag - once to actually be able to store the domain into the pci_bus structure, as it isn't available at probe time. My last suggestion is to indicate to the core code that there is a *possible* MSI controller available in the form of a MSI domain. This is still suboptimal compared to checking the presence an MSI domain in core code (my initial suggestion), but the fallback stuff gets in the way (though I still think it can be made to work). Anyway, this was my last attempt at addressing the problem. Most people won't see it. The couple of drivers that require the fallback hack are usually selected in distro kernels, and do a good job hiding the error. M. -- Jazz is not dead. It just smells funny...
Re: Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
On Thu, Nov 05 2020 at 09:20, Marc Zyngier wrote: > On 2020-11-04 23:14, Thomas Gleixner wrote: >> /* Resource alignment requirements */ >> resource_size_t (*align_resource)(struct pci_dev *dev, > > If that's the direction of travel, we also need something like this > for configuration where the host bridge relies on an external MSI block > that uses MSI domains (boot-tested in a GICv3 guest). Some more context would be helpful. Brain fails to decode the logic here.
Aw: Re: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
Marc's Patch based on thomas' last one also seems to work well for r2, again no warning, PCI and AHCI (connected to pcie bus) working regards Frank
Re: Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-04 23:14, Thomas Gleixner wrote:
[...]
TBH, that's butt ugly. So after staring long enough into the PCI code I
came up with a way to transport that information to the probe code.
That allows a particular device to say 'I can't do MSI' and at the same
time keeps the warning machinery intact which tells us that a
particular
host controller driver is broken.
Uncompiled and untested as usual :)
Thanks,
tglx
---
drivers/pci/controller/pcie-mediatek.c |4
drivers/pci/probe.c|3 +++
include/linux/pci.h|1 +
3 files changed, 8 insertions(+)
--- 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);
@@ -1084,6 +1086,7 @@ static int mtk_pcie_probe(struct platfor
host->ops = pcie->soc->ops;
host->sysdata = pcie;
+ host->no_msi = pcie->soc->no_msi;
err = pci_host_probe(host);
if (err)
@@ -1173,6 +1176,7 @@ static const struct dev_pm_ops mtk_pcie_
};
static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
+ .no_msi = true,
.ops = _pcie_ops,
.startup = mtk_pcie_startup_port,
};
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -889,6 +889,9 @@ static int pci_register_host_bridge(stru
if (!bus)
return -ENOMEM;
+ if (bridge->no_msi)
+ bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+
bridge->bus = bus;
/* Temporarily move resources off the list */
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -545,6 +545,7 @@ struct pci_host_bridge {
unsigned intnative_dpc:1; /* OS may use PCIe DPC */
unsigned intpreserve_config:1; /* Preserve FW resource setup */
unsigned intsize_windows:1; /* Enable root bus sizing */
+ unsigned intno_msi:1; /* Bridge has no MSI support */
/* Resource alignment requirements */
resource_size_t (*align_resource)(struct pci_dev *dev,
If that's the direction of travel, we also need something like this
for configuration where the host bridge relies on an external MSI block
that uses MSI domains (boot-tested in a GICv3 guest).
M.
diff --git a/drivers/pci/controller/pci-host-common.c
b/drivers/pci/controller/pci-host-common.c
index 6ce34a1deecb..603f6fbbe68a 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -77,6 +77,7 @@ int pci_host_common_probe(struct platform_device
*pdev)
bridge->sysdata = cfg;
bridge->ops = (struct pci_ops *)>pci_ops;
+ bridge->msi_domain = true;
platform_set_drvdata(pdev, bridge);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 16fb150fbb8d..f421b2869bca 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -889,9 +889,6 @@ static int pci_register_host_bridge(struct
pci_host_bridge *bridge)
if (!bus)
return -ENOMEM;
- if (bridge->no_msi)
- bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
-
bridge->bus = bus;
/* Temporarily move resources off the list */
@@ -928,6 +925,9 @@ static int pci_register_host_bridge(struct
pci_host_bridge *bridge)
device_enable_async_suspend(bus->bridge);
pci_set_bus_of_node(bus);
pci_set_bus_msi_domain(bus);
+ if (bridge->no_msi ||
+ (bridge->msi_domain && !bus->dev.msi_domain))
+ bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
if (!parent)
set_dev_node(bus->bridge, pcibus_to_node(bus));
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c2a0c1d471d6..81f72fd46e06 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -546,6 +546,7 @@ struct pci_host_bridge {
unsigned intpreserve_config:1; /* Preserve FW resource setup */
unsigned intsize_windows:1; /* Enable root bus sizing */
unsigned intno_msi:1; /* Bridge has no MSI support */
+ unsigned intmsi_domain:1; /* Bridge wants MSI domain */
/* Resource alignment requirements */
resource_size_t (*align_resource)(struct pci_dev *dev,
--
Jazz is not dead. It just smells funny...
Re: Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
Frank,
On Wed, Nov 04 2020 at 17:49, Frank Wunderlich wrote:
>> Von: "Thomas Gleixner"
>> 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'm not that deep into this, but just my thoughts...you are the experts :)
>
> checking for PCI_MSI_ARCH_FALLBACKS here does not help?
>
> something like this:
>
> #ifndef PCI_MSI_ARCH_FALLBACKS
> if (!d)
> bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> #endif
TBH, that's butt ugly. So after staring long enough into the PCI code I
came up with a way to transport that information to the probe code.
That allows a particular device to say 'I can't do MSI' and at the same
time keeps the warning machinery intact which tells us that a particular
host controller driver is broken.
Uncompiled and untested as usual :)
Thanks,
tglx
---
drivers/pci/controller/pcie-mediatek.c |4
drivers/pci/probe.c|3 +++
include/linux/pci.h|1 +
3 files changed, 8 insertions(+)
--- 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);
@@ -1084,6 +1086,7 @@ static int mtk_pcie_probe(struct platfor
host->ops = pcie->soc->ops;
host->sysdata = pcie;
+ host->no_msi = pcie->soc->no_msi;
err = pci_host_probe(host);
if (err)
@@ -1173,6 +1176,7 @@ static const struct dev_pm_ops mtk_pcie_
};
static const struct mtk_pcie_soc mtk_pcie_soc_v1 = {
+ .no_msi = true,
.ops = _pcie_ops,
.startup = mtk_pcie_startup_port,
};
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -889,6 +889,9 @@ static int pci_register_host_bridge(stru
if (!bus)
return -ENOMEM;
+ if (bridge->no_msi)
+ bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+
bridge->bus = bus;
/* Temporarily move resources off the list */
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -545,6 +545,7 @@ struct pci_host_bridge {
unsigned intnative_dpc:1; /* OS may use PCIe DPC */
unsigned intpreserve_config:1; /* Preserve FW resource setup */
unsigned intsize_windows:1; /* Enable root bus sizing */
+ unsigned intno_msi:1; /* Bridge has no MSI support */
/* Resource alignment requirements */
resource_size_t (*align_resource)(struct pci_dev *dev,
Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
> Gesendet: Dienstag, 03. November 2020 um 11:16 Uhr > Von: "Thomas Gleixner" > 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'm not that deep into this, but just my thoughts...you are the experts :) checking for PCI_MSI_ARCH_FALLBACKS here does not help? something like this: #ifndef PCI_MSI_ARCH_FALLBACKS if (!d) bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI; #endif imho pci_enable_msi[x]() does not do anything if there is no msi-support (or does not get called), so maybe check the PCI_BUS_FLAGS_NO_MSI before the call (if this is inside core) or inside the enable-function (if called from outside) or is this the issue marc talkes about (called before flag is set)? regards Frank
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
Aw: Re: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
> Gesendet: Montag, 02. November 2020 um 14:58 Uhr
> Von: "Marc Zyngier"
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d15c881e2e7e..5bb1306162c7 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -387,10 +387,20 @@ static ssize_t msi_bus_store(struct device *dev,
> struct device_attribute *attr,
> return count;
> }
>
> - if (val)
> + if (val) {
> + /*
> + * If there is no possibility for this bus to deal with
> + * MSIs, then allowing them to be requested would lead to
> + * the kernel complaining loudly. In this situation, don't
> + * let userspace mess things up.
> + */
> + if (!pci_bus_is_msi_capable(subordinate))
> + return -EINVAL;
> +
> subordinate->bus_flags &= ~PCI_BUS_FLAGS_NO_MSI;
> - else
> + } else {
> subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> + }
>
> dev_info(>dev, "MSI/MSI-X %s for future drivers of
> devices on this bus\n",
>val ? "allowed" : "disallowed");
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4289030b0fff..28861cc6435a 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 (!pci_bus_is_msi_capable(bus))
> + bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> }
>
> static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 22207a79762c..6aadb863dff4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2333,6 +2333,12 @@ pci_host_bridge_acpi_msi_domain(struct pci_bus
> *bus) { return NULL; }
> static inline bool pci_pr3_present(struct pci_dev *pdev) { return
> false; }
> #endif
>
> +static inline bool pci_bus_is_msi_capable(struct pci_bus *bus)
> +{
> + return (IS_ENABLED(CONFIG_PCI_MSI_ARCH_FALLBACKS) ||
> + dev_get_msi_domain(>dev));
> +}
> +
> #ifdef CONFIG_EEH
> static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
> {
Hi,
this Patch seems to work well too...no warning, pcie-card and hdd recognized
regards Frank
Re: Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
On 2020-11-02 11:56, Frank Wunderlich wrote:
looks good on bananapi-r2, no warning, pcie-card and hdd recognized
Thanks for giving it a shot. Still needs a bit of tweaking, as I expect
it to break configurations that select CONFIG_PCI_MSI_ARCH_FALLBACKS
(we have to assume that MSIs can be handled until we hit the
arch-specific
stuff).
There is also a small nit in the way we allow userspace to mess with
this flag via sysfs, and similar restrictions should probably apply.
Updated patch below.
M.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d15c881e2e7e..5bb1306162c7 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -387,10 +387,20 @@ static ssize_t msi_bus_store(struct device *dev,
struct device_attribute *attr,
return count;
}
- if (val)
+ if (val) {
+ /*
+* If there is no possibility for this bus to deal with
+* MSIs, then allowing them to be requested would lead to
+* the kernel complaining loudly. In this situation, don't
+* let userspace mess things up.
+*/
+ if (!pci_bus_is_msi_capable(subordinate))
+ return -EINVAL;
+
subordinate->bus_flags &= ~PCI_BUS_FLAGS_NO_MSI;
- else
+ } else {
subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
+ }
dev_info(>dev, "MSI/MSI-X %s for future drivers of
devices on this bus\n",
val ? "allowed" : "disallowed");
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4289030b0fff..28861cc6435a 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 (!pci_bus_is_msi_capable(bus))
+ bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
}
static int pci_register_host_bridge(struct pci_host_bridge *bridge)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 22207a79762c..6aadb863dff4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2333,6 +2333,12 @@ pci_host_bridge_acpi_msi_domain(struct pci_bus
*bus) { return NULL; }
static inline bool pci_pr3_present(struct pci_dev *pdev) { return
false; }
#endif
+static inline bool pci_bus_is_msi_capable(struct pci_bus *bus)
+{
+ return (IS_ENABLED(CONFIG_PCI_MSI_ARCH_FALLBACKS) ||
+ dev_get_msi_domain(>dev));
+}
+
#ifdef CONFIG_EEH
static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
{
--
Jazz is not dead. It just smells funny...
Aw: Re: Re: [PATCH] pci: mediatek: fix warning in msi.h
looks good on bananapi-r2, no warning, pcie-card and hdd recognized regards Frank > 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)
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.
Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
> 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 regards Frank
Re: [PATCH] pci: mediatek: fix warning in msi.h
On Sun, 2020-11-01 at 11:43 +, Marc Zyngier wrote:
> On Sun, 01 Nov 2020 09:25:04 +,
> Frank Wunderlich wrote:
> >
> > Am 31. Oktober 2020 22:49:14 MEZ schrieb Thomas Gleixner
> > :
> >
> > >That's not a fix. It's just supressing the warning.
> >
> > Ok sorry
> >
> > >So it needs to be figured out why the domain association is not there.
> >
> > It looks like for mt7623 there is no msi domain setup (done via
> > mtk_pcie_setup_irq callback + mtk_pcie_init_irq_domain) in mtk pcie
> > driver.
>
> Does this mean that this SoC never handled MSIs the first place? Which
> would explain the warning, as there is no MSI domain registered for
> the device, and we end-up falling back to arch_setup_msi_irqs().
>
> If this system truly is unable to handle MSIs, one potential
> workaround would be to register a PCI-MSI domain that would always
> fail its allocation with -ENOSPC. It is really ugly, but would keep
> the horror localised. See the patchlet below, which I can't test.
>
> If this situation is more common than we expect, we may need something
> in core code instead.
>
> M.
>
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
> diff --git a/drivers/pci/controller/pcie-mediatek.c
> b/drivers/pci/controller/pcie-mediatek.c
> index cf4c18f0c25a..52758b546d40 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -151,6 +151,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);
> @@ -435,6 +436,9 @@ static int mtk_pcie_irq_domain_alloc(struct irq_domain
> *domain, unsigned int vir
> struct mtk_pcie_port *port = domain->host_data;
> unsigned long bit;
>
> + if (port->pcie->soc->no_msi)
> + return -ENOSPC;
> +
> WARN_ON(nr_irqs != 1);
> mutex_lock(>lock);
>
> @@ -966,11 +970,13 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
> port->slot = slot;
> port->pcie = pcie;
>
> - if (pcie->soc->setup_irq) {
> + if (pcie->soc->setup_irq)
> err = pcie->soc->setup_irq(port, node);
> - if (err)
> - return err;
> - }
> + else
> + err = mtk_pcie_allocate_msi_domains(port);
> +
> + if (err)
> + return err;
>
> INIT_LIST_HEAD(>list);
> list_add_tail(>list, >ports);
> @@ -1173,6 +1179,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,
> };
>
Aw: Re: [PATCH] pci: mediatek: fix warning in msi.h
> Gesendet: Sonntag, 01. November 2020 um 12:43 Uhr > Von: "Marc Zyngier" > On Sun, 01 Nov 2020 09:25:04 +, > Frank Wunderlich wrote: > > It looks like for mt7623 there is no msi domain setup (done via > > mtk_pcie_setup_irq callback + mtk_pcie_init_irq_domain) in mtk pcie > > driver. > > Does this mean that this SoC never handled MSIs the first place? Which > would explain the warning, as there is no MSI domain registered for > the device, and we end-up falling back to arch_setup_msi_irqs(). i tried 5.10-rc1 (without my patch of course) on bananapi-r64 to check if driver on mt7622 works better (this does setup a msi-domain in pcie-driver)and i got no warning. so mt7623 needs to create an msi-domain or handle it in the correct way e.g. by returning -ENOSPC like in your code. > If this system truly is unable to handle MSIs, one potential > workaround would be to register a PCI-MSI domain that would always > fail its allocation with -ENOSPC. It is really ugly, but would keep > the horror localised. See the patchlet below, which I can't test. > > If this situation is more common than we expect, we may need something > in core code instead. thanks for your code-example, here we need a response from MTK (CC'd Chuanjia Liu) regards Frank
Re: [PATCH] pci: mediatek: fix warning in msi.h
On Sun, 01 Nov 2020 09:25:04 +,
Frank Wunderlich wrote:
>
> Am 31. Oktober 2020 22:49:14 MEZ schrieb Thomas Gleixner :
>
> >That's not a fix. It's just supressing the warning.
>
> Ok sorry
>
> >So it needs to be figured out why the domain association is not there.
>
> It looks like for mt7623 there is no msi domain setup (done via
> mtk_pcie_setup_irq callback + mtk_pcie_init_irq_domain) in mtk pcie
> driver.
Does this mean that this SoC never handled MSIs the first place? Which
would explain the warning, as there is no MSI domain registered for
the device, and we end-up falling back to arch_setup_msi_irqs().
If this system truly is unable to handle MSIs, one potential
workaround would be to register a PCI-MSI domain that would always
fail its allocation with -ENOSPC. It is really ugly, but would keep
the horror localised. See the patchlet below, which I can't test.
If this situation is more common than we expect, we may need something
in core code instead.
M.
diff --git a/drivers/pci/controller/pcie-mediatek.c
b/drivers/pci/controller/pcie-mediatek.c
index cf4c18f0c25a..52758b546d40 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -151,6 +151,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);
@@ -435,6 +436,9 @@ static int mtk_pcie_irq_domain_alloc(struct irq_domain
*domain, unsigned int vir
struct mtk_pcie_port *port = domain->host_data;
unsigned long bit;
+ if (port->pcie->soc->no_msi)
+ return -ENOSPC;
+
WARN_ON(nr_irqs != 1);
mutex_lock(>lock);
@@ -966,11 +970,13 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
port->slot = slot;
port->pcie = pcie;
- if (pcie->soc->setup_irq) {
+ if (pcie->soc->setup_irq)
err = pcie->soc->setup_irq(port, node);
- if (err)
- return err;
- }
+ else
+ err = mtk_pcie_allocate_msi_domains(port);
+
+ if (err)
+ return err;
INIT_LIST_HEAD(>list);
list_add_tail(>list, >ports);
@@ -1173,6 +1179,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,
};
--
Without deviation from the norm, progress is not possible.
Re: [PATCH] pci: mediatek: fix warning in msi.h
On Sun, Nov 01 2020 at 10:25, Frank Wunderlich wrote: > Am 31. Oktober 2020 22:49:14 MEZ schrieb Thomas Gleixner : > >>So it needs to be figured out why the domain association is not there. > > It looks like for mt7623 there is no msi domain setup (done via > mtk_pcie_setup_irq callback + mtk_pcie_init_irq_domain) in mtk pcie > driver. Hrm. No idea how that is supposed to work. I defer that to the mt wizards then. Thanks, tglx
Re: [PATCH] pci: mediatek: fix warning in msi.h
Am 31. Oktober 2020 22:49:14 MEZ schrieb Thomas Gleixner : >That's not a fix. It's just supressing the warning. Ok sorry >So it needs to be figured out why the domain association is not there. It looks like for mt7623 there is no msi domain setup (done via mtk_pcie_setup_irq callback + mtk_pcie_init_irq_domain) in mtk pcie driver. https://elixir.bootlin.com/linux/v5.10-rc1/source/drivers/pci/controller/pcie-mediatek.c#L1204 regards Frank
Re: [PATCH] pci: mediatek: fix warning in msi.h
Frank, On Sat, Oct 31 2020 at 15:03, Frank Wunderlich wrote: > From: Frank Wunderlich > > 5.10 shows these warnings on bootup while enabling pcie > at least on bananapi-r2: > > [6.161730] WARNING: CPU: 2 PID: 73 at include/linux/msi.h:213 > pci_msi_setup_ > msi_irqs.constprop.0+0x78/0x80 > > [6.724607] WARNING: CPU: 2 PID: 73 at include/linux/msi.h:219 > free_msi_irqs+ > > fix this by selecting PCI_MSI_ARCH_FALLBACKS for MTK PCIe driver That's not a fix. It's just supressing the warning. PCI_MSI_ARCH_FALLBACKS is only valid for 1) Architectures which implement the fallbacks 2) Outdated PCI controller drivers on architectures without #1 which implement the deprecated msi_controller mechanism. That is handled in the weak arch fallback implementation. The mediatek PCIE driver does not qualify for #2. It's purely irq domain based. So there is something else going wrong. The PCI device which tries to allocate MSIs does not have an irq domain associated which makes it run into that warning. If you enable PCI_MSI_ARCH_FALLBACKS then the MSI allocation fails silently. So it's just papering over the underlying problem. So it needs to be figured out why the domain association is not there. Thanks, tglx
[PATCH] pci: mediatek: fix warning in msi.h
From: Frank Wunderlich
5.10 shows these warnings on bootup while enabling pcie
at least on bananapi-r2:
[6.161730] WARNING: CPU: 2 PID: 73 at include/linux/msi.h:213 pci_msi_setup_
msi_irqs.constprop.0+0x78/0x80
[6.724607] WARNING: CPU: 2 PID: 73 at include/linux/msi.h:219 free_msi_irqs+
fix this by selecting PCI_MSI_ARCH_FALLBACKS for MTK PCIe driver
Fixes: 077ee78e3928 ("PCI/MSI: Make arch_.*_msi_irq[s] fallbacks selectable")
Signed-off-by: Frank Wunderlich
---
drivers/pci/controller/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 64e2f5e379aa..8345de010186 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -238,6 +238,7 @@ config PCIE_MEDIATEK
depends on ARCH_MEDIATEK || COMPILE_TEST
depends on OF
depends on PCI_MSI_IRQ_DOMAIN
+ select PCI_MSI_ARCH_FALLBACKS
help
Say Y here if you want to enable PCIe controller support on
MediaTek SoCs.
--
2.25.1
