Re: [RFC PATCH] PCI-E broken on PPC (regression)
From: Benjamin Herrenschmidt b...@kernel.crashing.org Date: Wed, 27 Jan 2010 13:10:56 +1100 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 Thanks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Wed, 17 Feb 2010 16:22:59 -0800 (PST) David Miller da...@davemloft.net wrote: From: Benjamin Herrenschmidt b...@kernel.crashing.org Date: Wed, 27 Jan 2010 13:10:56 +1100 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 Ok I'll include you guys on further changes. -- Jesse Barnes, Intel Open Source Technology Center ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Wed, Jan 27, 2010 at 01:10:56PM +1100, Benjamin Herrenschmidt wrote: 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. 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. -- Matthew Wilcox Intel Open Source Technology Centre Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Thu, 2010-01-28 at 20:45 -0700, Matthew Wilcox wrote: 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. Yup, and you'll notice that I didn't complain about the breakage for that precise reason :-) Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Wed, 27 Jan 2010 13:10:56 +1100 Benjamin Herrenschmidt b...@kernel.crashing.org wrote: 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? -- Jesse Barnes, Intel Open Source Technology Center ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
From: Benjamin Herrenschmidt b...@kernel.crashing.org Date: Thu, 28 Jan 2010 09:00:12 +1100 On Wed, 2010-01-27 at 08:26 -0800, Jesse Barnes wrote: 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Wed, 27 Jan 2010 16:01:21 -0800 (PST) David Miller da...@davemloft.net wrote: From: Benjamin Herrenschmidt b...@kernel.crashing.org Date: Thu, 28 Jan 2010 09:00:12 +1100 On Wed, 2010-01-27 at 08:26 -0800, Jesse Barnes wrote: 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. Ok, that's what I thought. We'll need to make these functions available then... -- Jesse Barnes, Intel Open Source Technology Center ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
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. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[RFC PATCH] 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.
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 -
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 98ffb2d..328c3ab 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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 -
diff --git a/arch/powerpc/kernel/pci_of_scan.c
b/arch/powerpc/kernel/pci_of_scan.c
index 7311fdf..f8820e8 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -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 = 0x;
+ set_pcie_port_type(dev);
if (!strcmp(type, pci) || !strcmp(type, pciex)) {
/* a PCI-PCI bridge */
dev-hdr_type = PCI_HEADER_TYPE_BRIDGE;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 98ffb2d..f787eea 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 174e539..765095b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Mon, 25 Jan 2010 11:42:29 -0200 Breno Leitao lei...@linux.vnet.ibm.com wrote: 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... -- Jesse Barnes, Intel Open Source Technology Center ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Mon, 25 Jan 2010 12:38:49 -0800 Jesse Barnes jbar...@virtuousgeek.org wrote: On Mon, 25 Jan 2010 11:42:29 -0200 Breno Leitao lei...@linux.vnet.ibm.com wrote: 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... 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... -- Jesse Barnes, Intel Open Source Technology Center ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
On Mon, 2010-01-25 at 17:50 -0800, Jesse Barnes wrote: 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... I need to give it more brain cells than I have available today :-) I'll have a look tomorrow. In some case we build the PCI stuff up from the firmware device-tree without actually doing any config space scanning, so the root busses are treated a bit special. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
Breno Leitao wrote:
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.
Hi Breno,
I'm sorry for the regression. I didn't realize that PPC uses open-coded
PCI scan.
I guess the problem is caused by dev-pcie_cap, right? But I don't
understand what is happening clearly. Could you send me the detailed
information about the problem?
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.
The set_pcie_port_type() needs to be called before pci_fixup_device() in
pci_setup_device() because fixup code might refer dev-pcie_cap. So I
don't think the first one is a good idea.
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.
I think the second one is better. But to prevent similar problems, I
think it would be better to remove open-coded PCI scan in PPC in a
long term, though I don't know the background about that.
Thanks,
Kenji Kaneshige
Thanks,
Breno
- First idea -
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 98ffb2d..328c3ab 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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 -
diff --git a/arch/powerpc/kernel/pci_of_scan.c
b/arch/powerpc/kernel/pci_of_scan.c
index 7311fdf..f8820e8 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -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 = 0x;
+ set_pcie_port_type(dev);
if (!strcmp(type, pci) || !strcmp(type, pciex)) {
/* a PCI-PCI bridge */
dev-hdr_type = PCI_HEADER_TYPE_BRIDGE;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 98ffb2d..f787eea 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 174e539..765095b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] PCI-E broken on PPC (regression)
Jesse Barnes wrote: On Mon, 25 Jan 2010 12:38:49 -0800 Jesse Barnes jbar...@virtuousgeek.org wrote: On Mon, 25 Jan 2010 11:42:29 -0200 Breno Leitao lei...@linux.vnet.ibm.com wrote: 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... 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... As I mentioned in the other e-mail, I think the set_pcie_port_type() needs to be called before pci_fixup_device() call in the pci_setup_device() because some fixup handler might refer the dev-is_pcie, dev-pcie_cap, or dev-pcie_type. Thanks, Kenji Kaneshige ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
