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.
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: 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)
