[PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[Jean-Philippe Brucker]

Since commit 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe()
declare its reliance on MSI domains"), platforms that rely on the
"msi-map" device-tree property don't get MSIs anymore.

On the Arm Fast Model for example [1], the host bridge doesn't have a
"msi-parent" property since it doesn't itself generate MSIs, and so
doesn't get a MSI domain. It has an "msi-map" property instead to
describe MSI controllers of child devices. As a result, due to the new
msi_domain check in pci_register_host_bridge(), the whole bus gets
PCI_BUS_FLAGS_NO_MSI.

Check whether the root complex has an "msi-map" property before giving
up on MSIs.

[1] arch/arm64/boot/dts/arm/fvp-base-revc.dts

Fixes: 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe() declare its reliance on MSI domains")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 include/linux/pci.h | 2 ++
 drivers/pci/of.c    | 7 +++++++
 drivers/pci/probe.c | 3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59a57..24306504226a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2344,6 +2344,7 @@ int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
 struct device_node;
 struct irq_domain;
 struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus);
+bool pci_host_of_has_msi_map(struct device *dev);
 
 /* Arch may override this (weak) */
 struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
@@ -2351,6 +2352,7 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
 #else	/* CONFIG_OF */
 static inline struct irq_domain *
 pci_host_bridge_of_msi_domain(struct pci_bus *bus) { return NULL; }
+static inline bool pci_host_of_has_msi_map(struct device *dev) { return false; }
 #endif  /* CONFIG_OF */
 
 static inline struct device_node *
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index da5b414d585a..85dcb7097da4 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -103,6 +103,13 @@ struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus)
 #endif
 }
 
+bool pci_host_of_has_msi_map(struct device *dev)
+{
+	if (dev && dev->of_node)
+		return of_get_property(dev->of_node, "msi-map", NULL);
+	return false;
+}
+
 static inline int __of_pci_pci_compare(struct device_node *node,
 				       unsigned int data)
 {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3a62d09b8869..275204646c68 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -925,7 +925,8 @@ 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->msi_domain && !dev_get_msi_domain(&bus->dev))
+	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&
+	    !pci_host_of_has_msi_map(parent))
 		bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
 
 	if (!parent)
-- 
2.31.1

Re: [PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[Marc Zyngier]

Hi Jean-Philippe,

On Mon, 10 May 2021 18:31:30 +0100,
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> Since commit 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe()
> declare its reliance on MSI domains"), platforms that rely on the
> "msi-map" device-tree property don't get MSIs anymore.
> 
> On the Arm Fast Model for example [1], the host bridge doesn't have a
> "msi-parent" property since it doesn't itself generate MSIs, and so
> doesn't get a MSI domain. It has an "msi-map" property instead to
> describe MSI controllers of child devices. As a result, due to the new
> msi_domain check in pci_register_host_bridge(), the whole bus gets
> PCI_BUS_FLAGS_NO_MSI.

Urgh... I knew I was going to break something... :-( Thanks for
picking this up.

> Check whether the root complex has an "msi-map" property before giving
> up on MSIs.
> 
> [1] arch/arm64/boot/dts/arm/fvp-base-revc.dts
> 
> Fixes: 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe() declare its reliance on MSI domains")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  include/linux/pci.h | 2 ++
>  drivers/pci/of.c    | 7 +++++++
>  drivers/pci/probe.c | 3 ++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59a57..24306504226a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2344,6 +2344,7 @@ int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
>  struct device_node;
>  struct irq_domain;
>  struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus);
> +bool pci_host_of_has_msi_map(struct device *dev);
>  
>  /* Arch may override this (weak) */
>  struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
> @@ -2351,6 +2352,7 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
>  #else	/* CONFIG_OF */
>  static inline struct irq_domain *
>  pci_host_bridge_of_msi_domain(struct pci_bus *bus) { return NULL; }
> +static inline bool pci_host_of_has_msi_map(struct device *dev) { return false; }
>  #endif  /* CONFIG_OF */
>  
>  static inline struct device_node *
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index da5b414d585a..85dcb7097da4 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -103,6 +103,13 @@ struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus)
>  #endif
>  }
>  
> +bool pci_host_of_has_msi_map(struct device *dev)
> +{
> +	if (dev && dev->of_node)
> +		return of_get_property(dev->of_node, "msi-map", NULL);
> +	return false;
> +}
> +
>  static inline int __of_pci_pci_compare(struct device_node *node,
>  				       unsigned int data)
>  {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3a62d09b8869..275204646c68 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -925,7 +925,8 @@ 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->msi_domain && !dev_get_msi_domain(&bus->dev))
> +	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&
> +	    !pci_host_of_has_msi_map(parent))
>  		bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
>  
>  	if (!parent)

Do we need something similar for IORT, which implements a similar
functionality to "msi-map"? My ACPI boxes seem to get their MSIs just
fine though...

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[Jean-Philippe Brucker]

Hi Marc,

