PCI-E broken on PPC (regression)

Hello,

I found that qlge is broken on PPC, and it got broken after commit 06a1cbafb253c4c60d6a54a994887f5fbceabcc0. It happens because dev->pcie is not set on PPC, because the function set_pcie_port_type(), who sets dev->pcie, is not being called on PPC PCI code.

Quoted 7 lines · On Mon, 25 Jan 2010 11:42:29 -0200 Breno Leitao <[email protected]>…

Hello,

I found that qlge is broken on PPC, and it got broken after commit 06a1cbafb253c4c60d6a54a994887f5fbceabcc0. It happens because dev->pcie is not set on PPC, because the function set_pcie_port_type(), who sets dev->pcie, is not being called on PPC PCI code.

You mean dev->is_pcie?

Why isn't pci_scan_device calling pci_setup_device for you? That should do the proper PCIe init depending on the device, along with extracting other device info...

Quoted 5 lines

You mean dev->is_pcie?

Why isn't pci_scan_device calling pci_setup_device for you? That should do the proper PCIe init depending on the device, along with extracting other device info...

Cc'ing Ben for PPC. Ben, should PPC use pci_scan_device when probing its root busses? Sounds like it just uses pci_device_add for each one it finds instead?

If you don't actually need scanning (though what about hotplug?) we can move the call to device_add instead...

Hello,

I found that qlge is broken on PPC, and it got broken after commit 06a1cbafb253c4c60d6a54a994887f5fbceabcc0. It happens because dev->pcie is not set on PPC, because the function set_pcie_port_type(), who sets dev->pcie, is not being called on PPC PCI code.

So, I have two ideas to fix it, the first one is to call set_pcie_port_type() on pci_device_add() instead of pci_setup_device(). Since that PPC device flow calls pci_device_add(), it fixes the problem without duplicating the caller for this function.

OTOH, it's also possible to add set_pcie_port_type() on pci.h and call it inside the PPC PCI files, specifically on of_create_pci_dev().

I tested both ideas and they work perfect, so I'd like to figure out which one is the most correct one. Both patches are attached here.

Thanks, Breno

----- First idea -----

drivers/pci/probe.c +1 -2

index 98ffb2d..328c3ab 100644
@@ -746,7 +746,6 @@ int pci_setup_device(struct pci_dev *dev)
 	dev->hdr_type = hdr_type & 0x7f;
 	dev->multifunction = !!(hdr_type & 0x80);
 	dev->error_state = pci_channel_io_normal;
-	set_pcie_port_type(dev);
 	set_pci_aer_firmware_first(dev);
 
 	list_for_each_entry(slot, &dev->bus->slots, list)

@@ -1052,6 +1051,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	/* Initialize various capabilities */
 	pci_init_capabilities(dev);
 
+	set_pcie_port_type(dev);
 	/*
 	 * Add the device to our list of discovered devices
 	 * and the bus list for fixup functions, etc.
 
----- Second idea -----

arch/powerpc/kernel/pci_of_scan.c +1 -0

index 7311fdf..f8820e8 100644
@@ -160,6 +160,7 @@ struct pci_dev *of_create_pci_dev(struct device_node *node,
 	dev->error_state = pci_channel_io_normal;
 	dev->dma_mask = 0xffffffff;
 
+	set_pcie_port_type(dev);
 	if (!strcmp(type, "pci") || !strcmp(type, "pciex")) {
 		/* a PCI-PCI bridge */
 		dev->hdr_type = PCI_HEADER_TYPE_BRIDGE;

drivers/pci/probe.c +1 -1

index 98ffb2d..f787eea 100644
@@ -681,7 +681,7 @@ static void pci_read_irq(struct pci_dev *dev)
 	dev->irq = irq;
 }
 
-static void set_pcie_port_type(struct pci_dev *pdev)
+void set_pcie_port_type(struct pci_dev *pdev)
 {
 	int pos;
 	u16 reg16;

include/linux/pci.h +1 -0

index 174e539..765095b 100644
@@ -1269,6 +1269,7 @@ int pcibios_add_platform_entries(struct pci_dev *dev);
 void pcibios_disable_device(struct pci_dev *dev);
 int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 				 enum pcie_reset_state state);
+extern void set_pcie_port_type(struct pci_dev *pdev);
 
 #ifdef CONFIG_PCI_MMCONFIG
 extern void __init pci_mmcfg_early_init(void);

Quoted 9 lines · On Mon, 25 Jan 2010 12:38:49 -0800 Jesse Barnes <[email protected]>…

Quoted 6 lines · On Mon, 25 Jan 2010 11:42:29 -0200 Breno Leitao <[email protected]>…

Hello,

I found that qlge is broken on PPC, and it got broken after commit 06a1cbafb253c4c60d6a54a994887f5fbceabcc0. It happens because dev->pcie is not set on PPC, because the function
set_pcie_port_type(), who sets dev->pcie, is not being called on PPC
PCI code.

