ARM: PCI: kill pcibios_msi_controller()

From: Lorenzo Pieralisi <[email protected]>

In the ARM PCI bios32 layer, failures to dynamically allocate pci_sys_data for a PCI bus, or a PCI bus scan failure have to be considered serious warnings but they should not trigger a system panic so that at least the system is given a chance to be debugged.

This patch replaces the panic statements with WARN() messages to improve error reporting in the ARM PCI bios32 layer.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

CC Russell King <[email protected]>

CC Marc Zyngier <[email protected]>

---
 arch/arm/kernel/bios32.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

arch/arm/kernel/bios32.c +6 -4

index bf370bc..4e95260 100644
@@ -459,8 +459,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 
 	for (nr = busnr = 0; nr < hw->nr_controllers; nr++) {
 		sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL);
-		if (!sys)
-			panic("PCI: unable to allocate sys data!");
+		if (WARN(!sys, "PCI: unable to allocate sys data!"))
+			break;
 
 #ifdef CONFIG_PCI_MSI
 		sys->msi_ctrl = hw->msi_ctrl;

@@ -489,8 +489,10 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 				sys->bus = pci_scan_root_bus(parent, sys->busnr,
 						hw->ops, sys, &sys->resources);
 
-			if (!sys->bus)
-				panic("PCI: unable to scan bus!");
+			if (WARN(!sys->bus, "PCI: unable to scan bus!")) {
+				kfree(sys);
+				break;
+			}
 
 			busnr = sys->bus->busn_res.end + 1;
 

From: Lorenzo Pieralisi <[email protected]>

Add a pci_scan_root_bus_msi() interface so an arch can specify the MSI controller up front. This removes the need for a pcibios callback to set the MSI controller later.

This is not exported because I'd like to replace the variety of "scan root bus" interfaces with a single, more extensible interface that can handle the MSI controller, domain, pci_ops, resources, etc. I hope this interface is temporary.

[bhelgaas: changelog, split into separate patch, move to drivers/pci/pci.h]

Suggested-by Russell King <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/pci.h   |    5 +++++
 drivers/pci/probe.c |   14 ++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

drivers/pci/pci.h +5 -0

index 4ff0ff1..1d4c95c 100644
@@ -223,6 +223,11 @@ enum pci_bar_type {
 	pci_bar_mem64,		/* A 64-bit memory BAR */
 };
 
+struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
+				      struct pci_ops *ops, void *sysdata,
+				      struct list_head *resources,
+				      struct msi_controller *msi);
+
 bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 				int crs_timeout);
 int pci_setup_device(struct pci_dev *dev);

drivers/pci/probe.c +12 -2

index cefd636..9ff4df0 100644
@@ -2096,8 +2096,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
 			res, ret ? "can not be" : "is");
 }
 
-struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata,
+		struct list_head *resources, struct msi_controller *msi)
 {
 	struct resource_entry *window;
 	bool found = false;

@@ -2114,6 +2115,8 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	if (!b)
 		return NULL;
 
+	b->msi = msi;
+
 	if (!found) {
 		dev_info(&b->dev,
 		 "No busn resource found for root bus, will use [bus %02x-ff]\n",

@@ -2128,6 +2131,13 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 
 	return b;
 }
+
+struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return pci_scan_root_bus_msi(parent, bus, ops, sysdata, resources,
+				     NULL);
+}
 EXPORT_SYMBOL(pci_scan_root_bus);
 
 struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,

From: Lorenzo Pieralisi <[email protected]>

ARM previously stored the msi_controller pointer in its sysdata, struct pci_sys_data, and implemented pcibios_msi_controller() to retrieve it. That made PCI host controller drivers specific to ARM because they had to put the msi_controller pointer in the ARM-specific pci_sys_data.

There is now a generic mechanism, pci_scan_root_bus_msi(), for giving the msi_controller pointer to the PCI core. Use this for all ARM systems and for the DesignWare and Xilinx PCI host controller drivers.

This removes an ARM dependency from the DesignWare, DRA7xx, EXYNOS, i.MX6, Keystone, Layerscape, SPEAr13xx, and Xilinx drivers.

[bhelgaas: changelog, split into separate patch]

Suggested-by Russell King <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

CC Pratyush Anand <[email protected]>

CC Arnd Bergmann <[email protected]>

CC Jingoo Han <[email protected]>

CC Simon Horman <[email protected]>

CC Russell King <[email protected]>

CC Thomas Petazzoni <[email protected]>

CC Thierry Reding <[email protected]>

CC Michal Simek <[email protected]>

CC Marc Zyngier <[email protected]>

---
 arch/arm/include/asm/mach/pci.h    |    2 --
 arch/arm/kernel/bios32.c           |    5 +++--
 drivers/pci/host/pcie-designware.c |    6 +++---
 drivers/pci/host/pcie-xilinx.c     |    5 ++---
 4 files changed, 8 insertions(+), 10 deletions(-)

arch/arm/include/asm/mach/pci.h +0 -2

index 28b9bb3..c074e7a 100644
@@ -19,9 +19,7 @@ struct pci_bus;
 struct device;
 
 struct hw_pci {
-#ifdef CONFIG_PCI_MSI
 	struct msi_controller *msi_ctrl;
-#endif
 	struct pci_ops	*ops;
 	int		nr_controllers;
 	void		**private_data;

arch/arm/kernel/bios32.c +3 -2

index 4e95260..283bc1c 100644
@@ -486,8 +486,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 			if (hw->scan)
 				sys->bus = hw->scan(nr, sys);
 			else
-				sys->bus = pci_scan_root_bus(parent, sys->busnr,
-						hw->ops, sys, &sys->resources);
+				sys->bus = pci_scan_root_bus_msi(parent,
+					sys->busnr, hw->ops, sys,
+					&sys->resources, hw->msi_ctrl);
 
 			if (WARN(!sys->bus, "PCI: unable to scan bus!")) {
 				kfree(sys);

drivers/pci/host/pcie-designware.c +3 -3

index 69486be..bd0aeec 100644
@@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 #ifdef CONFIG_PCI_MSI
 	dw_pcie_msi_chip.dev = pp->dev;
-	dw_pci.msi_ctrl = &dw_pcie_msi_chip;
 #endif
 
 	dw_pci.nr_controllers = 1;

@@ -708,8 +707,9 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	struct pcie_port *pp = sys_to_pcie(sys);
 
 	pp->root_bus_nr = sys->busnr;
-	bus = pci_scan_root_bus(pp->dev, sys->busnr,
-				  &dw_pcie_ops, sys, &sys->resources);
+	bus = pci_scan_root_bus_msi(pp->dev, sys->busnr, &dw_pcie_ops, sys,
+				    &sys->resources, &dw_pcie_msi_chip);
+
 	if (!bus)
 		return NULL;
 

drivers/pci/host/pcie-xilinx.c +2 -3

index f1a06a0..526807d 100644
@@ -647,8 +647,8 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	struct pci_bus *bus;
 
 	port->root_busno = sys->busnr;
-	bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
-				sys, &sys->resources);
+	bus = pci_scan_root_bus_msi(port->dev, sys->busnr, &xilinx_pcie_ops,
+				sys, &sys->resources, &xilinx_pcie_msi_chip);
 
 	return bus;
 }

@@ -847,7 +847,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
 
 #ifdef CONFIG_PCI_MSI
 	xilinx_pcie_msi_chip.dev = port->dev;
-	hw.msi_ctrl = &xilinx_pcie_msi_chip;
 #endif
 	pci_common_init_dev(dev, &hw);
 

From: Lorenzo Pieralisi <[email protected]>

ARM now uses pci_bus->msi to store the msi_controller pointer, so we don't need to save it in struct pci_sys_data, and we don't need to implement pcibios_msi_controller() to get it out of pci_sys_data.

Remove msi_controller from struct pci_sys_data and pcibios_msi_controller().

[bhelgaas: changelog, split into separate patch]

Signed-off-by Lorenzo Pieralisi <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

---
 arch/arm/include/asm/mach/pci.h |    3 ---
 arch/arm/kernel/bios32.c        |   12 ------------
 2 files changed, 15 deletions(-)

arch/arm/include/asm/mach/pci.h +0 -3

index c074e7a..8857d28 100644
@@ -40,9 +40,6 @@ struct hw_pci {
  * Per-controller structure
  */
 struct pci_sys_data {
-#ifdef CONFIG_PCI_MSI
-	struct msi_controller *msi_ctrl;
-#endif
 	struct list_head node;
 	int		busnr;		/* primary bus number			*/
 	u64		mem_offset;	/* bus->cpu memory mapping offset	*/

arch/arm/kernel/bios32.c +0 -12

index 283bc1c..874e182 100644
@@ -18,15 +18,6 @@
 
 static int debug_pci;
 
-#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
-	struct pci_sys_data *sysdata = dev->bus->sysdata;
-
-	return sysdata->msi_ctrl;
-}
-#endif
-
 /*
  * We can't use pci_get_device() here since we are
  * called from interrupt context.

@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (WARN(!sys, "PCI: unable to allocate sys data!"))
 			break;
 
-#ifdef CONFIG_PCI_MSI
-		sys->msi_ctrl = hw->msi_ctrl;
-#endif
 		sys->busnr   = busnr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;

of_parse_phandle() returns a device_node pointer with the refcount incremented. We should dispose of this reference when we're finished.

Drop the reference acquired by of_parse_phandle().

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/host/pci-mvebu.c |    1 +
 drivers/pci/host/pci-xgene.c |    1 +
 2 files changed, 2 insertions(+)

drivers/pci/host/pci-mvebu.c +1 -0

index 70aa095..67ec5e1 100644
@@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie)
 		return;
 
 	pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
+	of_node_put(msi_node);
 
 	if (pcie->msi)
 		pcie->msi->dev = &pcie->pdev->dev;

drivers/pci/host/pci-xgene.c +1 -0

index a9dfb70..4c2fb1f 100644
@@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus)
 	if (!bus->msi)
 		return -ENODEV;
 
+	of_node_put(msi_node);
 	bus->msi->dev = &bus->dev;
 	return 0;
 }

[+cc Ben, Rob]

Quoted 3 lines · Bjorn Helgaas

of_parse_phandle() returns a device_node pointer with the refcount incremented. We should dispose of this reference when we're finished.

Quoted 1 line

Drop the reference acquired by of_parse_phandle().

I cc'd everybody who wrote or signed off on 0d5a6db3aa46 ("of: pci: add registry of MSI chips"), which added of_pci_find_msi_chip_by_node().

The existing code (before this patch) looks wrong to me, but I'm not an OF person.

Quoted 32 lines

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/host/pci-mvebu.c |    1 +
 drivers/pci/host/pci-xgene.c |    1 +
 2 files changed, 2 insertions(+)

drivers/pci/host/pci-mvebu.c +1 -0

index 70aa095..67ec5e1 100644
@@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie)
              return;
 
      pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
+     of_node_put(msi_node);
 
      if (pcie->msi)
              pcie->msi->dev = &pcie->pdev->dev;

drivers/pci/host/pci-xgene.c +1 -0

index a9dfb70..4c2fb1f 100644
@@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus)
      if (!bus->msi)
              return -ENODEV;
 
+     of_node_put(msi_node);
      bus->msi->dev = &bus->dev;
      return 0;
 }

of_parse_phandle() returns a device_node pointer with the refcount incremented. We should dispose of this reference when we're finished.

Drop the reference acquired by of_parse_phandle().

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/host/pci-mvebu.c |    1 +
 drivers/pci/host/pci-xgene.c |    1 +
 2 files changed, 2 insertions(+)

drivers/pci/host/pci-mvebu.c +1 -0

index 70aa095..67ec5e1 100644
@@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie)
 		return;
 
 	pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
+	of_node_put(msi_node);
 
 	if (pcie->msi)
 		pcie->msi->dev = &pcie->pdev->dev;

drivers/pci/host/pci-xgene.c +1 -0

index a9dfb70..4c2fb1f 100644
@@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus)
 	if (!bus->msi)
 		return -ENODEV;
 
+	of_node_put(msi_node);
 	bus->msi->dev = &bus->dev;
 	return 0;
 }

Other users of struct msi_controller set msi->dev to the platform_device of the PCI host controller, not the device of the pci_bus for the root bus.

Set X-Gene's msi_controller->dev to the PCI host controller platform_device as other platforms do.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/pci/host/pci-xgene.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/pci/host/pci-xgene.c +2 -1

index 4c2fb1f..5e0d6de 100644
@@ -501,7 +501,8 @@ static int xgene_pcie_setup(struct xgene_pcie_port *port,
 	return 0;
 }
 
-static int xgene_pcie_msi_enable(struct pci_bus *bus)
+static int xgene_pcie_msi_enable(struct xgene_pcie_port *port,
+				 struct pci_bus *bus)
 {
 	struct device_node *msi_node;
 

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

Hi Bjorn,

Quoted 7 lines · Bjorn Helgaas

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

I'm wondering about these XGene patches.

With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver.

Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver.

Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function).

Thoughts?

Quoted 25 lines · Marc Zyngier

Hi Bjorn,

Quoted 7 lines · Bjorn Helgaas

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

I'm wondering about these XGene patches.

With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver.

Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver.

Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function).

Quoted 1 line

Thoughts?

In pcie-altera driver, it uses pci_scan_root_bus() and it doesn't call to of_pci_find_msi_chip_by_node() to retrieve msi_controller struct as well.
But, the msi domain stuff is still working. Should I change it to pci_scan_root_bus_msi()?

Quoted 26 lines · Ley Foon Tan

Quoted 25 lines · Marc Zyngier

Hi Bjorn,

Quoted 7 lines · Bjorn Helgaas

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

I'm wondering about these XGene patches.

With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver.

Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver.

Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function).

Quoted 7 lines

Quoted 1 line

Thoughts?

In pcie-altera driver, it uses pci_scan_root_bus() and it doesn't call to of_pci_find_msi_chip_by_node() to retrieve msi_controller struct as well.
But, the msi domain stuff is still working. Should I change it to pci_scan_root_bus_msi()?

If you're not using struct msi_controller, I don't see a reason to use pci_scan_root_bus_msi(). The MSI irq domain patches are queued for 4.3, and I don't believe your driver will be merged before that.

Eventually, I'd like to kill msi_controller entirely. It is completely redundant with the use of irq domains.

Quoted 7 lines · Bjorn Helgaas

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

I'm wondering about these XGene patches.

With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver.

Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver.

Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function).

Hi Marc,

Quoted 11 lines · Marc Zyngier

Quoted 7 lines · Bjorn Helgaas

Previously there was no way to specify the MSI controller when creating a new PCI root bus, so we had to create the bus, set its MSI controller, then scan the bus. With the new pci_scan_root_bus_msi() interface, we can specify the MSI controller up front and get rid of that intermediate step.

Look up the MSI controller first, then use pci_scan_root_bus_msi() to create and scan the root PCI bus.

I'm wondering about these XGene patches.

Quoted 7 lines

With the code that is queued for v4.3 in tip/irq/core, the X-Gene MSI driver doesn't export a struct msi_controller anymore, and entirely relies on IRQ domains to identify to be matched with the actual PCI driver.

Do you intend this as a cleanup until everything lands in mainline? At that point, we'd be able to remove all traces of struct msi_controller from this driver.

I haven't been following the IRQ domain stuff or tip/irq/core, so I really don't know how this relates to that. I took a look, and I see 8d63bc7beaee ("PCI/MSI: pci-xgene-msi: Get rid of struct msi_controller"), which removes an msi_controller pointer from pci-xgene-msi.c, but I don't see any pci-xgene.c changes in tip/irq/core.

Are you saying that xgene_pcie_msi_enable() will go away eventually? And the OF "msi-parent" lookup will go away, or at least be moved elsewhere? We currently have:

  xgene_pcie_probe_bridge(...)
  {
    ...
    bus = pci_create_root_bus(...);
    xgene_pcie_msi_enable(...);
    pci_scan_child_bus(bus);
    ...
  }

I'd like to get rid of that arch-specific MSI enable stuff because then we can use a higher level interface, e.g., pci_scan_root_bus(), and make the X-Gene code slightly simpler.

Quoted 3 lines

Alternatively, we could ask tglx to add an extra patch to the existing queue in order to clean up pci-xgene.c (nuking the whole xgene_pcie_msi_enable function).

Ah, I guess you *are* saying that xgene_pcie_msi_enable() will go away eventually. I don't know how to do that, but apparently you do, so I'll just drop these X-Gene-related patches for now.

Bjorn

The first 5 patches here are essentially the same as Lorenzo's v4 posting at
http://lkml.kernel.org/1438169598-24490-1-git-send-email-lorenzo.pieralisi at arm.com

I added some at the end to fix what look like some X-Gene bugs to me, and to make X-Gene use pci_scan_root_bus_msi() like the other drivers that use msi_controller.

v4->v5 changes:
- Declare pci_scan_root_bus_msi() in drivers/pci/pci.h instead of include/linux/pci.h. I like the idea, but I hope we can replace this and other "scan_root_bus" interfaces with a single, more-generic interface, so I'd rather not expose this one widely.