On Mon, May 10, 2021 at 07:23:14PM +0100, Marc Zyngier wrote:
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 3a62d09b8869..275204646c68 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -925,7 +925,8 @@ 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->msi_domain && !dev_get_msi_domain(&bus->dev))
> > +	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&
> > +	    !pci_host_of_has_msi_map(parent))
> >  		bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> >  
> >  	if (!parent)
> 
> Do we need something similar for IORT, which implements a similar
> functionality to "msi-map"? My ACPI boxes seem to get their MSIs just
> fine though...

I'm not seeing the issue either under ACPI on the fast model, because it
doesn't go through pci_host_common_probe(), and bridge->msi_domain is not
set:

 acpi_pci_root_add()
  pci_acpi_scan_root()
   acpi_pci_root_create()
    pci_create_root_bus()
     pci_register_host_bridge()

It doesn't look like any ACPI platform can set bridge->msi_domain at the
moment. If they do flip the switch at some point, MSIs won't work and
we'll need something for IORT. But I'd leave it out for the moment.

Thanks,
Jean

Re: [PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[Marc Zyngier]

On Tue, 11 May 2021 15:12:08 +0100,
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> Hi Marc,
> 
> On Mon, May 10, 2021 at 07:23:14PM +0100, Marc Zyngier wrote:
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 3a62d09b8869..275204646c68 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -925,7 +925,8 @@ 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->msi_domain && !dev_get_msi_domain(&bus->dev))
> > > +	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&
> > > +	    !pci_host_of_has_msi_map(parent))
> > >  		bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
> > >  
> > >  	if (!parent)
> > 
> > Do we need something similar for IORT, which implements a similar
> > functionality to "msi-map"? My ACPI boxes seem to get their MSIs just
> > fine though...
> 
> I'm not seeing the issue either under ACPI on the fast model, because it
> doesn't go through pci_host_common_probe(), and bridge->msi_domain is not
> set:
> 
>  acpi_pci_root_add()
>   pci_acpi_scan_root()
>    acpi_pci_root_create()
>     pci_create_root_bus()
>      pci_register_host_bridge()
> 
> It doesn't look like any ACPI platform can set bridge->msi_domain at the
> moment. If they do flip the switch at some point, MSIs won't work and
> we'll need something for IORT. But I'd leave it out for the moment.

Fair enough, although there seem to be the opposite issue on some
platforms that do not have any MSI to offer (see [1]). I guess we'll
cross that bridge eventually.

For this patch:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

[1] https://lore.kernel.org/r/b5a5a6d8-6ffc-8c5c-c5b1-fb4f5616069f@arm.com

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] PCI/MSI: Fix MSIs for generic hosts that use device-tree's "msi-map"

[Bjorn Helgaas]

On Mon, May 10, 2021 at 07:31:30PM +0200, Jean-Philippe Brucker wrote:
> Since commit 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe()
> declare its reliance on MSI domains"), platforms that rely on the
> "msi-map" device-tree property don't get MSIs anymore.
> 
> On the Arm Fast Model for example [1], the host bridge doesn't have a
> "msi-parent" property since it doesn't itself generate MSIs, and so
> doesn't get a MSI domain. It has an "msi-map" property instead to
> describe MSI controllers of child devices. As a result, due to the new
> msi_domain check in pci_register_host_bridge(), the whole bus gets
> PCI_BUS_FLAGS_NO_MSI.
> 
> Check whether the root complex has an "msi-map" property before giving
> up on MSIs.
> 
> [1] arch/arm64/boot/dts/arm/fvp-base-revc.dts
> 
> Fixes: 9ec37efb8783 ("PCI/MSI: Make pci_host_common_probe() declare its reliance on MSI domains")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Applied to for-linus for v5.13, since we merged 9ec37efb8783 for
v5.13-rc1.  Thanks!

> ---
>  include/linux/pci.h | 2 ++
>  drivers/pci/of.c    | 7 +++++++
>  drivers/pci/probe.c | 3 ++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59a57..24306504226a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2344,6 +2344,7 @@ int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
>  struct device_node;
>  struct irq_domain;
>  struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus);
> +bool pci_host_of_has_msi_map(struct device *dev);
>  
>  /* Arch may override this (weak) */
>  struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
> @@ -2351,6 +2352,7 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);
>  #else	/* CONFIG_OF */
>  static inline struct irq_domain *
>  pci_host_bridge_of_msi_domain(struct pci_bus *bus) { return NULL; }
> +static inline bool pci_host_of_has_msi_map(struct device *dev) { return false; }
>  #endif  /* CONFIG_OF */
>  
>  static inline struct device_node *
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index da5b414d585a..85dcb7097da4 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -103,6 +103,13 @@ struct irq_domain *pci_host_bridge_of_msi_domain(struct pci_bus *bus)
>  #endif
>  }
>  
> +bool pci_host_of_has_msi_map(struct device *dev)
> +{
> +	if (dev && dev->of_node)
> +		return of_get_property(dev->of_node, "msi-map", NULL);
> +	return false;
> +}
> +
>  static inline int __of_pci_pci_compare(struct device_node *node,
>  				       unsigned int data)
>  {
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3a62d09b8869..275204646c68 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -925,7 +925,8 @@ 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->msi_domain && !dev_get_msi_domain(&bus->dev))
> +	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&
> +	    !pci_host_of_has_msi_map(parent))
>  		bus->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
>  
>  	if (!parent)
> -- 
> 2.31.1
>