Quoted 5 lines

You mean dev->is_pcie?

Why isn't pci_scan_device calling pci_setup_device for you? That should do the proper PCIe init depending on the device, along with extracting other device info...

Cc'ing Ben for PPC. Ben, should PPC use pci_scan_device when probing its root busses? Sounds like it just uses pci_device_add for each one it finds instead?

If you don't actually need scanning (though what about hotplug?) we can move the call to device_add instead...

Cc'ing Ben for PPC. Ben, should PPC use pci_scan_device when probing its root busses? Sounds like it just uses pci_device_add for each one it finds instead?

If you don't actually need scanning (though what about hotplug?) we can move the call to device_add instead...

Quoted 7 lines

Cc'ing Ben for PPC. Ben, should PPC use pci_scan_device when probing its root busses? Sounds like it just uses pci_device_add for each one it finds instead?

If you don't actually need scanning (though what about hotplug?) we can move the call to device_add instead...

Ok so I looked at the code and the problem goes way beyond root busses.

Basically, powerpc can use the code in arch/powerpc/kernel/pci_of_scan.c to "generate" the pci_dev without using config space probing or at least using as little of it as possible, using the firmware device-tree information instead.

This is also probably going to be moved to a more generic place and extended to be used optionally by other architectures.

I think sparc does something similar in fact in arch/sparc/kernel/pci.c (of_create_pci_dev()) though it would be logical to have sparc and powerpc share the same implementation here in the long run and I believe Grant Likely is working on it.

That means that potentially, pci_dev will be created on those archs for which pci_setup_device() is never called. Thus we need to be very careful when adding initializations there that at least we (myself and davem) are notified of that so we can mirror them in our code, or even better, if people doing so put them there too...

So as far as I can tell, we are missing that set_pcie_port_type(), so we need to add it to sparc and powerpc (and so make the function non-static in drivers/pci/probe.c). We are also missing the manipulation of dev->slot in fact, so that will need to be fixed too.

set_pcie_hotplug_bridge() might be something we want to add too, it's not totally clear yet due to possible issues with our firmwares. pci_fixup_device(pci_fixup_early,...) as well in fact.

I'll try do make ppc catch up with some of that see how it goes.

Thanks Ben. Any refactoring we need to handle this stuff better is fine with me too. I guess on some platforms calling pci_setup_device may cause problems with special platform devices?

Quoted 4 lines

Thanks Ben. Any refactoring we need to handle this stuff better is fine with me too. I guess on some platforms calling pci_setup_device may cause problems with special platform devices?

Well, we don't call pci_setup_device() because part of the deal is to avoid all of that config space reading that it does :-) Especially in the case of some of the IBM EADS bridges which don't let you access everything we may want.

From: Benjamin Herrenschmidt <[email protected]>
Date: Thu, 28 Jan 2010 09:00:12 +1100

Quoted 10 lines · Jesse Barnes

Quoted 4 lines

Thanks Ben. Any refactoring we need to handle this stuff better is fine with me too. I guess on some platforms calling pci_setup_device may cause problems with special platform devices?

Well, we don't call pci_setup_device() because part of the deal is to avoid all of that config space reading that it does :-) Especially in the case of some of the IBM EADS bridges which don't let you access everything we may want.

Same problem on sparc64, it's not safe to poke config space arbitrarily. Some PCI controllers even have bugs which cause them to hang if you try to access some parts of the host controller's PCI config space.

Quoted 7 lines

Cc'ing Ben for PPC. Ben, should PPC use pci_scan_device when probing its root busses? Sounds like it just uses pci_device_add for each one it finds instead?

If you don't actually need scanning (though what about hotplug?) we can move the call to device_add instead...

Ok so I looked at the code and the problem goes way beyond root busses.

Basically, powerpc can use the code in arch/powerpc/kernel/pci_of_scan.c to "generate" the pci_dev without using config space probing or at least using as little of it as possible, using the firmware device-tree information instead.

This is also probably going to be moved to a more generic place and extended to be used optionally by other architectures.

Quoted 2 lines

This is also probably going to be moved to a more generic place and extended to be used optionally by other architectures.

Yes, having it under drivers/pci/ somewhere would be a big improvement, that way we'd actually see it when trying to do cleanups and wouldn't accidentally break your architectures.

I'll try do make ppc catch up with some of that see how it goes.

From: Benjamin Herrenschmidt <[email protected]>
Date: Wed, 27 Jan 2010 13:10:56 +1100

Quoted 1 line

I'll try do make ppc catch up with some of that see how it goes.

FWIW I'm taking care of syncing up sparc in this area right now.

I just noticed the ->dma_mask assignment got moved as well.

Really, any change made to drivers/pci/probe.c is going to require powerpc and sparc changes these days.

We should and will commonize our OpenFirmware PCI device probing code under driver/pci but until that happens a real effort needs to be put in place to at least ping Benjamin and myself when changes are going in to drivers/pci/probe.c