* [PATCH v3 0/2] archdata init in device_add() notifier
@ 2012-05-23 22:36 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:36 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, benh, microblaze-uclinux, linux-pci, Jesse Larrew,
jbarnes, Kenji Kaneshige, linuxppc-dev
Here's what I put in my "for-3.6" branch for now. We can still change
it, so let me know if you see any problems.
I reworked the changelogs, changed the notification function name per
Jesse, folded the one-line pcibios_setup_bus_notifier() into the only
caller (on microblaze), and changed it from __devinit to __init (on
powerpc) since it now has nothing to do with hotplug.
Thank you very much for doing this work, Matsumoto-san. I hope to
eventually get rid of pcibios_fixup_bus() altogether, and this is a
significant step in that direction.
Bjorn
---
Hiroo Matsumoto (2):
powerpc/PCI: move DMA & IRQ init to device_add() notification path
microblaze/PCI: move DMA & IRQ init to device_add() notification path
arch/microblaze/include/asm/pci.h | 1
arch/microblaze/pci/pci-common.c | 62 +++++++++++---------
arch/powerpc/include/asm/pci.h | 2 -
arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
arch/powerpc/kernel/pci_32.c | 2 +
arch/powerpc/kernel/pci_64.c | 2 +
arch/powerpc/kernel/pci_of_scan.c | 1
arch/powerpc/platforms/pseries/pci_dlpar.c | 1
drivers/pci/pci.c | 5 --
drivers/pcmcia/cardbus.c | 3 -
include/linux/pci.h | 3 -
11 files changed, 83 insertions(+), 86 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 0/2] archdata init in device_add() notifier
@ 2012-05-23 22:36 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:36 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, microblaze-uclinux, Kenji Kaneshige, Jesse Larrew,
jbarnes, linux-pci, linuxppc-dev
Here's what I put in my "for-3.6" branch for now. We can still change
it, so let me know if you see any problems.
I reworked the changelogs, changed the notification function name per
Jesse, folded the one-line pcibios_setup_bus_notifier() into the only
caller (on microblaze), and changed it from __devinit to __init (on
powerpc) since it now has nothing to do with hotplug.
Thank you very much for doing this work, Matsumoto-san. I hope to
eventually get rid of pcibios_fixup_bus() altogether, and this is a
significant step in that direction.
Bjorn
---
Hiroo Matsumoto (2):
powerpc/PCI: move DMA & IRQ init to device_add() notification path
microblaze/PCI: move DMA & IRQ init to device_add() notification path
arch/microblaze/include/asm/pci.h | 1
arch/microblaze/pci/pci-common.c | 62 +++++++++++---------
arch/powerpc/include/asm/pci.h | 2 -
arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
arch/powerpc/kernel/pci_32.c | 2 +
arch/powerpc/kernel/pci_64.c | 2 +
arch/powerpc/kernel/pci_of_scan.c | 1
arch/powerpc/platforms/pseries/pci_dlpar.c | 1
drivers/pci/pci.c | 5 --
drivers/pcmcia/cardbus.c | 3 -
include/linux/pci.h | 3 -
11 files changed, 83 insertions(+), 86 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-23 22:36 ` Bjorn Helgaas
@ 2012-05-23 22:37 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:37 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, benh, microblaze-uclinux, linux-pci, Jesse Larrew,
jbarnes, Dominik Brodowski, Kenji Kaneshige, linuxppc-dev
From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
-> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
others don't, e.g., pciehp, so sometimes hot-added devices are only
partly initialized.
This patch moves that initialization from pcibios_fixup_bus() to a new
pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
That means the initialization happens the same way for all PCI devices,
whether they are present at boot or hot-added later.
Note that powerpc was the only user of pci_fixup_cardbus(), which was
used to do this same setup for cardbus devices. That's no longer
needed because this setup will now be done in the same device_add()
notification path as all other PCI devices.
Typical failure of a hot-added e1000e device prior to this change:
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5
Successful initialization after this change:
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
[bhelgaas: changelog, notifier name, registration can be __init]
CC: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
arch/powerpc/include/asm/pci.h | 2 -
arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
arch/powerpc/kernel/pci_32.c | 2 +
arch/powerpc/kernel/pci_64.c | 2 +
arch/powerpc/kernel/pci_of_scan.c | 1
arch/powerpc/platforms/pseries/pci_dlpar.c | 1
drivers/pci/pci.c | 5 --
drivers/pcmcia/cardbus.c | 3 -
include/linux/pci.h | 3 -
9 files changed, 49 insertions(+), 57 deletions(-)
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 6653f27..d6a36a4 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
resource_size_t *start, resource_size_t *end);
extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
extern void pcibios_setup_bus_self(struct pci_bus *bus);
extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
extern void pcibios_scan_phb(struct pci_controller *hose);
+extern void pcibios_setup_bus_notifier(void);
#endif /* __KERNEL__ */
#endif /* __ASM_POWERPC_PCI_H */
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 7320f36..41b39ba 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
ppc_md.pci_dma_bus_setup(bus);
}
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- pr_debug("PCI: Fixup bus devices %d (%s)\n",
- bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- /* Cardbus can call us to add new devices to a bus, so ignore
- * those who are already fully discovered
- */
- if (dev->is_added)
- continue;
-
- /* Fixup NUMA node as it may not be setup yet by the generic
- * code and is needed by the DMA init
- */
- set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
- /* Hook up default DMA ops */
- set_dma_ops(&dev->dev, pci_dma_ops);
- set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
-
- /* Additional platform DMA/iommu setup */
- if (ppc_md.pci_dma_dev_setup)
- ppc_md.pci_dma_dev_setup(dev);
-
- /* Read default IRQs and fixup if necessary */
- pci_read_irq_line(dev);
- if (ppc_md.pci_irq_fixup)
- ppc_md.pci_irq_fixup(dev);
- }
-}
-
void pcibios_set_master(struct pci_dev *dev)
{
/* No special bus mastering setup handling */
@@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
/* Now fixup the bus bus */
pcibios_setup_bus_self(bus);
-
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
}
EXPORT_SYMBOL(pcibios_fixup_bus);
-void __devinit pci_fixup_cardbus(struct pci_bus *bus)
-{
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
-}
-
-
static int skip_isa_ioresource_align(struct pci_dev *dev)
{
if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
@@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
}
}
+static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct pci_dev *dev = to_pci_dev(data);
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ /* Setup OF node pointer in the device */
+ dev->dev.of_node = pci_device_to_OF_node(dev);
+
+ /* Fixup NUMA node as it may not be setup yet by the generic
+ * code and is needed by the DMA init
+ */
+ set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+ /* Hook up default DMA ops */
+ set_dma_ops(&dev->dev, pci_dma_ops);
+ set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+ /* Additional platform DMA/iommu setup */
+ if (ppc_md.pci_dma_dev_setup)
+ ppc_md.pci_dma_dev_setup(dev);
+
+ /* Read default IRQs and fixup if necessary */
+ pci_read_irq_line(dev);
+ if (ppc_md.pci_irq_fixup)
+ ppc_md.pci_irq_fixup(dev);
+
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block device_nb = {
+ .notifier_call = pci_bus_notify,
+};
+
+void __init pcibios_setup_bus_notifier(void)
+{
+ bus_register_notifier(&pci_bus_type, &device_nb);
+}
+
static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
{
int i, class = dev->class >> 8;
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 4b06ec5..640cc35 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -231,6 +231,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ pcibios_setup_bus_notifier();
+
if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
pci_assign_all_buses = 1;
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 4ff190f..8b212d3 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -48,6 +48,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ pcibios_setup_bus_notifier();
+
/* For now, override phys_mem_access_prot. If we need it,g
* later, we may move that initialization to each ppc_md
*/
diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index ae5ea5e..eb09eca 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
*/
if (!rescan_existing)
pcibios_setup_bus_self(bus);
- pcibios_setup_bus_devices(bus);
/* Now scan child busses */
list_for_each_entry(dev, &bus->devices, bus_list) {
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
index 3ccebc8..0b1b6b3 100644
--- a/arch/powerpc/platforms/pseries/pci_dlpar.c
+++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
@@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
if (!num)
return;
- pcibios_setup_bus_devices(bus);
max = bus->busn_res.start;
for (pass=0; pass < 2; pass++)
list_for_each_entry(dev, &bus->devices, bus_list) {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 15d442a..43e0a4f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
return 1;
}
-void __weak pci_fixup_cardbus(struct pci_bus *bus)
-{
-}
-EXPORT_SYMBOL(pci_fixup_cardbus);
-
static int __init pci_setup(char *str)
{
while (str) {
diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
index 24caeaf..a980691 100644
--- a/drivers/pcmcia/cardbus.c
+++ b/drivers/pcmcia/cardbus.c
@@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
unsigned int max, pass;
s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
- pci_fixup_cardbus(bus);
max = bus->busn_res.start;
for (pass = 0; pass < 2; pass++)
@@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
*/
pci_bus_size_bridges(bus);
pci_bus_assign_resources(bus);
- cardbus_config_irq_and_cls(bus, s->pci_irq);
/* socket specific tune function */
if (s->tune_bridge)
@@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
pci_enable_bridges(bus);
pci_bus_add_devices(bus);
+ cardbus_config_irq_and_cls(bus, s->pci_irq);
return 0;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a0e2d7f..3924c02 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
resource_size_t);
void pcibios_update_irq(struct pci_dev *, int irq);
-/* Weak but can be overriden by arch */
-void pci_fixup_cardbus(struct pci_bus *);
-
/* Generic PCI functions used internally */
void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-05-23 22:37 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:37 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, microblaze-uclinux, Kenji Kaneshige, Jesse Larrew,
jbarnes, Dominik Brodowski, linux-pci, linuxppc-dev
From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
-> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
others don't, e.g., pciehp, so sometimes hot-added devices are only
partly initialized.
This patch moves that initialization from pcibios_fixup_bus() to a new
pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
That means the initialization happens the same way for all PCI devices,
whether they are present at boot or hot-added later.
Note that powerpc was the only user of pci_fixup_cardbus(), which was
used to do this same setup for cardbus devices. That's no longer
needed because this setup will now be done in the same device_add()
notification path as all other PCI devices.
Typical failure of a hot-added e1000e device prior to this change:
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
e1000e 0000:03:00.0: No usable DMA configuration, aborting
e1000e: probe of 0000:03:00.0 failed with error -5
Successful initialization after this change:
# echo 1 > /sys/bus/pci/slots/1/power
<snip>
e1000e 0000:03:00.0: enabling device (0000 -> 0002)
irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
[bhelgaas: changelog, notifier name, registration can be __init]
CC: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
arch/powerpc/include/asm/pci.h | 2 -
arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
arch/powerpc/kernel/pci_32.c | 2 +
arch/powerpc/kernel/pci_64.c | 2 +
arch/powerpc/kernel/pci_of_scan.c | 1
arch/powerpc/platforms/pseries/pci_dlpar.c | 1
drivers/pci/pci.c | 5 --
drivers/pcmcia/cardbus.c | 3 -
include/linux/pci.h | 3 -
9 files changed, 49 insertions(+), 57 deletions(-)
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 6653f27..d6a36a4 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
resource_size_t *start, resource_size_t *end);
extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
extern void pcibios_setup_bus_self(struct pci_bus *bus);
extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
extern void pcibios_scan_phb(struct pci_controller *hose);
+extern void pcibios_setup_bus_notifier(void);
#endif /* __KERNEL__ */
#endif /* __ASM_POWERPC_PCI_H */
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 7320f36..41b39ba 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
ppc_md.pci_dma_bus_setup(bus);
}
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- pr_debug("PCI: Fixup bus devices %d (%s)\n",
- bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- /* Cardbus can call us to add new devices to a bus, so ignore
- * those who are already fully discovered
- */
- if (dev->is_added)
- continue;
-
- /* Fixup NUMA node as it may not be setup yet by the generic
- * code and is needed by the DMA init
- */
- set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
- /* Hook up default DMA ops */
- set_dma_ops(&dev->dev, pci_dma_ops);
- set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
-
- /* Additional platform DMA/iommu setup */
- if (ppc_md.pci_dma_dev_setup)
- ppc_md.pci_dma_dev_setup(dev);
-
- /* Read default IRQs and fixup if necessary */
- pci_read_irq_line(dev);
- if (ppc_md.pci_irq_fixup)
- ppc_md.pci_irq_fixup(dev);
- }
-}
-
void pcibios_set_master(struct pci_dev *dev)
{
/* No special bus mastering setup handling */
@@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
/* Now fixup the bus bus */
pcibios_setup_bus_self(bus);
-
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
}
EXPORT_SYMBOL(pcibios_fixup_bus);
-void __devinit pci_fixup_cardbus(struct pci_bus *bus)
-{
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
-}
-
-
static int skip_isa_ioresource_align(struct pci_dev *dev)
{
if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
@@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
}
}
+static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct pci_dev *dev = to_pci_dev(data);
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ /* Setup OF node pointer in the device */
+ dev->dev.of_node = pci_device_to_OF_node(dev);
+
+ /* Fixup NUMA node as it may not be setup yet by the generic
+ * code and is needed by the DMA init
+ */
+ set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+ /* Hook up default DMA ops */
+ set_dma_ops(&dev->dev, pci_dma_ops);
+ set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
+
+ /* Additional platform DMA/iommu setup */
+ if (ppc_md.pci_dma_dev_setup)
+ ppc_md.pci_dma_dev_setup(dev);
+
+ /* Read default IRQs and fixup if necessary */
+ pci_read_irq_line(dev);
+ if (ppc_md.pci_irq_fixup)
+ ppc_md.pci_irq_fixup(dev);
+
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block device_nb = {
+ .notifier_call = pci_bus_notify,
+};
+
+void __init pcibios_setup_bus_notifier(void)
+{
+ bus_register_notifier(&pci_bus_type, &device_nb);
+}
+
static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
{
int i, class = dev->class >> 8;
diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
index 4b06ec5..640cc35 100644
--- a/arch/powerpc/kernel/pci_32.c
+++ b/arch/powerpc/kernel/pci_32.c
@@ -231,6 +231,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ pcibios_setup_bus_notifier();
+
if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
pci_assign_all_buses = 1;
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 4ff190f..8b212d3 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -48,6 +48,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ pcibios_setup_bus_notifier();
+
/* For now, override phys_mem_access_prot. If we need it,g
* later, we may move that initialization to each ppc_md
*/
diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index ae5ea5e..eb09eca 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
*/
if (!rescan_existing)
pcibios_setup_bus_self(bus);
- pcibios_setup_bus_devices(bus);
/* Now scan child busses */
list_for_each_entry(dev, &bus->devices, bus_list) {
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
index 3ccebc8..0b1b6b3 100644
--- a/arch/powerpc/platforms/pseries/pci_dlpar.c
+++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
@@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
if (!num)
return;
- pcibios_setup_bus_devices(bus);
max = bus->busn_res.start;
for (pass=0; pass < 2; pass++)
list_for_each_entry(dev, &bus->devices, bus_list) {
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 15d442a..43e0a4f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
return 1;
}
-void __weak pci_fixup_cardbus(struct pci_bus *bus)
-{
-}
-EXPORT_SYMBOL(pci_fixup_cardbus);
-
static int __init pci_setup(char *str)
{
while (str) {
diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
index 24caeaf..a980691 100644
--- a/drivers/pcmcia/cardbus.c
+++ b/drivers/pcmcia/cardbus.c
@@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
unsigned int max, pass;
s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
- pci_fixup_cardbus(bus);
max = bus->busn_res.start;
for (pass = 0; pass < 2; pass++)
@@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
*/
pci_bus_size_bridges(bus);
pci_bus_assign_resources(bus);
- cardbus_config_irq_and_cls(bus, s->pci_irq);
/* socket specific tune function */
if (s->tune_bridge)
@@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
pci_enable_bridges(bus);
pci_bus_add_devices(bus);
+ cardbus_config_irq_and_cls(bus, s->pci_irq);
return 0;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a0e2d7f..3924c02 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
resource_size_t);
void pcibios_update_irq(struct pci_dev *, int irq);
-/* Weak but can be overriden by arch */
-void pci_fixup_cardbus(struct pci_bus *);
-
/* Generic PCI functions used internally */
void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] microblaze/PCI: move DMA & IRQ init to device_add() notification path
2012-05-23 22:36 ` Bjorn Helgaas
@ 2012-05-23 22:37 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:37 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, benh, microblaze-uclinux, linux-pci, Jesse Larrew,
jbarnes, Kenji Kaneshige, linuxppc-dev
From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
Microblaze initialized DMA and IRQ information in the pci_scan_child_bus()
-> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
others don't, e.g., pciehp, so sometimes hot-added devices are only
partly initialized.
This patch moves that initialization from pcibios_fixup_bus() to a new
pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
That means the initialization happens the same way for all PCI devices,
whether they are present at boot or hot-added later.
[bhelgaas: changelog, notifier name, inline registration]
Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
arch/microblaze/include/asm/pci.h | 1 -
arch/microblaze/pci/pci-common.c | 62 ++++++++++++++++++++-----------------
2 files changed, 34 insertions(+), 29 deletions(-)
diff --git a/arch/microblaze/include/asm/pci.h b/arch/microblaze/include/asm/pci.h
index a0da88b..2aa97fd 100644
--- a/arch/microblaze/include/asm/pci.h
+++ b/arch/microblaze/include/asm/pci.h
@@ -141,7 +141,6 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
const struct resource *rsrc,
resource_size_t *start, resource_size_t *end);
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
extern void pcibios_setup_bus_self(struct pci_bus *bus);
/* This part of code was originally in xilinx-pci.h */
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 9b32483..6487ed0 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -983,31 +983,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
pcibios_fixup_bridge(bus);
}
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- pr_debug("PCI: Fixup bus devices %d (%s)\n",
- bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- /* Setup OF node pointer in archdata */
- dev->dev.of_node = pci_device_to_OF_node(dev);
-
- /* Fixup NUMA node as it may not be setup yet by the generic
- * code and is needed by the DMA init
- */
- set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
- /* Hook up default DMA ops */
- set_dma_ops(&dev->dev, pci_dma_ops);
- dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
-
- /* Read default IRQs and fixup if necessary */
- pci_read_irq_line(dev);
- }
-}
-
void __devinit pcibios_fixup_bus(struct pci_bus *bus)
{
/* When called from the generic PCI probe, read PCI<->PCI bridge
@@ -1019,9 +994,6 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
/* Now fixup the bus bus */
pcibios_setup_bus_self(bus);
-
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
}
EXPORT_SYMBOL(pcibios_fixup_bus);
@@ -1512,6 +1484,38 @@ static void __devinit pcibios_scan_phb(struct pci_controller *hose)
hose->last_busno = bus->busn_res.end;
}
+static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct pci_dev *dev = to_pci_dev(data);
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ /* Setup OF node pointer in archdata */
+ dev->dev.of_node = pci_device_to_OF_node(dev);
+
+ /* Fixup NUMA node as it may not be setup yet by the generic
+ * code and is needed by the DMA init
+ */
+ set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+ /* Hook up default DMA ops */
+ set_dma_ops(&dev->dev, pci_dma_ops);
+ dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
+
+ /* Read default IRQs and fixup if necessary */
+ pci_read_irq_line(dev);
+
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block device_nb = {
+ .notifier_call = pci_bus_notify,
+};
+
static int __init pcibios_init(void)
{
struct pci_controller *hose, *tmp;
@@ -1519,6 +1523,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ bus_register_notifier(&pci_bus_type, &device_nb);
+
/* Scan all of the recorded PCI controllers. */
list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
hose->last_busno = 0xff;
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] microblaze/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-05-23 22:37 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-23 22:37 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, microblaze-uclinux, Kenji Kaneshige, Jesse Larrew,
jbarnes, linux-pci, linuxppc-dev
From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
Microblaze initialized DMA and IRQ information in the pci_scan_child_bus()
-> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
others don't, e.g., pciehp, so sometimes hot-added devices are only
partly initialized.
This patch moves that initialization from pcibios_fixup_bus() to a new
pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
That means the initialization happens the same way for all PCI devices,
whether they are present at boot or hot-added later.
[bhelgaas: changelog, notifier name, inline registration]
Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
arch/microblaze/include/asm/pci.h | 1 -
arch/microblaze/pci/pci-common.c | 62 ++++++++++++++++++++-----------------
2 files changed, 34 insertions(+), 29 deletions(-)
diff --git a/arch/microblaze/include/asm/pci.h b/arch/microblaze/include/asm/pci.h
index a0da88b..2aa97fd 100644
--- a/arch/microblaze/include/asm/pci.h
+++ b/arch/microblaze/include/asm/pci.h
@@ -141,7 +141,6 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
const struct resource *rsrc,
resource_size_t *start, resource_size_t *end);
-extern void pcibios_setup_bus_devices(struct pci_bus *bus);
extern void pcibios_setup_bus_self(struct pci_bus *bus);
/* This part of code was originally in xilinx-pci.h */
diff --git a/arch/microblaze/pci/pci-common.c b/arch/microblaze/pci/pci-common.c
index 9b32483..6487ed0 100644
--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -983,31 +983,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
pcibios_fixup_bridge(bus);
}
-void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
-{
- struct pci_dev *dev;
-
- pr_debug("PCI: Fixup bus devices %d (%s)\n",
- bus->number, bus->self ? pci_name(bus->self) : "PHB");
-
- list_for_each_entry(dev, &bus->devices, bus_list) {
- /* Setup OF node pointer in archdata */
- dev->dev.of_node = pci_device_to_OF_node(dev);
-
- /* Fixup NUMA node as it may not be setup yet by the generic
- * code and is needed by the DMA init
- */
- set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
-
- /* Hook up default DMA ops */
- set_dma_ops(&dev->dev, pci_dma_ops);
- dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
-
- /* Read default IRQs and fixup if necessary */
- pci_read_irq_line(dev);
- }
-}
-
void __devinit pcibios_fixup_bus(struct pci_bus *bus)
{
/* When called from the generic PCI probe, read PCI<->PCI bridge
@@ -1019,9 +994,6 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
/* Now fixup the bus bus */
pcibios_setup_bus_self(bus);
-
- /* Now fixup devices on that bus */
- pcibios_setup_bus_devices(bus);
}
EXPORT_SYMBOL(pcibios_fixup_bus);
@@ -1512,6 +1484,38 @@ static void __devinit pcibios_scan_phb(struct pci_controller *hose)
hose->last_busno = bus->busn_res.end;
}
+static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct pci_dev *dev = to_pci_dev(data);
+
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
+ /* Setup OF node pointer in archdata */
+ dev->dev.of_node = pci_device_to_OF_node(dev);
+
+ /* Fixup NUMA node as it may not be setup yet by the generic
+ * code and is needed by the DMA init
+ */
+ set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+ /* Hook up default DMA ops */
+ set_dma_ops(&dev->dev, pci_dma_ops);
+ dev->dev.archdata.dma_data = (void *)PCI_DRAM_OFFSET;
+
+ /* Read default IRQs and fixup if necessary */
+ pci_read_irq_line(dev);
+
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block device_nb = {
+ .notifier_call = pci_bus_notify,
+};
+
static int __init pcibios_init(void)
{
struct pci_controller *hose, *tmp;
@@ -1519,6 +1523,8 @@ static int __init pcibios_init(void)
printk(KERN_INFO "PCI: Probing PCI hardware\n");
+ bus_register_notifier(&pci_bus_type, &device_nb);
+
/* Scan all of the recorded PCI controllers. */
list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
hose->last_busno = 0xff;
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-23 22:37 ` Bjorn Helgaas
@ 2012-05-25 3:00 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-05-25 3:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Hiroo Matsumoto, Michal Simek, microblaze-uclinux, linux-pci,
Jesse Larrew, jbarnes, Dominik Brodowski, Kenji Kaneshige,
linuxppc-dev
On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>
> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
> -> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
> others don't, e.g., pciehp, so sometimes hot-added devices are only
> partly initialized.
>
> This patch moves that initialization from pcibios_fixup_bus() to a new
> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
> That means the initialization happens the same way for all PCI devices,
> whether they are present at boot or hot-added later.
>
> Note that powerpc was the only user of pci_fixup_cardbus(), which was
> used to do this same setup for cardbus devices. That's no longer
> needed because this setup will now be done in the same device_add()
> notification path as all other PCI devices.
Hrm. That will require a good deal of testing... Unfortunately I'm out
for a few weeks getting some surgery and then recovering...
Our PCI code has ancient roots and I wouldn't be surprised if that
change breaks subtle assumptions made here or there, we'd need to test
at least on a good range of macs and ibm hotplug stuff.
Cheers,
Ben.
> Typical failure of a hot-added e1000e device prior to this change:
>
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> e1000e 0000:03:00.0: No usable DMA configuration, aborting
> e1000e: probe of 0000:03:00.0 failed with error -5
>
> Successful initialization after this change:
>
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>
> [bhelgaas: changelog, notifier name, registration can be __init]
> CC: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> arch/powerpc/include/asm/pci.h | 2 -
> arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
> arch/powerpc/kernel/pci_32.c | 2 +
> arch/powerpc/kernel/pci_64.c | 2 +
> arch/powerpc/kernel/pci_of_scan.c | 1
> arch/powerpc/platforms/pseries/pci_dlpar.c | 1
> drivers/pci/pci.c | 5 --
> drivers/pcmcia/cardbus.c | 3 -
> include/linux/pci.h | 3 -
> 9 files changed, 49 insertions(+), 57 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 6653f27..d6a36a4 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
> resource_size_t *start, resource_size_t *end);
>
> extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
> extern void pcibios_setup_bus_self(struct pci_bus *bus);
> extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
> extern void pcibios_scan_phb(struct pci_controller *hose);
> +extern void pcibios_setup_bus_notifier(void);
>
> #endif /* __KERNEL__ */
> #endif /* __ASM_POWERPC_PCI_H */
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 7320f36..41b39ba 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
> ppc_md.pci_dma_bus_setup(bus);
> }
>
> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
> -{
> - struct pci_dev *dev;
> -
> - pr_debug("PCI: Fixup bus devices %d (%s)\n",
> - bus->number, bus->self ? pci_name(bus->self) : "PHB");
> -
> - list_for_each_entry(dev, &bus->devices, bus_list) {
> - /* Cardbus can call us to add new devices to a bus, so ignore
> - * those who are already fully discovered
> - */
> - if (dev->is_added)
> - continue;
> -
> - /* Fixup NUMA node as it may not be setup yet by the generic
> - * code and is needed by the DMA init
> - */
> - set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> -
> - /* Hook up default DMA ops */
> - set_dma_ops(&dev->dev, pci_dma_ops);
> - set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> -
> - /* Additional platform DMA/iommu setup */
> - if (ppc_md.pci_dma_dev_setup)
> - ppc_md.pci_dma_dev_setup(dev);
> -
> - /* Read default IRQs and fixup if necessary */
> - pci_read_irq_line(dev);
> - if (ppc_md.pci_irq_fixup)
> - ppc_md.pci_irq_fixup(dev);
> - }
> -}
> -
> void pcibios_set_master(struct pci_dev *dev)
> {
> /* No special bus mastering setup handling */
> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>
> /* Now fixup the bus bus */
> pcibios_setup_bus_self(bus);
> -
> - /* Now fixup devices on that bus */
> - pcibios_setup_bus_devices(bus);
> }
> EXPORT_SYMBOL(pcibios_fixup_bus);
>
> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
> -{
> - /* Now fixup devices on that bus */
> - pcibios_setup_bus_devices(bus);
> -}
> -
> -
> static int skip_isa_ioresource_align(struct pci_dev *dev)
> {
> if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
> }
> }
>
> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct pci_dev *dev = to_pci_dev(data);
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + /* Setup OF node pointer in the device */
> + dev->dev.of_node = pci_device_to_OF_node(dev);
> +
> + /* Fixup NUMA node as it may not be setup yet by the generic
> + * code and is needed by the DMA init
> + */
> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> +
> + /* Hook up default DMA ops */
> + set_dma_ops(&dev->dev, pci_dma_ops);
> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> +
> + /* Additional platform DMA/iommu setup */
> + if (ppc_md.pci_dma_dev_setup)
> + ppc_md.pci_dma_dev_setup(dev);
> +
> + /* Read default IRQs and fixup if necessary */
> + pci_read_irq_line(dev);
> + if (ppc_md.pci_irq_fixup)
> + ppc_md.pci_irq_fixup(dev);
> +
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block device_nb = {
> + .notifier_call = pci_bus_notify,
> +};
> +
> +void __init pcibios_setup_bus_notifier(void)
> +{
> + bus_register_notifier(&pci_bus_type, &device_nb);
> +}
> +
> static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
> {
> int i, class = dev->class >> 8;
> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> index 4b06ec5..640cc35 100644
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>
> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>
> + pcibios_setup_bus_notifier();
> +
> if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> pci_assign_all_buses = 1;
>
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 4ff190f..8b212d3 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>
> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>
> + pcibios_setup_bus_notifier();
> +
> /* For now, override phys_mem_access_prot. If we need it,g
> * later, we may move that initialization to each ppc_md
> */
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index ae5ea5e..eb09eca 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
> */
> if (!rescan_existing)
> pcibios_setup_bus_self(bus);
> - pcibios_setup_bus_devices(bus);
>
> /* Now scan child busses */
> list_for_each_entry(dev, &bus->devices, bus_list) {
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index 3ccebc8..0b1b6b3 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
> num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
> if (!num)
> return;
> - pcibios_setup_bus_devices(bus);
> max = bus->busn_res.start;
> for (pass=0; pass < 2; pass++)
> list_for_each_entry(dev, &bus->devices, bus_list) {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 15d442a..43e0a4f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
> return 1;
> }
>
> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
> -{
> -}
> -EXPORT_SYMBOL(pci_fixup_cardbus);
> -
> static int __init pci_setup(char *str)
> {
> while (str) {
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index 24caeaf..a980691 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> unsigned int max, pass;
>
> s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
> - pci_fixup_cardbus(bus);
>
> max = bus->busn_res.start;
> for (pass = 0; pass < 2; pass++)
> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> */
> pci_bus_size_bridges(bus);
> pci_bus_assign_resources(bus);
> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> /* socket specific tune function */
> if (s->tune_bridge)
> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>
> pci_enable_bridges(bus);
> pci_bus_add_devices(bus);
> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> return 0;
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a0e2d7f..3924c02 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
> resource_size_t);
> void pcibios_update_irq(struct pci_dev *, int irq);
>
> -/* Weak but can be overriden by arch */
> -void pci_fixup_cardbus(struct pci_bus *);
> -
> /* Generic PCI functions used internally */
>
> void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-05-25 3:00 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-05-25 3:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Michal Simek, Hiroo Matsumoto, microblaze-uclinux,
Kenji Kaneshige, Jesse Larrew, jbarnes, Dominik Brodowski,
linux-pci, linuxppc-dev
On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>
> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
> -> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
> others don't, e.g., pciehp, so sometimes hot-added devices are only
> partly initialized.
>
> This patch moves that initialization from pcibios_fixup_bus() to a new
> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
> That means the initialization happens the same way for all PCI devices,
> whether they are present at boot or hot-added later.
>
> Note that powerpc was the only user of pci_fixup_cardbus(), which was
> used to do this same setup for cardbus devices. That's no longer
> needed because this setup will now be done in the same device_add()
> notification path as all other PCI devices.
Hrm. That will require a good deal of testing... Unfortunately I'm out
for a few weeks getting some surgery and then recovering...
Our PCI code has ancient roots and I wouldn't be surprised if that
change breaks subtle assumptions made here or there, we'd need to test
at least on a good range of macs and ibm hotplug stuff.
Cheers,
Ben.
> Typical failure of a hot-added e1000e device prior to this change:
>
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> e1000e 0000:03:00.0: No usable DMA configuration, aborting
> e1000e: probe of 0000:03:00.0 failed with error -5
>
> Successful initialization after this change:
>
> # echo 1 > /sys/bus/pci/slots/1/power
> <snip>
> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>
> [bhelgaas: changelog, notifier name, registration can be __init]
> CC: Dominik Brodowski <linux@dominikbrodowski.net>
> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> arch/powerpc/include/asm/pci.h | 2 -
> arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
> arch/powerpc/kernel/pci_32.c | 2 +
> arch/powerpc/kernel/pci_64.c | 2 +
> arch/powerpc/kernel/pci_of_scan.c | 1
> arch/powerpc/platforms/pseries/pci_dlpar.c | 1
> drivers/pci/pci.c | 5 --
> drivers/pcmcia/cardbus.c | 3 -
> include/linux/pci.h | 3 -
> 9 files changed, 49 insertions(+), 57 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 6653f27..d6a36a4 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
> resource_size_t *start, resource_size_t *end);
>
> extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
> extern void pcibios_setup_bus_self(struct pci_bus *bus);
> extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
> extern void pcibios_scan_phb(struct pci_controller *hose);
> +extern void pcibios_setup_bus_notifier(void);
>
> #endif /* __KERNEL__ */
> #endif /* __ASM_POWERPC_PCI_H */
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 7320f36..41b39ba 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
> ppc_md.pci_dma_bus_setup(bus);
> }
>
> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
> -{
> - struct pci_dev *dev;
> -
> - pr_debug("PCI: Fixup bus devices %d (%s)\n",
> - bus->number, bus->self ? pci_name(bus->self) : "PHB");
> -
> - list_for_each_entry(dev, &bus->devices, bus_list) {
> - /* Cardbus can call us to add new devices to a bus, so ignore
> - * those who are already fully discovered
> - */
> - if (dev->is_added)
> - continue;
> -
> - /* Fixup NUMA node as it may not be setup yet by the generic
> - * code and is needed by the DMA init
> - */
> - set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> -
> - /* Hook up default DMA ops */
> - set_dma_ops(&dev->dev, pci_dma_ops);
> - set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> -
> - /* Additional platform DMA/iommu setup */
> - if (ppc_md.pci_dma_dev_setup)
> - ppc_md.pci_dma_dev_setup(dev);
> -
> - /* Read default IRQs and fixup if necessary */
> - pci_read_irq_line(dev);
> - if (ppc_md.pci_irq_fixup)
> - ppc_md.pci_irq_fixup(dev);
> - }
> -}
> -
> void pcibios_set_master(struct pci_dev *dev)
> {
> /* No special bus mastering setup handling */
> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>
> /* Now fixup the bus bus */
> pcibios_setup_bus_self(bus);
> -
> - /* Now fixup devices on that bus */
> - pcibios_setup_bus_devices(bus);
> }
> EXPORT_SYMBOL(pcibios_fixup_bus);
>
> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
> -{
> - /* Now fixup devices on that bus */
> - pcibios_setup_bus_devices(bus);
> -}
> -
> -
> static int skip_isa_ioresource_align(struct pci_dev *dev)
> {
> if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
> }
> }
>
> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct pci_dev *dev = to_pci_dev(data);
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + /* Setup OF node pointer in the device */
> + dev->dev.of_node = pci_device_to_OF_node(dev);
> +
> + /* Fixup NUMA node as it may not be setup yet by the generic
> + * code and is needed by the DMA init
> + */
> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> +
> + /* Hook up default DMA ops */
> + set_dma_ops(&dev->dev, pci_dma_ops);
> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> +
> + /* Additional platform DMA/iommu setup */
> + if (ppc_md.pci_dma_dev_setup)
> + ppc_md.pci_dma_dev_setup(dev);
> +
> + /* Read default IRQs and fixup if necessary */
> + pci_read_irq_line(dev);
> + if (ppc_md.pci_irq_fixup)
> + ppc_md.pci_irq_fixup(dev);
> +
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block device_nb = {
> + .notifier_call = pci_bus_notify,
> +};
> +
> +void __init pcibios_setup_bus_notifier(void)
> +{
> + bus_register_notifier(&pci_bus_type, &device_nb);
> +}
> +
> static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
> {
> int i, class = dev->class >> 8;
> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> index 4b06ec5..640cc35 100644
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>
> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>
> + pcibios_setup_bus_notifier();
> +
> if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> pci_assign_all_buses = 1;
>
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 4ff190f..8b212d3 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>
> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>
> + pcibios_setup_bus_notifier();
> +
> /* For now, override phys_mem_access_prot. If we need it,g
> * later, we may move that initialization to each ppc_md
> */
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index ae5ea5e..eb09eca 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
> */
> if (!rescan_existing)
> pcibios_setup_bus_self(bus);
> - pcibios_setup_bus_devices(bus);
>
> /* Now scan child busses */
> list_for_each_entry(dev, &bus->devices, bus_list) {
> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
> index 3ccebc8..0b1b6b3 100644
> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
> num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
> if (!num)
> return;
> - pcibios_setup_bus_devices(bus);
> max = bus->busn_res.start;
> for (pass=0; pass < 2; pass++)
> list_for_each_entry(dev, &bus->devices, bus_list) {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 15d442a..43e0a4f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
> return 1;
> }
>
> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
> -{
> -}
> -EXPORT_SYMBOL(pci_fixup_cardbus);
> -
> static int __init pci_setup(char *str)
> {
> while (str) {
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index 24caeaf..a980691 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> unsigned int max, pass;
>
> s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
> - pci_fixup_cardbus(bus);
>
> max = bus->busn_res.start;
> for (pass = 0; pass < 2; pass++)
> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> */
> pci_bus_size_bridges(bus);
> pci_bus_assign_resources(bus);
> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> /* socket specific tune function */
> if (s->tune_bridge)
> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>
> pci_enable_bridges(bus);
> pci_bus_add_devices(bus);
> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> return 0;
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a0e2d7f..3924c02 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
> resource_size_t);
> void pcibios_update_irq(struct pci_dev *, int irq);
>
> -/* Weak but can be overriden by arch */
> -void pci_fixup_cardbus(struct pci_bus *);
> -
> /* Generic PCI functions used internally */
>
> void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-25 3:00 ` Benjamin Herrenschmidt
@ 2012-05-25 3:08 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-25 3:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Hiroo Matsumoto, Michal Simek, microblaze-uclinux, linux-pci,
Jesse Larrew, jbarnes, Dominik Brodowski, Kenji Kaneshige,
linuxppc-dev
On Thu, May 24, 2012 at 9:00 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
>> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>>
>> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
>> -> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
>> others don't, e.g., pciehp, so sometimes hot-added devices are only
>> partly initialized.
>>
>> This patch moves that initialization from pcibios_fixup_bus() to a new
>> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
>> That means the initialization happens the same way for all PCI devices,
>> whether they are present at boot or hot-added later.
>>
>> Note that powerpc was the only user of pci_fixup_cardbus(), which was
>> used to do this same setup for cardbus devices. That's no longer
>> needed because this setup will now be done in the same device_add()
>> notification path as all other PCI devices.
>
> Hrm. That will require a good deal of testing... Unfortunately I'm out
> for a few weeks getting some surgery and then recovering...
>
> Our PCI code has ancient roots and I wouldn't be surprised if that
> change breaks subtle assumptions made here or there, we'd need to test
> at least on a good range of macs and ibm hotplug stuff.
OK. Are you worried about cardbus in particular?
This is headed for the 3.6, not 3.5, so we should have plenty of time.
As soon as everything for the current merge window gets merged and
-next is ready for the next batch, I'll put this in there.
>> Typical failure of a hot-added e1000e device prior to this change:
>>
>> # echo 1 > /sys/bus/pci/slots/1/power
>> <snip>
>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> e1000e 0000:03:00.0: No usable DMA configuration, aborting
>> e1000e: probe of 0000:03:00.0 failed with error -5
>>
>> Successful initialization after this change:
>>
>> # echo 1 > /sys/bus/pci/slots/1/power
>> <snip>
>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>
>> [bhelgaas: changelog, notifier name, registration can be __init]
>> CC: Dominik Brodowski <linux@dominikbrodowski.net>
>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>> arch/powerpc/include/asm/pci.h | 2 -
>> arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
>> arch/powerpc/kernel/pci_32.c | 2 +
>> arch/powerpc/kernel/pci_64.c | 2 +
>> arch/powerpc/kernel/pci_of_scan.c | 1
>> arch/powerpc/platforms/pseries/pci_dlpar.c | 1
>> drivers/pci/pci.c | 5 --
>> drivers/pcmcia/cardbus.c | 3 -
>> include/linux/pci.h | 3 -
>> 9 files changed, 49 insertions(+), 57 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
>> index 6653f27..d6a36a4 100644
>> --- a/arch/powerpc/include/asm/pci.h
>> +++ b/arch/powerpc/include/asm/pci.h
>> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
>> resource_size_t *start, resource_size_t *end);
>>
>> extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
>> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
>> extern void pcibios_setup_bus_self(struct pci_bus *bus);
>> extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
>> extern void pcibios_scan_phb(struct pci_controller *hose);
>> +extern void pcibios_setup_bus_notifier(void);
>>
>> #endif /* __KERNEL__ */
>> #endif /* __ASM_POWERPC_PCI_H */
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>> index 7320f36..41b39ba 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
>> ppc_md.pci_dma_bus_setup(bus);
>> }
>>
>> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>> -{
>> - struct pci_dev *dev;
>> -
>> - pr_debug("PCI: Fixup bus devices %d (%s)\n",
>> - bus->number, bus->self ? pci_name(bus->self) : "PHB");
>> -
>> - list_for_each_entry(dev, &bus->devices, bus_list) {
>> - /* Cardbus can call us to add new devices to a bus, so ignore
>> - * those who are already fully discovered
>> - */
>> - if (dev->is_added)
>> - continue;
>> -
>> - /* Fixup NUMA node as it may not be setup yet by the generic
>> - * code and is needed by the DMA init
>> - */
>> - set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>> -
>> - /* Hook up default DMA ops */
>> - set_dma_ops(&dev->dev, pci_dma_ops);
>> - set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> -
>> - /* Additional platform DMA/iommu setup */
>> - if (ppc_md.pci_dma_dev_setup)
>> - ppc_md.pci_dma_dev_setup(dev);
>> -
>> - /* Read default IRQs and fixup if necessary */
>> - pci_read_irq_line(dev);
>> - if (ppc_md.pci_irq_fixup)
>> - ppc_md.pci_irq_fixup(dev);
>> - }
>> -}
>> -
>> void pcibios_set_master(struct pci_dev *dev)
>> {
>> /* No special bus mastering setup handling */
>> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>>
>> /* Now fixup the bus bus */
>> pcibios_setup_bus_self(bus);
>> -
>> - /* Now fixup devices on that bus */
>> - pcibios_setup_bus_devices(bus);
>> }
>> EXPORT_SYMBOL(pcibios_fixup_bus);
>>
>> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
>> -{
>> - /* Now fixup devices on that bus */
>> - pcibios_setup_bus_devices(bus);
>> -}
>> -
>> -
>> static int skip_isa_ioresource_align(struct pci_dev *dev)
>> {
>> if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
>> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
>> }
>> }
>>
>> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
>> + void *data)
>> +{
>> + struct pci_dev *dev = to_pci_dev(data);
>> +
>> + switch (action) {
>> + case BUS_NOTIFY_ADD_DEVICE:
>> + /* Setup OF node pointer in the device */
>> + dev->dev.of_node = pci_device_to_OF_node(dev);
>> +
>> + /* Fixup NUMA node as it may not be setup yet by the generic
>> + * code and is needed by the DMA init
>> + */
>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>> +
>> + /* Hook up default DMA ops */
>> + set_dma_ops(&dev->dev, pci_dma_ops);
>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> + /* Additional platform DMA/iommu setup */
>> + if (ppc_md.pci_dma_dev_setup)
>> + ppc_md.pci_dma_dev_setup(dev);
>> +
>> + /* Read default IRQs and fixup if necessary */
>> + pci_read_irq_line(dev);
>> + if (ppc_md.pci_irq_fixup)
>> + ppc_md.pci_irq_fixup(dev);
>> +
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct notifier_block device_nb = {
>> + .notifier_call = pci_bus_notify,
>> +};
>> +
>> +void __init pcibios_setup_bus_notifier(void)
>> +{
>> + bus_register_notifier(&pci_bus_type, &device_nb);
>> +}
>> +
>> static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>> {
>> int i, class = dev->class >> 8;
>> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
>> index 4b06ec5..640cc35 100644
>> --- a/arch/powerpc/kernel/pci_32.c
>> +++ b/arch/powerpc/kernel/pci_32.c
>> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>>
>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>
>> + pcibios_setup_bus_notifier();
>> +
>> if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>> pci_assign_all_buses = 1;
>>
>> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>> index 4ff190f..8b212d3 100644
>> --- a/arch/powerpc/kernel/pci_64.c
>> +++ b/arch/powerpc/kernel/pci_64.c
>> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>>
>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>
>> + pcibios_setup_bus_notifier();
>> +
>> /* For now, override phys_mem_access_prot. If we need it,g
>> * later, we may move that initialization to each ppc_md
>> */
>> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
>> index ae5ea5e..eb09eca 100644
>> --- a/arch/powerpc/kernel/pci_of_scan.c
>> +++ b/arch/powerpc/kernel/pci_of_scan.c
>> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
>> */
>> if (!rescan_existing)
>> pcibios_setup_bus_self(bus);
>> - pcibios_setup_bus_devices(bus);
>>
>> /* Now scan child busses */
>> list_for_each_entry(dev, &bus->devices, bus_list) {
>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
>> index 3ccebc8..0b1b6b3 100644
>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
>> num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>> if (!num)
>> return;
>> - pcibios_setup_bus_devices(bus);
>> max = bus->busn_res.start;
>> for (pass=0; pass < 2; pass++)
>> list_for_each_entry(dev, &bus->devices, bus_list) {
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 15d442a..43e0a4f 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
>> return 1;
>> }
>>
>> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
>> -{
>> -}
>> -EXPORT_SYMBOL(pci_fixup_cardbus);
>> -
>> static int __init pci_setup(char *str)
>> {
>> while (str) {
>> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
>> index 24caeaf..a980691 100644
>> --- a/drivers/pcmcia/cardbus.c
>> +++ b/drivers/pcmcia/cardbus.c
>> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>> unsigned int max, pass;
>>
>> s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
>> - pci_fixup_cardbus(bus);
>>
>> max = bus->busn_res.start;
>> for (pass = 0; pass < 2; pass++)
>> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>> */
>> pci_bus_size_bridges(bus);
>> pci_bus_assign_resources(bus);
>> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>>
>> /* socket specific tune function */
>> if (s->tune_bridge)
>> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>
>> pci_enable_bridges(bus);
>> pci_bus_add_devices(bus);
>> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>>
>> return 0;
>> }
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index a0e2d7f..3924c02 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
>> resource_size_t);
>> void pcibios_update_irq(struct pci_dev *, int irq);
>>
>> -/* Weak but can be overriden by arch */
>> -void pci_fixup_cardbus(struct pci_bus *);
>> -
>> /* Generic PCI functions used internally */
>>
>> void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-05-25 3:08 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-05-25 3:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Michal Simek, Hiroo Matsumoto, microblaze-uclinux,
Kenji Kaneshige, Jesse Larrew, jbarnes, Dominik Brodowski,
linux-pci, linuxppc-dev
On Thu, May 24, 2012 at 9:00 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
>> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>>
>> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
>> -> pcibios_fixup_bus() path. =A0Some hotplug drivers use that path, but
>> others don't, e.g., pciehp, so sometimes hot-added devices are only
>> partly initialized.
>>
>> This patch moves that initialization from pcibios_fixup_bus() to a new
>> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path=
.
>> That means the initialization happens the same way for all PCI devices,
>> whether they are present at boot or hot-added later.
>>
>> Note that powerpc was the only user of pci_fixup_cardbus(), which was
>> used to do this same setup for cardbus devices. =A0That's no longer
>> needed because this setup will now be done in the same device_add()
>> notification path as all other PCI devices.
>
> Hrm. That will require a good deal of testing... Unfortunately I'm out
> for a few weeks getting some surgery and then recovering...
>
> Our PCI code has ancient roots and I wouldn't be surprised if that
> change breaks subtle assumptions made here or there, we'd need to test
> at least on a good range of macs and ibm hotplug stuff.
OK. Are you worried about cardbus in particular?
This is headed for the 3.6, not 3.5, so we should have plenty of time.
As soon as everything for the current merge window gets merged and
-next is ready for the next batch, I'll put this in there.
>> Typical failure of a hot-added e1000e device prior to this change:
>>
>> =A0 =A0 # echo 1 > /sys/bus/pci/slots/1/power
>> =A0 =A0 <snip>
>> =A0 =A0 e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> =A0 =A0 e1000e 0000:03:00.0: No usable DMA configuration, aborting
>> =A0 =A0 e1000e: probe of 0000:03:00.0 failed with error -5
>>
>> Successful initialization after this change:
>>
>> =A0 =A0 # echo 1 > /sys/bus/pci/slots/1/power
>> =A0 =A0 <snip>
>> =A0 =A0 e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>> =A0 =A0 irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq=
27
>> =A0 =A0 e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:=
17:bf:c0:c9
>> =A0 =A0 e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>> =A0 =A0 e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>
>> [bhelgaas: changelog, notifier name, registration can be __init]
>> CC: Dominik Brodowski <linux@dominikbrodowski.net>
>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>> =A0arch/powerpc/include/asm/pci.h =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A02 -
>> =A0arch/powerpc/kernel/pci-common.c =A0 =A0 =A0 =A0 =A0 | =A0 87 +++++++=
+++++++--------------
>> =A0arch/powerpc/kernel/pci_32.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A02 +
>> =A0arch/powerpc/kernel/pci_64.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A02 +
>> =A0arch/powerpc/kernel/pci_of_scan.c =A0 =A0 =A0 =A0 =A0| =A0 =A01
>> =A0arch/powerpc/platforms/pseries/pci_dlpar.c | =A0 =A01
>> =A0drivers/pci/pci.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
| =A0 =A05 --
>> =A0drivers/pcmcia/cardbus.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =
=A03 -
>> =A0include/linux/pci.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| =
=A0 =A03 -
>> =A09 files changed, 49 insertions(+), 57 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/p=
ci.h
>> index 6653f27..d6a36a4 100644
>> --- a/arch/powerpc/include/asm/pci.h
>> +++ b/arch/powerpc/include/asm/pci.h
>> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_=
dev *dev, int bar,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0resource_=
size_t *start, resource_size_t *end);
>>
>> =A0extern resource_size_t pcibios_io_space_offset(struct pci_controller =
*hose);
>> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
>> =A0extern void pcibios_setup_bus_self(struct pci_bus *bus);
>> =A0extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
>> =A0extern void pcibios_scan_phb(struct pci_controller *hose);
>> +extern void pcibios_setup_bus_notifier(void);
>>
>> =A0#endif =A0 =A0 =A0 /* __KERNEL__ */
>> =A0#endif /* __ASM_POWERPC_PCI_H */
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-=
common.c
>> index 7320f36..41b39ba 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_=
bus *bus)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 ppc_md.pci_dma_bus_setup(bus);
>> =A0}
>>
>> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>> -{
>> - =A0 =A0 struct pci_dev *dev;
>> -
>> - =A0 =A0 pr_debug("PCI: Fixup bus devices %d (%s)\n",
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0bus->number, bus->self ? pci_name(bus->self=
) : "PHB");
>> -
>> - =A0 =A0 list_for_each_entry(dev, &bus->devices, bus_list) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 /* Cardbus can call us to add new devices to a=
bus, so ignore
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0* those who are already fully discovered
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> - =A0 =A0 =A0 =A0 =A0 =A0 if (dev->is_added)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 /* Fixup NUMA node as it may not be setup yet =
by the generic
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0* code and is needed by the DMA init
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> - =A0 =A0 =A0 =A0 =A0 =A0 set_dev_node(&dev->dev, pcibus_to_node(dev->bu=
s));
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 /* Hook up default DMA ops */
>> - =A0 =A0 =A0 =A0 =A0 =A0 set_dma_ops(&dev->dev, pci_dma_ops);
>> - =A0 =A0 =A0 =A0 =A0 =A0 set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 /* Additional platform DMA/iommu setup */
>> - =A0 =A0 =A0 =A0 =A0 =A0 if (ppc_md.pci_dma_dev_setup)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ppc_md.pci_dma_dev_setup(dev);
>> -
>> - =A0 =A0 =A0 =A0 =A0 =A0 /* Read default IRQs and fixup if necessary */
>> - =A0 =A0 =A0 =A0 =A0 =A0 pci_read_irq_line(dev);
>> - =A0 =A0 =A0 =A0 =A0 =A0 if (ppc_md.pci_irq_fixup)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ppc_md.pci_irq_fixup(dev);
>> - =A0 =A0 }
>> -}
>> -
>> =A0void pcibios_set_master(struct pci_dev *dev)
>> =A0{
>> =A0 =A0 =A0 /* No special bus mastering setup handling */
>> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *=
bus)
>>
>> =A0 =A0 =A0 /* Now fixup the bus bus */
>> =A0 =A0 =A0 pcibios_setup_bus_self(bus);
>> -
>> - =A0 =A0 /* Now fixup devices on that bus */
>> - =A0 =A0 pcibios_setup_bus_devices(bus);
>> =A0}
>> =A0EXPORT_SYMBOL(pcibios_fixup_bus);
>>
>> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
>> -{
>> - =A0 =A0 /* Now fixup devices on that bus */
>> - =A0 =A0 pcibios_setup_bus_devices(bus);
>> -}
>> -
>> -
>> =A0static int skip_isa_ioresource_align(struct pci_dev *dev)
>> =A0{
>> =A0 =A0 =A0 if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
>> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_contro=
ller *hose)
>> =A0 =A0 =A0 }
>> =A0}
>>
>> +static int pci_bus_notify(struct notifier_block *nb, unsigned long acti=
on,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 void *data)
>> +{
>> + =A0 =A0 struct pci_dev *dev =3D to_pci_dev(data);
>> +
>> + =A0 =A0 switch (action) {
>> + =A0 =A0 case BUS_NOTIFY_ADD_DEVICE:
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Setup OF node pointer in the device */
>> + =A0 =A0 =A0 =A0 =A0 =A0 dev->dev.of_node =3D pci_device_to_OF_node(dev=
);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Fixup NUMA node as it may not be setup yet =
by the generic
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* code and is needed by the DMA init
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> + =A0 =A0 =A0 =A0 =A0 =A0 set_dev_node(&dev->dev, pcibus_to_node(dev->bu=
s));
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Hook up default DMA ops */
>> + =A0 =A0 =A0 =A0 =A0 =A0 set_dma_ops(&dev->dev, pci_dma_ops);
>> + =A0 =A0 =A0 =A0 =A0 =A0 set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Additional platform DMA/iommu setup */
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (ppc_md.pci_dma_dev_setup)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ppc_md.pci_dma_dev_setup(dev);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* Read default IRQs and fixup if necessary */
>> + =A0 =A0 =A0 =A0 =A0 =A0 pci_read_irq_line(dev);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (ppc_md.pci_irq_fixup)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ppc_md.pci_irq_fixup(dev);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 break;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return 0;
>> +}
>> +
>> +static struct notifier_block device_nb =3D {
>> + =A0 =A0 .notifier_call =3D pci_bus_notify,
>> +};
>> +
>> +void __init pcibios_setup_bus_notifier(void)
>> +{
>> + =A0 =A0 bus_register_notifier(&pci_bus_type, &device_nb);
>> +}
>> +
>> =A0static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>> =A0{
>> =A0 =A0 =A0 int i, class =3D dev->class >> 8;
>> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
>> index 4b06ec5..640cc35 100644
>> --- a/arch/powerpc/kernel/pci_32.c
>> +++ b/arch/powerpc/kernel/pci_32.c
>> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>>
>> =A0 =A0 =A0 printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>
>> + =A0 =A0 pcibios_setup_bus_notifier();
>> +
>> =A0 =A0 =A0 if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 pci_assign_all_buses =3D 1;
>>
>> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>> index 4ff190f..8b212d3 100644
>> --- a/arch/powerpc/kernel/pci_64.c
>> +++ b/arch/powerpc/kernel/pci_64.c
>> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>>
>> =A0 =A0 =A0 printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>
>> + =A0 =A0 pcibios_setup_bus_notifier();
>> +
>> =A0 =A0 =A0 /* For now, override phys_mem_access_prot. If we need it,g
>> =A0 =A0 =A0 =A0* later, we may move that initialization to each ppc_md
>> =A0 =A0 =A0 =A0*/
>> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci=
_of_scan.c
>> index ae5ea5e..eb09eca 100644
>> --- a/arch/powerpc/kernel/pci_of_scan.c
>> +++ b/arch/powerpc/kernel/pci_of_scan.c
>> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_no=
de *node,
>> =A0 =A0 =A0 =A0*/
>> =A0 =A0 =A0 if (!rescan_existing)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 pcibios_setup_bus_self(bus);
>> - =A0 =A0 pcibios_setup_bus_devices(bus);
>>
>> =A0 =A0 =A0 /* Now scan child busses */
>> =A0 =A0 =A0 list_for_each_entry(dev, &bus->devices, bus_list) {
>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/p=
latforms/pseries/pci_dlpar.c
>> index 3ccebc8..0b1b6b3 100644
>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 num =3D pci_scan_slot(bus, PCI_DEVFN(slotno,=
0));
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!num)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
>> - =A0 =A0 =A0 =A0 =A0 =A0 pcibios_setup_bus_devices(bus);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 max =3D bus->busn_res.start;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (pass=3D0; pass < 2; pass++)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 list_for_each_entry(dev, &bu=
s->devices, bus_list) {
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 15d442a..43e0a4f 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(stru=
ct pci_dev *dev)
>> =A0 =A0 =A0 return 1;
>> =A0}
>>
>> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
>> -{
>> -}
>> -EXPORT_SYMBOL(pci_fixup_cardbus);
>> -
>> =A0static int __init pci_setup(char *str)
>> =A0{
>> =A0 =A0 =A0 while (str) {
>> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
>> index 24caeaf..a980691 100644
>> --- a/drivers/pcmcia/cardbus.c
>> +++ b/drivers/pcmcia/cardbus.c
>> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>> =A0 =A0 =A0 unsigned int max, pass;
>>
>> =A0 =A0 =A0 s->functions =3D pci_scan_slot(bus, PCI_DEVFN(0, 0));
>> - =A0 =A0 pci_fixup_cardbus(bus);
>>
>> =A0 =A0 =A0 max =3D bus->busn_res.start;
>> =A0 =A0 =A0 for (pass =3D 0; pass < 2; pass++)
>> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>> =A0 =A0 =A0 =A0*/
>> =A0 =A0 =A0 pci_bus_size_bridges(bus);
>> =A0 =A0 =A0 pci_bus_assign_resources(bus);
>> - =A0 =A0 cardbus_config_irq_and_cls(bus, s->pci_irq);
>>
>> =A0 =A0 =A0 /* socket specific tune function */
>> =A0 =A0 =A0 if (s->tune_bridge)
>> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>
>> =A0 =A0 =A0 pci_enable_bridges(bus);
>> =A0 =A0 =A0 pci_bus_add_devices(bus);
>> + =A0 =A0 cardbus_config_irq_and_cls(bus, s->pci_irq);
>>
>> =A0 =A0 =A0 return 0;
>> =A0}
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index a0e2d7f..3924c02 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const=
struct resource *,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 resource_siz=
e_t);
>> =A0void pcibios_update_irq(struct pci_dev *, int irq);
>>
>> -/* Weak but can be overriden by arch */
>> -void pci_fixup_cardbus(struct pci_bus *);
>> -
>> =A0/* Generic PCI functions used internally */
>>
>> =A0void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_regi=
on *region,
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-25 3:08 ` Bjorn Helgaas
@ 2012-05-25 3:38 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-05-25 3:38 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Hiroo Matsumoto, Michal Simek, microblaze-uclinux, linux-pci,
Jesse Larrew, jbarnes, Dominik Brodowski, Kenji Kaneshige,
linuxppc-dev
On Thu, 2012-05-24 at 21:08 -0600, Bjorn Helgaas wrote:
>
> OK. Are you worried about cardbus in particular?
>
> This is headed for the 3.6, not 3.5, so we should have plenty of time.
> As soon as everything for the current merge window gets merged and
> -next is ready for the next batch, I'll put this in there.
I'm not that worried about cardbus ... in fact I wouldn't be surprised
if it's already somewhat broken, I haven't tested it in a long time...
I'm more worried about basic functionality and expectations about when
things happen during boot vs. platform hacks, that and pseries specific
kind of hotplug which can be a bit odd.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-05-25 3:38 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-05-25 3:38 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Michal Simek, Hiroo Matsumoto, microblaze-uclinux,
Kenji Kaneshige, Jesse Larrew, jbarnes, Dominik Brodowski,
linux-pci, linuxppc-dev
On Thu, 2012-05-24 at 21:08 -0600, Bjorn Helgaas wrote:
>
> OK. Are you worried about cardbus in particular?
>
> This is headed for the 3.6, not 3.5, so we should have plenty of time.
> As soon as everything for the current merge window gets merged and
> -next is ready for the next batch, I'll put this in there.
I'm not that worried about cardbus ... in fact I wouldn't be surprised
if it's already somewhat broken, I haven't tested it in a long time...
I'm more worried about basic functionality and expectations about when
things happen during boot vs. platform hacks, that and pseries specific
kind of hotplug which can be a bit odd.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-25 3:08 ` Bjorn Helgaas
@ 2012-06-07 12:58 ` Hiroo Matsumoto
-1 siblings, 0 replies; 20+ messages in thread
From: Hiroo Matsumoto @ 2012-06-07 12:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Benjamin Herrenschmidt, Michal Simek, microblaze-uclinux,
linux-pci, Jesse Larrew, jbarnes, Dominik Brodowski,
Kenji Kaneshige, linuxppc-dev
Thanks for your rapid modification about function name and comments.
As Ben said, I want to check that my patch does not cause problems.
I have no complain about my patch being in 3.6 branch (or lately).
I appreciate your time and effort.
Regards.
Hiroo MATSUMOTO
> On Thu, May 24, 2012 at 9:00 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
>>> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>>>
>>> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
>>> -> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
>>> others don't, e.g., pciehp, so sometimes hot-added devices are only
>>> partly initialized.
>>>
>>> This patch moves that initialization from pcibios_fixup_bus() to a new
>>> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
>>> That means the initialization happens the same way for all PCI devices,
>>> whether they are present at boot or hot-added later.
>>>
>>> Note that powerpc was the only user of pci_fixup_cardbus(), which was
>>> used to do this same setup for cardbus devices. That's no longer
>>> needed because this setup will now be done in the same device_add()
>>> notification path as all other PCI devices.
>> Hrm. That will require a good deal of testing... Unfortunately I'm out
>> for a few weeks getting some surgery and then recovering...
>>
>> Our PCI code has ancient roots and I wouldn't be surprised if that
>> change breaks subtle assumptions made here or there, we'd need to test
>> at least on a good range of macs and ibm hotplug stuff.
>
> OK. Are you worried about cardbus in particular?
>
> This is headed for the 3.6, not 3.5, so we should have plenty of time.
> As soon as everything for the current merge window gets merged and
> -next is ready for the next batch, I'll put this in there.
>
>>> Typical failure of a hot-added e1000e device prior to this change:
>>>
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> e1000e 0000:03:00.0: No usable DMA configuration, aborting
>>> e1000e: probe of 0000:03:00.0 failed with error -5
>>>
>>> Successful initialization after this change:
>>>
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>>> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>>> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>>> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>>
>>> [bhelgaas: changelog, notifier name, registration can be __init]
>>> CC: Dominik Brodowski <linux@dominikbrodowski.net>
>>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>> arch/powerpc/include/asm/pci.h | 2 -
>>> arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
>>> arch/powerpc/kernel/pci_32.c | 2 +
>>> arch/powerpc/kernel/pci_64.c | 2 +
>>> arch/powerpc/kernel/pci_of_scan.c | 1
>>> arch/powerpc/platforms/pseries/pci_dlpar.c | 1
>>> drivers/pci/pci.c | 5 --
>>> drivers/pcmcia/cardbus.c | 3 -
>>> include/linux/pci.h | 3 -
>>> 9 files changed, 49 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
>>> index 6653f27..d6a36a4 100644
>>> --- a/arch/powerpc/include/asm/pci.h
>>> +++ b/arch/powerpc/include/asm/pci.h
>>> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
>>> resource_size_t *start, resource_size_t *end);
>>>
>>> extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
>>> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
>>> extern void pcibios_setup_bus_self(struct pci_bus *bus);
>>> extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
>>> extern void pcibios_scan_phb(struct pci_controller *hose);
>>> +extern void pcibios_setup_bus_notifier(void);
>>>
>>> #endif /* __KERNEL__ */
>>> #endif /* __ASM_POWERPC_PCI_H */
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index 7320f36..41b39ba 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
>>> ppc_md.pci_dma_bus_setup(bus);
>>> }
>>>
>>> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>>> -{
>>> - struct pci_dev *dev;
>>> -
>>> - pr_debug("PCI: Fixup bus devices %d (%s)\n",
>>> - bus->number, bus->self ? pci_name(bus->self) : "PHB");
>>> -
>>> - list_for_each_entry(dev, &bus->devices, bus_list) {
>>> - /* Cardbus can call us to add new devices to a bus, so ignore
>>> - * those who are already fully discovered
>>> - */
>>> - if (dev->is_added)
>>> - continue;
>>> -
>>> - /* Fixup NUMA node as it may not be setup yet by the generic
>>> - * code and is needed by the DMA init
>>> - */
>>> - set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>> -
>>> - /* Hook up default DMA ops */
>>> - set_dma_ops(&dev->dev, pci_dma_ops);
>>> - set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>>> -
>>> - /* Additional platform DMA/iommu setup */
>>> - if (ppc_md.pci_dma_dev_setup)
>>> - ppc_md.pci_dma_dev_setup(dev);
>>> -
>>> - /* Read default IRQs and fixup if necessary */
>>> - pci_read_irq_line(dev);
>>> - if (ppc_md.pci_irq_fixup)
>>> - ppc_md.pci_irq_fixup(dev);
>>> - }
>>> -}
>>> -
>>> void pcibios_set_master(struct pci_dev *dev)
>>> {
>>> /* No special bus mastering setup handling */
>>> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>>>
>>> /* Now fixup the bus bus */
>>> pcibios_setup_bus_self(bus);
>>> -
>>> - /* Now fixup devices on that bus */
>>> - pcibios_setup_bus_devices(bus);
>>> }
>>> EXPORT_SYMBOL(pcibios_fixup_bus);
>>>
>>> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> - /* Now fixup devices on that bus */
>>> - pcibios_setup_bus_devices(bus);
>>> -}
>>> -
>>> -
>>> static int skip_isa_ioresource_align(struct pci_dev *dev)
>>> {
>>> if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
>>> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
>>> }
>>> }
>>>
>>> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct pci_dev *dev = to_pci_dev(data);
>>> +
>>> + switch (action) {
>>> + case BUS_NOTIFY_ADD_DEVICE:
>>> + /* Setup OF node pointer in the device */
>>> + dev->dev.of_node = pci_device_to_OF_node(dev);
>>> +
>>> + /* Fixup NUMA node as it may not be setup yet by the generic
>>> + * code and is needed by the DMA init
>>> + */
>>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>> +
>>> + /* Hook up default DMA ops */
>>> + set_dma_ops(&dev->dev, pci_dma_ops);
>>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>>> +
>>> + /* Additional platform DMA/iommu setup */
>>> + if (ppc_md.pci_dma_dev_setup)
>>> + ppc_md.pci_dma_dev_setup(dev);
>>> +
>>> + /* Read default IRQs and fixup if necessary */
>>> + pci_read_irq_line(dev);
>>> + if (ppc_md.pci_irq_fixup)
>>> + ppc_md.pci_irq_fixup(dev);
>>> +
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct notifier_block device_nb = {
>>> + .notifier_call = pci_bus_notify,
>>> +};
>>> +
>>> +void __init pcibios_setup_bus_notifier(void)
>>> +{
>>> + bus_register_notifier(&pci_bus_type, &device_nb);
>>> +}
>>> +
>>> static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>>> {
>>> int i, class = dev->class >> 8;
>>> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
>>> index 4b06ec5..640cc35 100644
>>> --- a/arch/powerpc/kernel/pci_32.c
>>> +++ b/arch/powerpc/kernel/pci_32.c
>>> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>>>
>>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> + pcibios_setup_bus_notifier();
>>> +
>>> if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>>> pci_assign_all_buses = 1;
>>>
>>> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>>> index 4ff190f..8b212d3 100644
>>> --- a/arch/powerpc/kernel/pci_64.c
>>> +++ b/arch/powerpc/kernel/pci_64.c
>>> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>>>
>>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> + pcibios_setup_bus_notifier();
>>> +
>>> /* For now, override phys_mem_access_prot. If we need it,g
>>> * later, we may move that initialization to each ppc_md
>>> */
>>> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
>>> index ae5ea5e..eb09eca 100644
>>> --- a/arch/powerpc/kernel/pci_of_scan.c
>>> +++ b/arch/powerpc/kernel/pci_of_scan.c
>>> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
>>> */
>>> if (!rescan_existing)
>>> pcibios_setup_bus_self(bus);
>>> - pcibios_setup_bus_devices(bus);
>>>
>>> /* Now scan child busses */
>>> list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> index 3ccebc8..0b1b6b3 100644
>>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
>>> num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>>> if (!num)
>>> return;
>>> - pcibios_setup_bus_devices(bus);
>>> max = bus->busn_res.start;
>>> for (pass=0; pass < 2; pass++)
>>> list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 15d442a..43e0a4f 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
>>> return 1;
>>> }
>>>
>>> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> -}
>>> -EXPORT_SYMBOL(pci_fixup_cardbus);
>>> -
>>> static int __init pci_setup(char *str)
>>> {
>>> while (str) {
>>> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
>>> index 24caeaf..a980691 100644
>>> --- a/drivers/pcmcia/cardbus.c
>>> +++ b/drivers/pcmcia/cardbus.c
>>> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>> unsigned int max, pass;
>>>
>>> s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
>>> - pci_fixup_cardbus(bus);
>>>
>>> max = bus->busn_res.start;
>>> for (pass = 0; pass < 2; pass++)
>>> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>> */
>>> pci_bus_size_bridges(bus);
>>> pci_bus_assign_resources(bus);
>>> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>> /* socket specific tune function */
>>> if (s->tune_bridge)
>>> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>>
>>> pci_enable_bridges(bus);
>>> pci_bus_add_devices(bus);
>>> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>> return 0;
>>> }
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index a0e2d7f..3924c02 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
>>> resource_size_t);
>>> void pcibios_update_irq(struct pci_dev *, int irq);
>>>
>>> -/* Weak but can be overriden by arch */
>>> -void pci_fixup_cardbus(struct pci_bus *);
>>> -
>>> /* Generic PCI functions used internally */
>>>
>>> void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-06-07 12:58 ` Hiroo Matsumoto
0 siblings, 0 replies; 20+ messages in thread
From: Hiroo Matsumoto @ 2012-06-07 12:58 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Michal Simek, microblaze-uclinux, Kenji Kaneshige, Jesse Larrew,
jbarnes, Dominik Brodowski, linux-pci, linuxppc-dev
Thanks for your rapid modification about function name and comments.
As Ben said, I want to check that my patch does not cause problems.
I have no complain about my patch being in 3.6 branch (or lately).
I appreciate your time and effort.
Regards.
Hiroo MATSUMOTO
> On Thu, May 24, 2012 at 9:00 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Wed, 2012-05-23 at 16:37 -0600, Bjorn Helgaas wrote:
>>> From: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
>>>
>>> PowerPC initialized DMA and IRQ information in the pci_scan_child_bus()
>>> -> pcibios_fixup_bus() path. Some hotplug drivers use that path, but
>>> others don't, e.g., pciehp, so sometimes hot-added devices are only
>>> partly initialized.
>>>
>>> This patch moves that initialization from pcibios_fixup_bus() to a new
>>> pci_bus_notify() called in the pci_bus_add_device() -> device_add() path.
>>> That means the initialization happens the same way for all PCI devices,
>>> whether they are present at boot or hot-added later.
>>>
>>> Note that powerpc was the only user of pci_fixup_cardbus(), which was
>>> used to do this same setup for cardbus devices. That's no longer
>>> needed because this setup will now be done in the same device_add()
>>> notification path as all other PCI devices.
>> Hrm. That will require a good deal of testing... Unfortunately I'm out
>> for a few weeks getting some surgery and then recovering...
>>
>> Our PCI code has ancient roots and I wouldn't be surprised if that
>> change breaks subtle assumptions made here or there, we'd need to test
>> at least on a good range of macs and ibm hotplug stuff.
>
> OK. Are you worried about cardbus in particular?
>
> This is headed for the 3.6, not 3.5, so we should have plenty of time.
> As soon as everything for the current merge window gets merged and
> -next is ready for the next batch, I'll put this in there.
>
>>> Typical failure of a hot-added e1000e device prior to this change:
>>>
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> e1000e 0000:03:00.0: No usable DMA configuration, aborting
>>> e1000e: probe of 0000:03:00.0 failed with error -5
>>>
>>> Successful initialization after this change:
>>>
>>> # echo 1 > /sys/bus/pci/slots/1/power
>>> <snip>
>>> e1000e 0000:03:00.0: enabling device (0000 -> 0002)
>>> irq: irq 6 on host /soc@ffe00000/msi@41600 mapped to virtual irq 27
>>> e1000e 0000:03:00.0: eth0: (PCI Express:2.5GT/s:Width x1) 00:15:17:bf:c0:c9
>>> e1000e 0000:03:00.0: eth0: Intel(R) PRO/1000 Network Connection
>>> e1000e 0000:03:00.0: eth0: MAC: 1, PHY: 4, PBA No: D50861-003
>>>
>>> [bhelgaas: changelog, notifier name, registration can be __init]
>>> CC: Dominik Brodowski <linux@dominikbrodowski.net>
>>> Signed-off-by: Hiroo MATSUMOTO <matsumoto.hiroo@jp.fujitsu.com>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>> arch/powerpc/include/asm/pci.h | 2 -
>>> arch/powerpc/kernel/pci-common.c | 87 ++++++++++++++--------------
>>> arch/powerpc/kernel/pci_32.c | 2 +
>>> arch/powerpc/kernel/pci_64.c | 2 +
>>> arch/powerpc/kernel/pci_of_scan.c | 1
>>> arch/powerpc/platforms/pseries/pci_dlpar.c | 1
>>> drivers/pci/pci.c | 5 --
>>> drivers/pcmcia/cardbus.c | 3 -
>>> include/linux/pci.h | 3 -
>>> 9 files changed, 49 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
>>> index 6653f27..d6a36a4 100644
>>> --- a/arch/powerpc/include/asm/pci.h
>>> +++ b/arch/powerpc/include/asm/pci.h
>>> @@ -183,10 +183,10 @@ extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
>>> resource_size_t *start, resource_size_t *end);
>>>
>>> extern resource_size_t pcibios_io_space_offset(struct pci_controller *hose);
>>> -extern void pcibios_setup_bus_devices(struct pci_bus *bus);
>>> extern void pcibios_setup_bus_self(struct pci_bus *bus);
>>> extern void pcibios_setup_phb_io_space(struct pci_controller *hose);
>>> extern void pcibios_scan_phb(struct pci_controller *hose);
>>> +extern void pcibios_setup_bus_notifier(void);
>>>
>>> #endif /* __KERNEL__ */
>>> #endif /* __ASM_POWERPC_PCI_H */
>>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>> index 7320f36..41b39ba 100644
>>> --- a/arch/powerpc/kernel/pci-common.c
>>> +++ b/arch/powerpc/kernel/pci-common.c
>>> @@ -1009,40 +1009,6 @@ void __devinit pcibios_setup_bus_self(struct pci_bus *bus)
>>> ppc_md.pci_dma_bus_setup(bus);
>>> }
>>>
>>> -void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>>> -{
>>> - struct pci_dev *dev;
>>> -
>>> - pr_debug("PCI: Fixup bus devices %d (%s)\n",
>>> - bus->number, bus->self ? pci_name(bus->self) : "PHB");
>>> -
>>> - list_for_each_entry(dev, &bus->devices, bus_list) {
>>> - /* Cardbus can call us to add new devices to a bus, so ignore
>>> - * those who are already fully discovered
>>> - */
>>> - if (dev->is_added)
>>> - continue;
>>> -
>>> - /* Fixup NUMA node as it may not be setup yet by the generic
>>> - * code and is needed by the DMA init
>>> - */
>>> - set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>> -
>>> - /* Hook up default DMA ops */
>>> - set_dma_ops(&dev->dev, pci_dma_ops);
>>> - set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>>> -
>>> - /* Additional platform DMA/iommu setup */
>>> - if (ppc_md.pci_dma_dev_setup)
>>> - ppc_md.pci_dma_dev_setup(dev);
>>> -
>>> - /* Read default IRQs and fixup if necessary */
>>> - pci_read_irq_line(dev);
>>> - if (ppc_md.pci_irq_fixup)
>>> - ppc_md.pci_irq_fixup(dev);
>>> - }
>>> -}
>>> -
>>> void pcibios_set_master(struct pci_dev *dev)
>>> {
>>> /* No special bus mastering setup handling */
>>> @@ -1059,19 +1025,9 @@ void __devinit pcibios_fixup_bus(struct pci_bus *bus)
>>>
>>> /* Now fixup the bus bus */
>>> pcibios_setup_bus_self(bus);
>>> -
>>> - /* Now fixup devices on that bus */
>>> - pcibios_setup_bus_devices(bus);
>>> }
>>> EXPORT_SYMBOL(pcibios_fixup_bus);
>>>
>>> -void __devinit pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> - /* Now fixup devices on that bus */
>>> - pcibios_setup_bus_devices(bus);
>>> -}
>>> -
>>> -
>>> static int skip_isa_ioresource_align(struct pci_dev *dev)
>>> {
>>> if (pci_has_flag(PCI_CAN_SKIP_ISA_ALIGN) &&
>>> @@ -1685,6 +1641,49 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
>>> }
>>> }
>>>
>>> +static int pci_bus_notify(struct notifier_block *nb, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct pci_dev *dev = to_pci_dev(data);
>>> +
>>> + switch (action) {
>>> + case BUS_NOTIFY_ADD_DEVICE:
>>> + /* Setup OF node pointer in the device */
>>> + dev->dev.of_node = pci_device_to_OF_node(dev);
>>> +
>>> + /* Fixup NUMA node as it may not be setup yet by the generic
>>> + * code and is needed by the DMA init
>>> + */
>>> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
>>> +
>>> + /* Hook up default DMA ops */
>>> + set_dma_ops(&dev->dev, pci_dma_ops);
>>> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
>>> +
>>> + /* Additional platform DMA/iommu setup */
>>> + if (ppc_md.pci_dma_dev_setup)
>>> + ppc_md.pci_dma_dev_setup(dev);
>>> +
>>> + /* Read default IRQs and fixup if necessary */
>>> + pci_read_irq_line(dev);
>>> + if (ppc_md.pci_irq_fixup)
>>> + ppc_md.pci_irq_fixup(dev);
>>> +
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct notifier_block device_nb = {
>>> + .notifier_call = pci_bus_notify,
>>> +};
>>> +
>>> +void __init pcibios_setup_bus_notifier(void)
>>> +{
>>> + bus_register_notifier(&pci_bus_type, &device_nb);
>>> +}
>>> +
>>> static void fixup_hide_host_resource_fsl(struct pci_dev *dev)
>>> {
>>> int i, class = dev->class >> 8;
>>> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
>>> index 4b06ec5..640cc35 100644
>>> --- a/arch/powerpc/kernel/pci_32.c
>>> +++ b/arch/powerpc/kernel/pci_32.c
>>> @@ -231,6 +231,8 @@ static int __init pcibios_init(void)
>>>
>>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> + pcibios_setup_bus_notifier();
>>> +
>>> if (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>>> pci_assign_all_buses = 1;
>>>
>>> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>>> index 4ff190f..8b212d3 100644
>>> --- a/arch/powerpc/kernel/pci_64.c
>>> +++ b/arch/powerpc/kernel/pci_64.c
>>> @@ -48,6 +48,8 @@ static int __init pcibios_init(void)
>>>
>>> printk(KERN_INFO "PCI: Probing PCI hardware\n");
>>>
>>> + pcibios_setup_bus_notifier();
>>> +
>>> /* For now, override phys_mem_access_prot. If we need it,g
>>> * later, we may move that initialization to each ppc_md
>>> */
>>> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
>>> index ae5ea5e..eb09eca 100644
>>> --- a/arch/powerpc/kernel/pci_of_scan.c
>>> +++ b/arch/powerpc/kernel/pci_of_scan.c
>>> @@ -333,7 +333,6 @@ static void __devinit __of_scan_bus(struct device_node *node,
>>> */
>>> if (!rescan_existing)
>>> pcibios_setup_bus_self(bus);
>>> - pcibios_setup_bus_devices(bus);
>>>
>>> /* Now scan child busses */
>>> list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> index 3ccebc8..0b1b6b3 100644
>>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>>> @@ -120,7 +120,6 @@ void pcibios_add_pci_devices(struct pci_bus * bus)
>>> num = pci_scan_slot(bus, PCI_DEVFN(slotno, 0));
>>> if (!num)
>>> return;
>>> - pcibios_setup_bus_devices(bus);
>>> max = bus->busn_res.start;
>>> for (pass=0; pass < 2; pass++)
>>> list_for_each_entry(dev, &bus->devices, bus_list) {
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 15d442a..43e0a4f 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -3789,11 +3789,6 @@ int __attribute__ ((weak)) pci_ext_cfg_avail(struct pci_dev *dev)
>>> return 1;
>>> }
>>>
>>> -void __weak pci_fixup_cardbus(struct pci_bus *bus)
>>> -{
>>> -}
>>> -EXPORT_SYMBOL(pci_fixup_cardbus);
>>> -
>>> static int __init pci_setup(char *str)
>>> {
>>> while (str) {
>>> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
>>> index 24caeaf..a980691 100644
>>> --- a/drivers/pcmcia/cardbus.c
>>> +++ b/drivers/pcmcia/cardbus.c
>>> @@ -71,7 +71,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>> unsigned int max, pass;
>>>
>>> s->functions = pci_scan_slot(bus, PCI_DEVFN(0, 0));
>>> - pci_fixup_cardbus(bus);
>>>
>>> max = bus->busn_res.start;
>>> for (pass = 0; pass < 2; pass++)
>>> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>> */
>>> pci_bus_size_bridges(bus);
>>> pci_bus_assign_resources(bus);
>>> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>> /* socket specific tune function */
>>> if (s->tune_bridge)
>>> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>>>
>>> pci_enable_bridges(bus);
>>> pci_bus_add_devices(bus);
>>> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>>>
>>> return 0;
>>> }
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index a0e2d7f..3924c02 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -651,9 +651,6 @@ resource_size_t pcibios_align_resource(void *, const struct resource *,
>>> resource_size_t);
>>> void pcibios_update_irq(struct pci_dev *, int irq);
>>>
>>> -/* Weak but can be overriden by arch */
>>> -void pci_fixup_cardbus(struct pci_bus *);
>>> -
>>> /* Generic PCI functions used internally */
>>>
>>> void pcibios_resource_to_bus(struct pci_dev *dev, struct pci_bus_region *region,
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-05-23 22:37 ` Bjorn Helgaas
@ 2012-06-18 21:06 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-06-18 21:06 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, benh, microblaze-uclinux, linux-pci, Jesse Larrew,
jbarnes, Dominik Brodowski, Kenji Kaneshige, linuxppc-dev
I'm trying to make some progress on these patches, but I'm concerned
about this bit:
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index 24caeaf..a980691 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> */
> pci_bus_size_bridges(bus);
> pci_bus_assign_resources(bus);
> - cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> /* socket specific tune function */
> if (s->tune_bridge)
> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>
> pci_enable_bridges(bus);
> pci_bus_add_devices(bus);
> + cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> return 0;
> }
We're moving the CardBus IRQ config from before pci_bus_add_devices()
to after. I see why you did that: we're proposing to do the powerpc
DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
powerpc IRQ init clobber the CardBus IRQ config.
But a driver can claim the device as soon as we call
pci_bus_add_devices(), so we're potentially changing dev->irq after a
driver has already looked at it, which sounds like a bug.
There are only five possibilities for powerpc pci_irq_fixup:
ppc47x_pci_irq_fixup
mpc85xx_cds_pci_irq_fixup
maple_pci_irq_fixup
pmac_pci_irq_fixup
rtas_msi_pci_irq_fixup
If these were normal PCI header quirks instead, they could run
earlier, and we wouldn't need to move this
cardbus_config_irq_and_cls() call. Is it possible to make these
quirks, Ben?
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-06-18 21:06 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-06-18 21:06 UTC (permalink / raw)
To: Hiroo Matsumoto
Cc: Michal Simek, microblaze-uclinux, Kenji Kaneshige, Jesse Larrew,
jbarnes, Dominik Brodowski, linux-pci, linuxppc-dev
I'm trying to make some progress on these patches, but I'm concerned
about this bit:
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index 24caeaf..a980691 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -85,7 +84,6 @@ int __ref cb_alloc(struct pcmcia_socket *s)
> =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0pci_bus_size_bridges(bus);
> =A0 =A0 =A0 =A0pci_bus_assign_resources(bus);
> - =A0 =A0 =A0 cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> =A0 =A0 =A0 =A0/* socket specific tune function */
> =A0 =A0 =A0 =A0if (s->tune_bridge)
> @@ -93,6 +91,7 @@ int __ref cb_alloc(struct pcmcia_socket *s)
>
> =A0 =A0 =A0 =A0pci_enable_bridges(bus);
> =A0 =A0 =A0 =A0pci_bus_add_devices(bus);
> + =A0 =A0 =A0 cardbus_config_irq_and_cls(bus, s->pci_irq);
>
> =A0 =A0 =A0 =A0return 0;
> =A0}
We're moving the CardBus IRQ config from before pci_bus_add_devices()
to after. I see why you did that: we're proposing to do the powerpc
DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
powerpc IRQ init clobber the CardBus IRQ config.
But a driver can claim the device as soon as we call
pci_bus_add_devices(), so we're potentially changing dev->irq after a
driver has already looked at it, which sounds like a bug.
There are only five possibilities for powerpc pci_irq_fixup:
ppc47x_pci_irq_fixup
mpc85xx_cds_pci_irq_fixup
maple_pci_irq_fixup
pmac_pci_irq_fixup
rtas_msi_pci_irq_fixup
If these were normal PCI header quirks instead, they could run
earlier, and we wouldn't need to move this
cardbus_config_irq_and_cls() call. Is it possible to make these
quirks, Ben?
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-06-18 21:06 ` Bjorn Helgaas
@ 2012-06-18 22:55 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-06-18 22:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Hiroo Matsumoto, Michal Simek, microblaze-uclinux, linux-pci,
Jesse Larrew, jbarnes, Dominik Brodowski, Kenji Kaneshige,
linuxppc-dev
On Mon, 2012-06-18 at 15:06 -0600, Bjorn Helgaas wrote:
> We're moving the CardBus IRQ config from before pci_bus_add_devices()
> to after. I see why you did that: we're proposing to do the powerpc
> DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
> powerpc IRQ init clobber the CardBus IRQ config.
>
> But a driver can claim the device as soon as we call
> pci_bus_add_devices(), so we're potentially changing dev->irq after a
> driver has already looked at it, which sounds like a bug.
>
> There are only five possibilities for powerpc pci_irq_fixup:
>
> ppc47x_pci_irq_fixup
> mpc85xx_cds_pci_irq_fixup
> maple_pci_irq_fixup
> pmac_pci_irq_fixup
> rtas_msi_pci_irq_fixup
>
> If these were normal PCI header quirks instead, they could run
> earlier, and we wouldn't need to move this
> cardbus_config_irq_and_cls() call. Is it possible to make these
> quirks, Ben?
Wait ... why are those fixups relevant ? They have to run after
pci_read_irq_line() (which should have been called pcibios_read_irq_line
really) but that's fine, we call both back to back....
The problem has to do with the fact that we setup pdev->irq inside
pci_bus_add_devices() with the new proposed code (the fixup itself is
just a detail).
You want cardbus to "quirk" the irq after that's been fixed up... maybe
that's a case for moving cardbus_config_irq_and_cls() to
pci_enable_device() ? Or add another hook inside
pci_bus_add_devices()...
Cheers,
Ben.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-06-18 22:55 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Herrenschmidt @ 2012-06-18 22:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Michal Simek, Hiroo Matsumoto, microblaze-uclinux,
Kenji Kaneshige, Jesse Larrew, jbarnes, Dominik Brodowski,
linux-pci, linuxppc-dev
On Mon, 2012-06-18 at 15:06 -0600, Bjorn Helgaas wrote:
> We're moving the CardBus IRQ config from before pci_bus_add_devices()
> to after. I see why you did that: we're proposing to do the powerpc
> DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
> powerpc IRQ init clobber the CardBus IRQ config.
>
> But a driver can claim the device as soon as we call
> pci_bus_add_devices(), so we're potentially changing dev->irq after a
> driver has already looked at it, which sounds like a bug.
>
> There are only five possibilities for powerpc pci_irq_fixup:
>
> ppc47x_pci_irq_fixup
> mpc85xx_cds_pci_irq_fixup
> maple_pci_irq_fixup
> pmac_pci_irq_fixup
> rtas_msi_pci_irq_fixup
>
> If these were normal PCI header quirks instead, they could run
> earlier, and we wouldn't need to move this
> cardbus_config_irq_and_cls() call. Is it possible to make these
> quirks, Ben?
Wait ... why are those fixups relevant ? They have to run after
pci_read_irq_line() (which should have been called pcibios_read_irq_line
really) but that's fine, we call both back to back....
The problem has to do with the fact that we setup pdev->irq inside
pci_bus_add_devices() with the new proposed code (the fixup itself is
just a detail).
You want cardbus to "quirk" the irq after that's been fixed up... maybe
that's a case for moving cardbus_config_irq_and_cls() to
pci_enable_device() ? Or add another hook inside
pci_bus_add_devices()...
Cheers,
Ben.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
2012-06-18 22:55 ` Benjamin Herrenschmidt
@ 2012-10-12 18:03 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-10-12 18:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Hiroo Matsumoto, Michal Simek, microblaze-uclinux, linux-pci,
Jesse Larrew, jbarnes, Dominik Brodowski, Kenji Kaneshige,
linuxppc-dev
On Mon, Jun 18, 2012 at 4:55 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-06-18 at 15:06 -0600, Bjorn Helgaas wrote:
>> We're moving the CardBus IRQ config from before pci_bus_add_devices()
>> to after. I see why you did that: we're proposing to do the powerpc
>> DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
>> powerpc IRQ init clobber the CardBus IRQ config.
>>
>> But a driver can claim the device as soon as we call
>> pci_bus_add_devices(), so we're potentially changing dev->irq after a
>> driver has already looked at it, which sounds like a bug.
>>
>> There are only five possibilities for powerpc pci_irq_fixup:
>>
>> ppc47x_pci_irq_fixup
>> mpc85xx_cds_pci_irq_fixup
>> maple_pci_irq_fixup
>> pmac_pci_irq_fixup
>> rtas_msi_pci_irq_fixup
>>
>> If these were normal PCI header quirks instead, they could run
>> earlier, and we wouldn't need to move this
>> cardbus_config_irq_and_cls() call. Is it possible to make these
>> quirks, Ben?
>
> Wait ... why are those fixups relevant ? They have to run after
> pci_read_irq_line() (which should have been called pcibios_read_irq_line
> really) but that's fine, we call both back to back....
>
> The problem has to do with the fact that we setup pdev->irq inside
> pci_bus_add_devices() with the new proposed code (the fixup itself is
> just a detail).
>
> You want cardbus to "quirk" the irq after that's been fixed up... maybe
> that's a case for moving cardbus_config_irq_and_cls() to
> pci_enable_device() ? Or add another hook inside
> pci_bus_add_devices()...
This thread has languished a long time, and I think the original
problem (hot-added e1000e devices on powerpc don't work) still exists,
doesn't it?
I'd like to get a kernel bugzilla opened so (a) this doesn't get
forgotten and (b) we can attach things like dmesg logs showing the
issue.
The current patch does this:
> pci_bus_add_devices(bus);
> + cardbus_config_irq_and_cls(bus, s->pci_irq);
I don't think we can do this, because pci_bus_add_devices() can bind a
driver to the device, and we can't change dev->irq after that point.
cardbus_config_irq_and_cls() has to do with the way CardBus interrupts
work, so it seems like it should be better integrated into the PCI
core. For example, maybe it could be integrated into the
PCI_HEADER_TYPE_CARDBUS case in pci_setup_device().
I think that would still require the powerpc IRQ fixups to be done
earlier, e.g., as a header quirk or a pcibios hook.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path
@ 2012-10-12 18:03 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2012-10-12 18:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Michal Simek, Hiroo Matsumoto, microblaze-uclinux,
Kenji Kaneshige, Jesse Larrew, jbarnes, Dominik Brodowski,
linux-pci, linuxppc-dev
On Mon, Jun 18, 2012 at 4:55 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2012-06-18 at 15:06 -0600, Bjorn Helgaas wrote:
>> We're moving the CardBus IRQ config from before pci_bus_add_devices()
>> to after. I see why you did that: we're proposing to do the powerpc
>> DMA & IRQ setup in pci_bus_add_devices(), so we don't want to have the
>> powerpc IRQ init clobber the CardBus IRQ config.
>>
>> But a driver can claim the device as soon as we call
>> pci_bus_add_devices(), so we're potentially changing dev->irq after a
>> driver has already looked at it, which sounds like a bug.
>>
>> There are only five possibilities for powerpc pci_irq_fixup:
>>
>> ppc47x_pci_irq_fixup
>> mpc85xx_cds_pci_irq_fixup
>> maple_pci_irq_fixup
>> pmac_pci_irq_fixup
>> rtas_msi_pci_irq_fixup
>>
>> If these were normal PCI header quirks instead, they could run
>> earlier, and we wouldn't need to move this
>> cardbus_config_irq_and_cls() call. Is it possible to make these
>> quirks, Ben?
>
> Wait ... why are those fixups relevant ? They have to run after
> pci_read_irq_line() (which should have been called pcibios_read_irq_line
> really) but that's fine, we call both back to back....
>
> The problem has to do with the fact that we setup pdev->irq inside
> pci_bus_add_devices() with the new proposed code (the fixup itself is
> just a detail).
>
> You want cardbus to "quirk" the irq after that's been fixed up... maybe
> that's a case for moving cardbus_config_irq_and_cls() to
> pci_enable_device() ? Or add another hook inside
> pci_bus_add_devices()...
This thread has languished a long time, and I think the original
problem (hot-added e1000e devices on powerpc don't work) still exists,
doesn't it?
I'd like to get a kernel bugzilla opened so (a) this doesn't get
forgotten and (b) we can attach things like dmesg logs showing the
issue.
The current patch does this:
> pci_bus_add_devices(bus);
> + cardbus_config_irq_and_cls(bus, s->pci_irq);
I don't think we can do this, because pci_bus_add_devices() can bind a
driver to the device, and we can't change dev->irq after that point.
cardbus_config_irq_and_cls() has to do with the way CardBus interrupts
work, so it seems like it should be better integrated into the PCI
core. For example, maybe it could be integrated into the
PCI_HEADER_TYPE_CARDBUS case in pci_setup_device().
I think that would still require the powerpc IRQ fixups to be done
earlier, e.g., as a header quirk or a pcibios hook.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-10-12 18:03 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-23 22:36 [PATCH v3 0/2] archdata init in device_add() notifier Bjorn Helgaas
2012-05-23 22:36 ` Bjorn Helgaas
2012-05-23 22:37 ` [PATCH v3 1/2] powerpc/PCI: move DMA & IRQ init to device_add() notification path Bjorn Helgaas
2012-05-23 22:37 ` Bjorn Helgaas
2012-05-25 3:00 ` Benjamin Herrenschmidt
2012-05-25 3:00 ` Benjamin Herrenschmidt
2012-05-25 3:08 ` Bjorn Helgaas
2012-05-25 3:08 ` Bjorn Helgaas
2012-05-25 3:38 ` Benjamin Herrenschmidt
2012-05-25 3:38 ` Benjamin Herrenschmidt
2012-06-07 12:58 ` Hiroo Matsumoto
2012-06-07 12:58 ` Hiroo Matsumoto
2012-06-18 21:06 ` Bjorn Helgaas
2012-06-18 21:06 ` Bjorn Helgaas
2012-06-18 22:55 ` Benjamin Herrenschmidt
2012-06-18 22:55 ` Benjamin Herrenschmidt
2012-10-12 18:03 ` Bjorn Helgaas
2012-10-12 18:03 ` Bjorn Helgaas
2012-05-23 22:37 ` [PATCH v3 2/2] microblaze/PCI: " Bjorn Helgaas
2012-05-23 22:37 ` Bjorn Helgaas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.