PCI scan interfaces clean up

PATCH In review Yijing Wang 4 participants 13 messages Nov 19, 2014 – Nov 20, 2014

Subsystems (14)

x86 PCI MIPS s390 +10 more subsystems

lore

Share & tools

lore

Yijing Wang

author

2014-11-19 06:52 UTC #0 lore raw

cover PCI scan interfaces clean up

Yijing Wang (5):
PCI: Rip out pci_bus_add_devices() from pci_scan_bus()
PCI: Introduce pci_scan_bus_simple()
PCI: Use pci_scan_bus_simple() in simple scene.
PCI: Remove pci_scan_bus() and pci_scan_bus_parented()
PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

 arch/alpha/kernel/pci.c           |    2 ++
 arch/alpha/kernel/sys_nautilus.c  |    3 ++-
 arch/frv/mb93090-mb00/pci-vdk.c   |    6 ++++--
 arch/ia64/sn/kernel/io_init.c     |    1 +
 arch/m68k/coldfire/pci.c          |    3 ++-
 arch/microblaze/pci/pci-common.c  |    1 +
 arch/mips/pci/pci.c               |    1 +
 arch/mn10300/unit-asb2305/pci.c   |    5 ++++-
 arch/s390/pci/pci.c               |    2 +-
 arch/sh/drivers/pci/pci.c         |    1 +
 arch/sparc/kernel/leon_pci.c      |    1 +
 arch/sparc/kernel/pcic.c          |    5 ++++-
 arch/tile/kernel/pci.c            |    2 ++
 arch/tile/kernel/pci_gx.c         |    2 ++
 arch/unicore32/kernel/pci.c       |   15 +++++----------
 arch/x86/pci/common.c             |    1 +
 arch/xtensa/kernel/pci.c          |    2 ++
 drivers/pci/host/pci-xgene.c      |    1 +
 drivers/pci/hotplug/ibmphp_core.c |    7 +++++--
 drivers/pci/probe.c               |   26 ++------------------------
 drivers/pci/xen-pcifront.c        |    2 +-
 include/linux/pci.h               |    3 +--
 22 files changed, 46 insertions(+), 46 deletions(-)

Yijing Wang

author

2014-11-19 06:52 UTC #1 lore raw

PATCH 1/5 PCI: Rip out pci_bus_add_devices() from pci_scan_bus()

Pci_bus_add_devices() should not be placed in pci_scan_bus(). Now pci devices will be added to driver core once its creation. All the things left in pci_bus_add_devices() are driver attach and other trivial things. pci_scan_bus() should be the function responsible for scanning PCI devices, not including device driver attached. Other, lots callers of pci_scan_bus() call pci_bus_size_bridges() and pci_bus_assign_resources() after pci_scan_bus(). It's not correct, but because pci_scan_bus() always be called before driver loaded, so maybe no problem will cause. We should rip out pci_bus_add_devices() for better code design.
No functional change.

Signed-off-by Yijing Wang <[email protected]>

CC "David S. Miller" <[email protected]>

CC Geert Uytterhoeven <[email protected]>

CC Guan Xuetao <[email protected]>

CC [email protected]

---
 arch/alpha/kernel/sys_nautilus.c  |    1 +
 arch/m68k/coldfire/pci.c          |    1 +
 arch/sparc/kernel/pcic.c          |    2 ++
 arch/unicore32/kernel/pci.c       |   11 +++--------
 drivers/pci/hotplug/ibmphp_core.c |    6 ++++--
 drivers/pci/probe.c               |    1 -
 6 files changed, 11 insertions(+), 11 deletions(-)

arch/alpha/kernel/sys_nautilus.c +1 -0

index 837c0fa..4ae4a40 100644
@@ -253,6 +253,7 @@ nautilus_init_pci(void)
 	   for the root bus, so just clear it. */
 	bus->self = NULL;
 	pci_fixup_irqs(alpha_mv.pci_swizzle, alpha_mv.pci_map_irq);
+	pci_bus_add_devices(bus);
 }
 
 /*

arch/m68k/coldfire/pci.c +1 -0

index df96792..d45f087 100644
@@ -319,6 +319,7 @@ static int __init mcf_pci_init(void)
 	pci_fixup_irqs(pci_common_swizzle, mcf_pci_map_irq);
 	pci_bus_size_bridges(rootbus);
 	pci_bus_assign_resources(rootbus);
+	pci_bus_add_devices(rootbus);
 	return 0;
 }

arch/sparc/kernel/pcic.c +2 -0

index 6cc78c2..7a82fe2 100644
@@ -391,6 +391,8 @@ static void __init pcic_pbm_scan_bus(struct linux_pcic *pcic)
 	struct linux_pbm_info *pbm = &pcic->pbm;
 
 	pbm->pci_bus = pci_scan_bus(pbm->pci_first_busno, &pcic_ops, pbm);
+	if (pbm->pci_bus)
+		pci_bus_add_devices(pbm->pci_bus);
 #if 0 /* deadwood transplanted from sparc64 */
 	pci_fill_in_pbm_cookies(pbm->pci_bus, pbm, pbm->prom_node);
 	pci_record_assignments(pbm, pbm->pci_bus);

arch/unicore32/kernel/pci.c +3 -8

index 374a055..3d82024 100644
@@ -266,17 +266,12 @@ static int __init pci_common_init(void)
 	pci_fixup_irqs(pci_common_swizzle, pci_puv3_map_irq);
 
 	if (!pci_has_flag(PCI_PROBE_ONLY)) {
-		/*
-		 * Size the bridge windows.
-		 */
+		/* Size the bridge windows. */
 		pci_bus_size_bridges(puv3_bus);
-
-		/*
-		 * Assign resources.
-		 */
+		/* Assign resources. */
 		pci_bus_assign_resources(puv3_bus);
 	}
-
+	pci_bus_add_devices(puv3_bus);
 	return 0;
 }
 subsys_initcall(pci_common_init);

drivers/pci/hotplug/ibmphp_core.c +4 -2

index 3efaf4c..db0f2e8 100644
@@ -740,7 +740,7 @@ static void ibm_unconfigure_device(struct pci_func *func)
  */
 static u8 bus_structure_fixup(u8 busno)
 {
-	struct pci_bus *bus;
+	struct pci_bus *bus, *b;
 	struct pci_dev *dev;
 	u16 l;

@@ -767,7 +767,9 @@ static u8 bus_structure_fixup(u8 busno)
 					(l != 0x0000) && (l != 0xffff)) {
 			debug("%s - Inside bus_structure_fixup()\n",
 							__func__);
-			pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL);
+			b = pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL);
+			if (b)
+				pci_bus_add_devices(b);
 			break;
 		}
 	}

drivers/pci/probe.c +0 -1

index db16678..5c5e01e 100644
@@ -2120,7 +2120,6 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
 	b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources);
 	if (b) {
 		pci_scan_child_bus(b);
-		pci_bus_add_devices(b);
 	} else {
 		pci_free_resource_list(&resources);
 	}

Signature · 1 line

1.7.1

Yijing Wang

author

2014-11-19 06:52 UTC #2 lore raw

PATCH 2/5 PCI: Introduce pci_scan_bus_simple()

In reply to Yijing Wang

Now pci_scan_bus() and pci_scan_bus_parented() are almost the same except parent device. There is no need to use two similar scan interfaces. Introduce pci_scan_bus_simple() which is same as pci_scan_bus_parented(). And remove pci_scan_bus() and pci_scan_bus_parented() in later patch.

Signed-off-by Yijing Wang <[email protected]>

---
 drivers/pci/probe.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

drivers/pci/probe.c +18 -0

index 5c5e01e..a8d90d4 100644
@@ -2089,6 +2089,24 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 }
 EXPORT_SYMBOL(pci_scan_root_bus);
 
+struct pci_bus *pci_scan_bus_simple(struct device *parent,
+		int bus, struct pci_ops *ops, void *sysdata)
+{
+	LIST_HEAD(resources);
+	struct pci_bus *b;
+
+	pci_add_resource(&resources, &ioport_resource);
+	pci_add_resource(&resources, &iomem_resource);
+	pci_add_resource(&resources, &busn_resource);
+	b = pci_create_root_bus(parent, bus, ops, sysdata, &resources);
+	if (b)
+		pci_scan_child_bus(b);
+	else
+		pci_free_resource_list(&resources);
+	return b;
+}
+EXPORT_SYMBOL(pci_scan_bus_simple);
+
 /* Deprecated; use pci_scan_root_bus() instead */
 struct pci_bus *pci_scan_bus_parented(struct device *parent,
 		int bus, struct pci_ops *ops, void *sysdata)

Signature · 1 line

1.7.1

Yijing Wang

author

2014-11-19 06:52 UTC #3 lore raw

PATCH 3/5 PCI: Use pci_scan_bus_simple() in simple scene.

In reply to Yijing Wang

Use pci_scan_bus_simple() to scan pci buses with
the default IO/MEM/BUS resources. Use it instead
of pci_scan_bus() and pci_scan_bus_parented().

Signed-off-by Yijing Wang <[email protected]>

---
 arch/alpha/kernel/sys_nautilus.c  |    2 +-
 arch/m68k/coldfire/pci.c          |    2 +-
 arch/sparc/kernel/pcic.c          |    3 ++-
 arch/unicore32/kernel/pci.c       |    4 ++--
 drivers/pci/hotplug/ibmphp_core.c |    3 ++-
 drivers/pci/xen-pcifront.c        |    2 +-
 include/linux/pci.h               |    2 ++
 7 files changed, 11 insertions(+), 7 deletions(-)

arch/alpha/kernel/sys_nautilus.c +1 -1

index 4ae4a40..a4f190b 100644
@@ -206,7 +206,7 @@ nautilus_init_pci(void)
 	unsigned long memtop = max_low_pfn << PAGE_SHIFT;
 
 	/* Scan our single hose.  */
-	bus = pci_scan_bus(0, alpha_mv.pci_ops, hose);
+	bus = pci_scan_bus_simple(NULL, 0, alpha_mv.pci_ops, hose);
 	hose->bus = bus;
 	pcibios_claim_one_bus(bus);

arch/m68k/coldfire/pci.c +1 -1

index d45f087..9f423d5 100644
@@ -312,7 +312,7 @@ static int __init mcf_pci_init(void)
 	set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(msecs_to_jiffies(200));
 
-	rootbus = pci_scan_bus(0, &mcf_pci_ops, NULL);
+	rootbus = pci_scan_bus_simple(NULL, 0, &mcf_pci_ops, NULL);
 	rootbus->resource[0] = &mcf_pci_io;
 	rootbus->resource[1] = &mcf_pci_mem;

arch/sparc/kernel/pcic.c +2 -1

index 7a82fe2..9665e03 100644
@@ -390,7 +390,8 @@ static void __init pcic_pbm_scan_bus(struct linux_pcic *pcic)
 {
 	struct linux_pbm_info *pbm = &pcic->pbm;
 
-	pbm->pci_bus = pci_scan_bus(pbm->pci_first_busno, &pcic_ops, pbm);
+	pbm->pci_bus = pci_scan_bus_simple(NULL,
+			pbm->pci_first_busno, &pcic_ops, pbm);
 	if (pbm->pci_bus)
 		pci_bus_add_devices(pbm->pci_bus);
 #if 0 /* deadwood transplanted from sparc64 */

arch/unicore32/kernel/pci.c +2 -2

index 3d82024..f290a3d 100644
@@ -258,8 +258,8 @@ static int __init pci_common_init(void)
 
 	pci_puv3_preinit();
 
-	puv3_bus = pci_scan_bus(0, &pci_puv3_ops, NULL);
-
+	puv3_bus = pci_scan_bus_simple(NULL, 0,
+			&pci_puv3_ops, NULL);
 	if (!puv3_bus)
 		panic("PCI: unable to scan bus!");

drivers/pci/hotplug/ibmphp_core.c +2 -1

index db0f2e8..9190cb1 100644
@@ -767,7 +767,8 @@ static u8 bus_structure_fixup(u8 busno)
 					(l != 0x0000) && (l != 0xffff)) {
 			debug("%s - Inside bus_structure_fixup()\n",
 							__func__);
-			b = pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL);
+			b = pci_scan_bus_simple(NULL, busno,
+					ibmphp_pci_bus->ops, NULL);
 			if (b)
 				pci_bus_add_devices(b);
 			break;

drivers/pci/xen-pcifront.c +1 -1

index b1ffebe..364aaf4 100644
@@ -474,7 +474,7 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 
 	pci_lock_rescan_remove();
 
-	b = pci_scan_bus_parented(&pdev->xdev->dev, bus,
+	b = pci_scan_bus_simple(&pdev->xdev->dev, bus,
 				  &pcifront_bus_ops, sd);
 	if (!b) {
 		dev_err(&pdev->xdev->dev,

include/linux/pci.h +2 -0

index 90a5fde..befefbd 100644
@@ -769,6 +769,8 @@ struct pci_bus *pci_find_bus(int domain, int busnr);
 void pci_bus_add_devices(const struct pci_bus *bus);
 struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus,
 				      struct pci_ops *ops, void *sysdata);
+struct pci_bus *pci_scan_bus_simple(struct device *parent, int bus,
+				      struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,

Signature · 1 line

1.7.1

Arnd Bergmann

2014-11-19 12:28 UTC #4 lore raw

PATCH 3/5 PCI: Use pci_scan_bus_simple() in simple scene.

Quoted 6 lines · On Wednesday 19 November 2014 15:32:47 Yijing Wang wrote:

Use pci_scan_bus_simple() to scan pci buses with
the default IO/MEM/BUS resources. Use it instead
of pci_scan_bus() and pci_scan_bus_parented().
Signed-off-by Yijing Wang <[email protected]>

I think we should replace pci_scan_bus_parented() with something better by making the xen code use pci_scan_root_bus() (see below for a draft patch). After that we no longer need the 'parent' argument here and we can just keep using the pci_scan_bus() function for the legacy users, or rename it if that helps.

Arnd

8<---

drivers/pci/xen-pcifront.c +6 -2

index 116ca3746adb..45a4843a608a 100644
@@ -448,6 +448,7 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 	struct pci_bus *b;
 	struct pcifront_sd *sd = NULL;
 	struct pci_bus_entry *bus_entry = NULL;
+	LIST_HEAD(resources);
 	int err = 0;
 
 #ifndef CONFIG_PCI_DOMAINS

@@ -470,17 +471,20 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 		err = -ENOMEM;
 		goto err_out;
 	}
+	pci_add_resource(&resources, &ioport_resource);
+	pci_add_resource(&resources, &iomem_resource);
 	pcifront_init_sd(sd, domain, bus, pdev);
 
 	pci_lock_rescan_remove();
 
-	b = pci_scan_bus_parented(&pdev->xdev->dev, bus,
-				  &pcifront_bus_ops, sd);
+	b = pci_scan_root_bus(&pdev->xdev->dev, bus, &pcifront_bus_ops, sd,
+			      &resources);
 	if (!b) {
 		dev_err(&pdev->xdev->dev,
 			"Error creating PCI Frontend Bus!\n");
 		err = -ENOMEM;
 		pci_unlock_rescan_remove();
+		pci_free_resource_list(&resources);
 		goto err_out;
 	}

Yijing Wang

author

2014-11-20 01:53 UTC #5 lore raw

PATCH 3/5 PCI: Use pci_scan_bus_simple() in simple scene.

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · On Wednesday 19 November 2014 15:32:47 Yijing Wang wrote:
Use pci_scan_bus_simple() to scan pci buses with
the default IO/MEM/BUS resources. Use it instead
of pci_scan_bus() and pci_scan_bus_parented().

Quoted 8 lines

Quoted 2 lines
Signed-off-by Yijing Wang <[email protected]>
I think we should replace pci_scan_bus_parented() with something better by making the xen code use pci_scan_root_bus() (see below for a draft patch). After that we no longer need the 'parent' argument here and we can just keep using the pci_scan_bus() function for the legacy users, or rename it if that helps.

It make sense to me, and we could rename pci_scan_bus() to pci_scan_bus_legacy(). Thanks, I will add this to my series,

Quoted 45 lines

Arnd

8<---

drivers/pci/xen-pcifront.c +6 -2

index 116ca3746adb..45a4843a608a 100644
@@ -448,6 +448,7 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 	struct pci_bus *b;
 	struct pcifront_sd *sd = NULL;
 	struct pci_bus_entry *bus_entry = NULL;
+	LIST_HEAD(resources);
 	int err = 0;
 
 #ifndef CONFIG_PCI_DOMAINS

@@ -470,17 +471,20 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 		err = -ENOMEM;
 		goto err_out;
 	}
+	pci_add_resource(&resources, &ioport_resource);
+	pci_add_resource(&resources, &iomem_resource);
 	pcifront_init_sd(sd, domain, bus, pdev);
 
 	pci_lock_rescan_remove();
 
-	b = pci_scan_bus_parented(&pdev->xdev->dev, bus,
-				  &pcifront_bus_ops, sd);
+	b = pci_scan_root_bus(&pdev->xdev->dev, bus, &pcifront_bus_ops, sd,
+			      &resources);
 	if (!b) {
 		dev_err(&pdev->xdev->dev,
 			"Error creating PCI Frontend Bus!\n");
 		err = -ENOMEM;
 		pci_unlock_rescan_remove();
+		pci_free_resource_list(&resources);
 		goto err_out;
 	}

Signature · 2 lines

Thanks!
Yijing

Yijing Wang

author

2014-11-19 06:52 UTC #6 lore raw

PATCH 4/5 PCI: Remove pci_scan_bus() and pci_scan_bus_parented()

In reply to Yijing Wang

Clean out pci_scan_bus() and pci_scan_bus_parented().

Signed-off-by Yijing Wang <[email protected]>

---
 drivers/pci/probe.c |   38 --------------------------------------
 include/linux/pci.h |    3 ---
 2 files changed, 0 insertions(+), 41 deletions(-)

drivers/pci/probe.c +0 ~~-38~~

index a8d90d4..79775b5 100644
@@ -2107,44 +2107,6 @@ struct pci_bus *pci_scan_bus_simple(struct device *parent,
 }
 EXPORT_SYMBOL(pci_scan_bus_simple);
 
-/* Deprecated; use pci_scan_root_bus() instead */
-struct pci_bus *pci_scan_bus_parented(struct device *parent,
-		int bus, struct pci_ops *ops, void *sysdata)
-{
-	LIST_HEAD(resources);
-	struct pci_bus *b;
-
-	pci_add_resource(&resources, &ioport_resource);
-	pci_add_resource(&resources, &iomem_resource);
-	pci_add_resource(&resources, &busn_resource);
-	b = pci_create_root_bus(parent, bus, ops, sysdata, &resources);
-	if (b)
-		pci_scan_child_bus(b);
-	else
-		pci_free_resource_list(&resources);
-	return b;
-}
-EXPORT_SYMBOL(pci_scan_bus_parented);
-
-struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
-					void *sysdata)
-{
-	LIST_HEAD(resources);
-	struct pci_bus *b;
-
-	pci_add_resource(&resources, &ioport_resource);
-	pci_add_resource(&resources, &iomem_resource);
-	pci_add_resource(&resources, &busn_resource);
-	b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources);
-	if (b) {
-		pci_scan_child_bus(b);
-	} else {
-		pci_free_resource_list(&resources);
-	}
-	return b;
-}
-EXPORT_SYMBOL(pci_scan_bus);
-
 /**
  * pci_rescan_bus_bridge_resize - scan a PCI bus for devices.
  * @bridge: PCI bridge for the bus to scan

include/linux/pci.h +0 -3

index befefbd..9ebc908 100644
@@ -767,11 +767,8 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 void pcibios_scan_specific_bus(int busn);
 struct pci_bus *pci_find_bus(int domain, int busnr);
 void pci_bus_add_devices(const struct pci_bus *bus);
-struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus,
-				      struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_scan_bus_simple(struct device *parent, int bus,
 				      struct pci_ops *ops, void *sysdata);
-struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);

Signature · 1 line

1.7.1

Yijing Wang

author

2014-11-19 06:52 UTC #7 lore raw

PATCH 5/5 PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

In reply to Yijing Wang

Just like pci_scan_bus(), we also should rip out pci_bus_add_devices() from pci_scan_root_bus(). Lots platforms first call pci_scan_root_bus(), but after that, they call pci_bus_size_bridges() and pci_bus_assign_resources(). Place pci_bus_add_devices() in pci_scan_root_bus() hurts PCI scan logic. Should no functional change.

Signed-off-by Yijing Wang <[email protected]>

CC [email protected]

---
 arch/alpha/kernel/pci.c          |    2 ++
 arch/frv/mb93090-mb00/pci-vdk.c  |    6 ++++--
 arch/ia64/sn/kernel/io_init.c    |    1 +
 arch/microblaze/pci/pci-common.c |    1 +
 arch/mips/pci/pci.c              |    1 +
 arch/mn10300/unit-asb2305/pci.c  |    5 ++++-
 arch/s390/pci/pci.c              |    2 +-
 arch/sh/drivers/pci/pci.c        |    1 +
 arch/sparc/kernel/leon_pci.c     |    1 +
 arch/tile/kernel/pci.c           |    2 ++
 arch/tile/kernel/pci_gx.c        |    2 ++
 arch/x86/pci/common.c            |    1 +
 arch/xtensa/kernel/pci.c         |    2 ++
 drivers/pci/host/pci-xgene.c     |    1 +
 drivers/pci/probe.c              |    1 -
 15 files changed, 24 insertions(+), 5 deletions(-)

arch/alpha/kernel/pci.c +2 -0

index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;

arch/frv/mb93090-mb00/pci-vdk.c +4 -2

index efa5d65..2b36044 100644
@@ -316,6 +316,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 
 int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	struct pci_ops *dir = NULL;
 	LIST_HEAD(resources);

@@ -383,12 +384,13 @@ int __init pcibios_init(void)
 	printk("PCI: Probing PCI hardware\n");
 	pci_add_resource(&resources, &pci_ioport_resource);
 	pci_add_resource(&resources, &pci_iomem_resource);
-	pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
-
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/ia64/sn/kernel/io_init.c +1 -0

index 0b5ce82..63b43a6 100644
@@ -272,6 +272,7 @@ sn_pci_controller_fixup(int segment, int busnum, struct pci_bus *bus)
 		kfree(res);
 		kfree(controller);
 	}
+	pci_bus_add_devices(bus);
 }
 
 /*

arch/microblaze/pci/pci-common.c +1 -0

index 9037914..651e25d 100644
@@ -1346,6 +1346,7 @@ static void pcibios_scan_phb(struct pci_controller *hose)
 	hose->bus = bus;
 
 	hose->last_busno = bus->busn_res.end;
+	pci_bus_add_devices(bus);
 }
 
 static int __init pcibios_init(void)

arch/mips/pci/pci.c +1 -0

index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }

arch/mn10300/unit-asb2305/pci.c +4 -1

index 6b4339f..860aa35 100644
@@ -345,6 +345,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
  */
 static int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	resource_size_t io_offset, mem_offset;
 	LIST_HEAD(resources);

@@ -376,11 +377,13 @@ static int __init pcibios_init(void)
 
 	pci_add_resource_offset(&resources, &pci_ioport_resource, io_offset);
 	pci_add_resource_offset(&resources, &pci_iomem_resource, mem_offset);
-	pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/s390/pci/pci.c +1 -1

index 552b990..98247f8 100644
@@ -755,7 +755,7 @@ static int zpci_scan_bus(struct zpci_dev *zdev)
 		zpci_cleanup_bus_resources(zdev);
 		return -EIO;
 	}
-
+	pci_bus_add_devices(zdev->bus);
 	zdev->bus->max_bus_speed = zdev->max_bus_speed;
 	return 0;
 }

arch/sh/drivers/pci/pci.c +1 -0

index 1bc09ee..efc1051 100644
@@ -69,6 +69,7 @@ static void pcibios_scanbus(struct pci_channel *hose)
 
 		pci_bus_size_bridges(bus);
 		pci_bus_assign_resources(bus);
+		pci_bus_add_devices(bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/sparc/kernel/leon_pci.c +1 -0

index 899b720..2971076 100644
@@ -40,6 +40,7 @@ void leon_pci_init(struct platform_device *ofdev, struct leon_pci_info *info)
 
 		/* Assign devices with resources */
 		pci_assign_unassigned_resources();
+		pci_bus_add_devices(root_bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/tile/kernel/pci.c +2 -0

index 1f80a88..007466e 100644
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}

arch/tile/kernel/pci_gx.c +2 -0

index e39f9c5..3f8fb2c 100644
@@ -889,6 +889,8 @@ int __init pcibios_init(void)
 		controller->first_busno = next_busno;
 		bus = pci_scan_root_bus(NULL, next_busno, controller->ops,
 					controller, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		controller->root_bus = bus;
 		next_busno = bus->busn_res.end + 1;
 	}

arch/x86/pci/common.c +1 -0

index 7b20bcc..300d39e 100644
@@ -475,6 +475,7 @@ void pcibios_scan_root(int busnum)
 		pci_free_resource_list(&resources);
 		kfree(sd);
 	}
+	pci_bus_add_devices(bus);
 }
 
 void __init pcibios_set_cache_line_size(void)

arch/xtensa/kernel/pci.c +2 -0

index 5b34033..f2ae64e 100644
@@ -185,6 +185,8 @@ static int __init pcibios_init(void)
 		pci_controller_apertures(pci_ctrl, &resources);
 		bus = pci_scan_root_bus(NULL, pci_ctrl->first_busno,
 					pci_ctrl->ops, pci_ctrl, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		pci_ctrl->bus = bus;
 		pci_ctrl->last_busno = bus->busn_res.end;
 		if (next_busno <= pci_ctrl->last_busno)

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

index 9ecabfa..c17afe5 100644
@@ -635,6 +635,7 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	if (!bus)
 		return -ENOMEM;
 
+	pci_bus_add_devices(bus);
 	platform_set_drvdata(pdev, port);
 	return 0;
 }

drivers/pci/probe.c +0 -1

index 79775b5..2ab5a40 100644
@@ -2084,7 +2084,6 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	if (!found)
 		pci_bus_update_busn_res_end(b, max);
 
-	pci_bus_add_devices(b);
 	return b;
 }
 EXPORT_SYMBOL(pci_scan_root_bus);

Signature · 1 line

1.7.1

Lorenzo Pieralisi

2014-11-19 11:49 UTC #8 lore raw

Re: [PATCH 5/5] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

Quoted 7 lines · Yijing Wang

Just like pci_scan_bus(), we also should rip out pci_bus_add_devices() from pci_scan_root_bus(). Lots platforms first call pci_scan_root_bus(), but after that, they call pci_bus_size_bridges() and pci_bus_assign_resources(). Place pci_bus_add_devices() in pci_scan_root_bus() hurts PCI scan logic. Should no functional change.

I agree with the patch, I do not agree it provides no functional change. You are actually fixing an issue on systems where the root bus is scanned at device_initcall time and pci drivers may have already been registered, so they end up being probed with possibly unassigned resources as you correctly mention in the log.

You could move the sequence:

if (!pci_has_flag(PCI_PROBE_ONLY)) {
	pci_bus_size_bridges();
	pci_bus_assign_resources();
}

in pci_scan_root_bus() instead of taking pci_bus_add_devices() out ?

Would this break any code ? Or add a helper function:

pci_scan_root_bus(..., bool assign) ?

arm64 xgene code has already been fixed by reshuffling the scanning code sequence.

I think we should have a look at eg drivers/pci/host/pcie-xilinx.c, I think there is a possible initcall ordering issue there (drivers being probed with unassigned resources).

Lorenzo

Quoted 251 lines

Signed-off-by Yijing Wang <[email protected]>

CC [email protected]

---
 arch/alpha/kernel/pci.c          |    2 ++
 arch/frv/mb93090-mb00/pci-vdk.c  |    6 ++++--
 arch/ia64/sn/kernel/io_init.c    |    1 +
 arch/microblaze/pci/pci-common.c |    1 +
 arch/mips/pci/pci.c              |    1 +
 arch/mn10300/unit-asb2305/pci.c  |    5 ++++-
 arch/s390/pci/pci.c              |    2 +-
 arch/sh/drivers/pci/pci.c        |    1 +
 arch/sparc/kernel/leon_pci.c     |    1 +
 arch/tile/kernel/pci.c           |    2 ++
 arch/tile/kernel/pci_gx.c        |    2 ++
 arch/x86/pci/common.c            |    1 +
 arch/xtensa/kernel/pci.c         |    2 ++
 drivers/pci/host/pci-xgene.c     |    1 +
 drivers/pci/probe.c              |    1 -
 15 files changed, 24 insertions(+), 5 deletions(-)

arch/alpha/kernel/pci.c +2 -0

index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;

arch/frv/mb93090-mb00/pci-vdk.c +4 -2

index efa5d65..2b36044 100644
@@ -316,6 +316,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 
 int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	struct pci_ops *dir = NULL;
 	LIST_HEAD(resources);

@@ -383,12 +384,13 @@ int __init pcibios_init(void)
 	printk("PCI: Probing PCI hardware\n");
 	pci_add_resource(&resources, &pci_ioport_resource);
 	pci_add_resource(&resources, &pci_iomem_resource);
-	pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
-
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/ia64/sn/kernel/io_init.c +1 -0

index 0b5ce82..63b43a6 100644
@@ -272,6 +272,7 @@ sn_pci_controller_fixup(int segment, int busnum, struct pci_bus *bus)
 		kfree(res);
 		kfree(controller);
 	}
+	pci_bus_add_devices(bus);
 }
 
 /*

arch/microblaze/pci/pci-common.c +1 -0

index 9037914..651e25d 100644
@@ -1346,6 +1346,7 @@ static void pcibios_scan_phb(struct pci_controller *hose)
 	hose->bus = bus;
 
 	hose->last_busno = bus->busn_res.end;
+	pci_bus_add_devices(bus);
 }
 
 static int __init pcibios_init(void)

arch/mips/pci/pci.c +1 -0

index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }

arch/mn10300/unit-asb2305/pci.c +4 -1

index 6b4339f..860aa35 100644
@@ -345,6 +345,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
  */
 static int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	resource_size_t io_offset, mem_offset;
 	LIST_HEAD(resources);

@@ -376,11 +377,13 @@ static int __init pcibios_init(void)
 
 	pci_add_resource_offset(&resources, &pci_ioport_resource, io_offset);
 	pci_add_resource_offset(&resources, &pci_iomem_resource, mem_offset);
-	pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/s390/pci/pci.c +1 -1

index 552b990..98247f8 100644
@@ -755,7 +755,7 @@ static int zpci_scan_bus(struct zpci_dev *zdev)
 		zpci_cleanup_bus_resources(zdev);
 		return -EIO;
 	}
-
+	pci_bus_add_devices(zdev->bus);
 	zdev->bus->max_bus_speed = zdev->max_bus_speed;
 	return 0;
 }

arch/sh/drivers/pci/pci.c +1 -0

index 1bc09ee..efc1051 100644
@@ -69,6 +69,7 @@ static void pcibios_scanbus(struct pci_channel *hose)
 
 		pci_bus_size_bridges(bus);
 		pci_bus_assign_resources(bus);
+		pci_bus_add_devices(bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/sparc/kernel/leon_pci.c +1 -0

index 899b720..2971076 100644
@@ -40,6 +40,7 @@ void leon_pci_init(struct platform_device *ofdev, struct leon_pci_info *info)
 
 		/* Assign devices with resources */
 		pci_assign_unassigned_resources();
+		pci_bus_add_devices(root_bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/tile/kernel/pci.c +2 -0

index 1f80a88..007466e 100644
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}

arch/tile/kernel/pci_gx.c +2 -0

index e39f9c5..3f8fb2c 100644
@@ -889,6 +889,8 @@ int __init pcibios_init(void)
 		controller->first_busno = next_busno;
 		bus = pci_scan_root_bus(NULL, next_busno, controller->ops,
 					controller, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		controller->root_bus = bus;
 		next_busno = bus->busn_res.end + 1;
 	}

arch/x86/pci/common.c +1 -0

index 7b20bcc..300d39e 100644
@@ -475,6 +475,7 @@ void pcibios_scan_root(int busnum)
 		pci_free_resource_list(&resources);
 		kfree(sd);
 	}
+	pci_bus_add_devices(bus);
 }
 
 void __init pcibios_set_cache_line_size(void)

arch/xtensa/kernel/pci.c +2 -0

index 5b34033..f2ae64e 100644
@@ -185,6 +185,8 @@ static int __init pcibios_init(void)
 		pci_controller_apertures(pci_ctrl, &resources);
 		bus = pci_scan_root_bus(NULL, pci_ctrl->first_busno,
 					pci_ctrl->ops, pci_ctrl, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		pci_ctrl->bus = bus;
 		pci_ctrl->last_busno = bus->busn_res.end;
 		if (next_busno <= pci_ctrl->last_busno)

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

index 9ecabfa..c17afe5 100644
@@ -635,6 +635,7 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	if (!bus)
 		return -ENOMEM;
 
+	pci_bus_add_devices(bus);
 	platform_set_drvdata(pdev, port);
 	return 0;
 }

drivers/pci/probe.c +0 -1

index 79775b5..2ab5a40 100644
@@ -2084,7 +2084,6 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	if (!found)
 		pci_bus_update_busn_res_end(b, max);
 
-	pci_bus_add_devices(b);
 	return b;
 }
 EXPORT_SYMBOL(pci_scan_root_bus);

Signature · 6 lines

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yijing Wang

author

2014-11-20 01:14 UTC #9 lore raw

Re: [PATCH 5/5] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

Quoted 15 lines · Lorenzo Pieralisi

Quoted 7 lines · Yijing Wang
Just like pci_scan_bus(), we also should rip out pci_bus_add_devices() from pci_scan_root_bus(). Lots platforms first call pci_scan_root_bus(), but after that, they call pci_bus_size_bridges() and pci_bus_assign_resources(). Place pci_bus_add_devices() in pci_scan_root_bus() hurts PCI scan logic. Should no functional change.
I agree with the patch, I do not agree it provides no functional change. You are actually fixing an issue on systems where the root bus is scanned at device_initcall time and pci drivers may have already been registered, so they end up being probed with possibly unassigned resources as you correctly mention in the log.

Quoted 8 lines

You could move the sequence:
if (!pci_has_flag(PCI_PROBE_ONLY)) {
	pci_bus_size_bridges();
	pci_bus_assign_resources();
}
in pci_scan_root_bus() instead of taking pci_bus_add_devices() out ?

No, many drivers need additional setup before resources setup.

Quoted 4 lines

Would this break any code ? Or add a helper function:
pci_scan_root_bus(..., bool assign) ?

I don't think it's worth, this make scan function involved something not scan business.

Quoted 7 lines

arm64 xgene code has already been fixed by reshuffling the scanning code sequence.
I think we should have a look at eg drivers/pci/host/pcie-xilinx.c, I think there is a possible initcall ordering issue there (drivers being probed with unassigned resources).

If pci device drivers(like network driver) are loaded before pcie host driver, use pci_scan_root_bus() may cause problem, because pci_bus_add_devices() will try to attach the device driver, but this time pci devices resources may not be configured properly.

Quoted 257 lines

Lorenzo

Quoted 251 lines

Signed-off-by Yijing Wang <[email protected]>

CC [email protected]

---
 arch/alpha/kernel/pci.c          |    2 ++
 arch/frv/mb93090-mb00/pci-vdk.c  |    6 ++++--
 arch/ia64/sn/kernel/io_init.c    |    1 +
 arch/microblaze/pci/pci-common.c |    1 +
 arch/mips/pci/pci.c              |    1 +
 arch/mn10300/unit-asb2305/pci.c  |    5 ++++-
 arch/s390/pci/pci.c              |    2 +-
 arch/sh/drivers/pci/pci.c        |    1 +
 arch/sparc/kernel/leon_pci.c     |    1 +
 arch/tile/kernel/pci.c           |    2 ++
 arch/tile/kernel/pci_gx.c        |    2 ++
 arch/x86/pci/common.c            |    1 +
 arch/xtensa/kernel/pci.c         |    2 ++
 drivers/pci/host/pci-xgene.c     |    1 +
 drivers/pci/probe.c              |    1 -
 15 files changed, 24 insertions(+), 5 deletions(-)

arch/alpha/kernel/pci.c +2 -0

index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;

arch/frv/mb93090-mb00/pci-vdk.c +4 -2

index efa5d65..2b36044 100644
@@ -316,6 +316,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
 
 int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	struct pci_ops *dir = NULL;
 	LIST_HEAD(resources);

@@ -383,12 +384,13 @@ int __init pcibios_init(void)
 	printk("PCI: Probing PCI hardware\n");
 	pci_add_resource(&resources, &pci_ioport_resource);
 	pci_add_resource(&resources, &pci_iomem_resource);
-	pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, pci_root_ops, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
-
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/ia64/sn/kernel/io_init.c +1 -0

index 0b5ce82..63b43a6 100644
@@ -272,6 +272,7 @@ sn_pci_controller_fixup(int segment, int busnum, struct pci_bus *bus)
 		kfree(res);
 		kfree(controller);
 	}
+	pci_bus_add_devices(bus);
 }
 
 /*

arch/microblaze/pci/pci-common.c +1 -0

index 9037914..651e25d 100644
@@ -1346,6 +1346,7 @@ static void pcibios_scan_phb(struct pci_controller *hose)
 	hose->bus = bus;
 
 	hose->last_busno = bus->busn_res.end;
+	pci_bus_add_devices(bus);
 }
 
 static int __init pcibios_init(void)

arch/mips/pci/pci.c +1 -0

index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }

arch/mn10300/unit-asb2305/pci.c +4 -1

index 6b4339f..860aa35 100644
@@ -345,6 +345,7 @@ void pcibios_fixup_bus(struct pci_bus *bus)
  */
 static int __init pcibios_init(void)
 {
+	struct pci_bus *bus;
 	resource_size_t io_offset, mem_offset;
 	LIST_HEAD(resources);

@@ -376,11 +377,13 @@ static int __init pcibios_init(void)
 
 	pci_add_resource_offset(&resources, &pci_ioport_resource, io_offset);
 	pci_add_resource_offset(&resources, &pci_iomem_resource, mem_offset);
-	pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
+	bus = pci_scan_root_bus(NULL, 0, &pci_direct_ampci, NULL, &resources);
 
 	pcibios_irq_init();
 	pcibios_fixup_irqs();
 	pcibios_resource_survey();
+	if (bus)
+		pci_bus_add_devices(bus);
 	return 0;
 }

arch/s390/pci/pci.c +1 -1

index 552b990..98247f8 100644
@@ -755,7 +755,7 @@ static int zpci_scan_bus(struct zpci_dev *zdev)
 		zpci_cleanup_bus_resources(zdev);
 		return -EIO;
 	}
-
+	pci_bus_add_devices(zdev->bus);
 	zdev->bus->max_bus_speed = zdev->max_bus_speed;
 	return 0;
 }

arch/sh/drivers/pci/pci.c +1 -0

index 1bc09ee..efc1051 100644
@@ -69,6 +69,7 @@ static void pcibios_scanbus(struct pci_channel *hose)
 
 		pci_bus_size_bridges(bus);
 		pci_bus_assign_resources(bus);
+		pci_bus_add_devices(bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/sparc/kernel/leon_pci.c +1 -0

index 899b720..2971076 100644
@@ -40,6 +40,7 @@ void leon_pci_init(struct platform_device *ofdev, struct leon_pci_info *info)
 
 		/* Assign devices with resources */
 		pci_assign_unassigned_resources();
+		pci_bus_add_devices(root_bus);
 	} else {
 		pci_free_resource_list(&resources);
 	}

arch/tile/kernel/pci.c +2 -0

index 1f80a88..007466e 100644
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}

arch/tile/kernel/pci_gx.c +2 -0

index e39f9c5..3f8fb2c 100644
@@ -889,6 +889,8 @@ int __init pcibios_init(void)
 		controller->first_busno = next_busno;
 		bus = pci_scan_root_bus(NULL, next_busno, controller->ops,
 					controller, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		controller->root_bus = bus;
 		next_busno = bus->busn_res.end + 1;
 	}

arch/x86/pci/common.c +1 -0

index 7b20bcc..300d39e 100644
@@ -475,6 +475,7 @@ void pcibios_scan_root(int busnum)
 		pci_free_resource_list(&resources);
 		kfree(sd);
 	}
+	pci_bus_add_devices(bus);
 }
 
 void __init pcibios_set_cache_line_size(void)

arch/xtensa/kernel/pci.c +2 -0

index 5b34033..f2ae64e 100644
@@ -185,6 +185,8 @@ static int __init pcibios_init(void)
 		pci_controller_apertures(pci_ctrl, &resources);
 		bus = pci_scan_root_bus(NULL, pci_ctrl->first_busno,
 					pci_ctrl->ops, pci_ctrl, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		pci_ctrl->bus = bus;
 		pci_ctrl->last_busno = bus->busn_res.end;
 		if (next_busno <= pci_ctrl->last_busno)

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

index 9ecabfa..c17afe5 100644
@@ -635,6 +635,7 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 	if (!bus)
 		return -ENOMEM;
 
+	pci_bus_add_devices(bus);
 	platform_set_drvdata(pdev, port);
 	return 0;
 }

drivers/pci/probe.c +0 -1

index 79775b5..2ab5a40 100644
@@ -2084,7 +2084,6 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
 	if (!found)
 		pci_bus_update_busn_res_end(b, max);
 
-	pci_bus_add_devices(b);
 	return b;
 }
 EXPORT_SYMBOL(pci_scan_root_bus);

Signature · 6 lines

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Thanks!
Yijing

Arnd Bergmann

2014-11-19 12:11 UTC #10 lore raw

Re: [PATCH 5/5] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

In reply to Yijing Wang

Quoted 6 lines · On Wednesday 19 November 2014 15:32:49 Yijing Wang wrote:

Just like pci_scan_bus(), we also should rip out pci_bus_add_devices() from pci_scan_root_bus(). Lots platforms first call pci_scan_root_bus(), but after that, they call pci_bus_size_bridges() and pci_bus_assign_resources(). Place pci_bus_add_devices() in pci_scan_root_bus() hurts PCI scan logic.

I think we really need to wait for Bjorn to comment on this patch, as I mentioned the idea behind pci_scan_root_bus() was to integrate the scanning part, which you now undo, though I can still see this working out in the long run.

Quoted 1 line

Should no functional change.

But you are moving the pci_bus_add_devices() later in a couple of places. While this seems entirely reasonable, I would consider it a functional change.

arch/alpha/kernel/pci.c common_init_pci(void) · line 341

Quoted 13 lines

arch/alpha/kernel/pci.c +2 -0

index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;

How about making pci_bus_add_devices() handle a NULL argument to save the if() here and elsewhere?

arch/mips/pci/pci.c static void pcibios_scanbus(struct pci_controller *hose) · line 119

Quoted 12 lines

arch/mips/pci/pci.c +1 -0

index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }

This one looks wrong, I think you still want to call pci_bus_add_devices() even with PCI_PROBE_ONLY set.

arch/tile/kernel/pci.c int __init pcibios_init(void) · line 315

Quoted 13 lines

arch/tile/kernel/pci.c +2 -0

index 1f80a88..007466e 100644
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}

Should the pci_bus_add_devices come after setting the bus numbers here?

Arnd

Liviu Dudau

2014-11-19 19:03 UTC #11 lore raw

Re: [PATCH 5/5] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

Quoted 25 lines · Arnd Bergmann

Quoted 6 lines · On Wednesday 19 November 2014 15:32:49 Yijing Wang wrote:
Just like pci_scan_bus(), we also should rip out pci_bus_add_devices() from pci_scan_root_bus(). Lots platforms first call pci_scan_root_bus(), but after that, they call pci_bus_size_bridges() and pci_bus_assign_resources(). Place pci_bus_add_devices() in pci_scan_root_bus() hurts PCI scan logic.
I think we really need to wait for Bjorn to comment on this patch, as I mentioned the idea behind pci_scan_root_bus() was to integrate the scanning part, which you now undo, though I can still see this working out in the long run.
Quoted 1 line
Should no functional change.
But you are moving the pci_bus_add_devices() later in a couple of places. While this seems entirely reasonable, I would consider it a functional change.
Quoted 6 lines
arch/alpha/kernel/pci.c +0 -0
index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 

Quoted 10 lines

arch/alpha/kernel/pci.c common_init_pci(void) · line 341
Quoted 7 lines
arch/alpha/kernel/pci.c +2 -0
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;
How about making pci_bus_add_devices() handle a NULL argument to save the if() here and elsewhere?

Actually, I would stop there if the bus is NULL and start cleaning up. If not, things are going to blow up three lines down when next_busno is calculated.

Best regards,
Liviu

Quoted 34 lines

arch/mips/pci/pci.c static void pcibios_scanbus(struct pci_controller *hose) · line 119
Quoted 12 lines
arch/mips/pci/pci.c +1 -0
index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }
This one looks wrong, I think you still want to call pci_bus_add_devices() even with PCI_PROBE_ONLY set.
arch/tile/kernel/pci.c int __init pcibios_init(void) · line 315
Quoted 13 lines
arch/tile/kernel/pci.c +2 -0
index 1f80a88..007466e 100644
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}
Should the pci_bus_add_devices come after setting the bus numbers here?
Arnd

Signature · 10 lines

-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Yijing Wang

author

2014-11-20 01:49 UTC #12 lore raw

Re: [PATCH 5/5] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()

In reply to Arnd Bergmann

Quoted 6 lines

Quoted 6 lines
arch/alpha/kernel/pci.c +0 -0
index 076c35c..97f9730 100644
@@ -334,6 +334,8 @@ common_init_pci(void)
 

Quoted 10 lines

arch/alpha/kernel/pci.c common_init_pci(void) · line 341
Quoted 7 lines
arch/alpha/kernel/pci.c +2 -0
 		bus = pci_scan_root_bus(NULL, next_busno, alpha_mv.pci_ops,
 					hose, &resources);
+		if (bus)
+			pci_bus_add_devices(bus);
 		hose->bus = bus;
 		hose->need_domain_info = need_domain_info;
 		next_busno = bus->busn_res.end + 1;
How about making pci_bus_add_devices() handle a NULL argument to save the if() here and elsewhere?

This make sense, will update it, thanks!

Quoted 7 lines

arch/mips/pci/pci.c +0 -0

index 1bf60b1..f083688 100644
@@ -113,6 +113,7 @@ static void pcibios_scanbus(struct pci_controller *hose)
 		if (!pci_has_flag(PCI_PROBE_ONLY)) {
 			pci_bus_size_bridges(bus);

Quoted 8 lines

arch/mips/pci/pci.c static void pcibios_scanbus(struct pci_controller *hose) · line 119
Quoted 5 lines
arch/mips/pci/pci.c +1 -0
 			pci_bus_assign_resources(bus);
+			pci_bus_add_devices(bus);
 		}
 	}
 }
This one looks wrong, I think you still want to call pci_bus_add_devices() even with PCI_PROBE_ONLY set.

Yes, this is my mistake, :(

Quoted 16 lines

Quoted 6 lines
arch/tile/kernel/pci.c +0 -0
index 1f80a88..007466e 100644
arch/tile/kernel/pci.c int __init pcibios_init(void) · line 315
Quoted 8 lines
arch/tile/kernel/pci.c +2 -0
@@ -308,6 +308,8 @@ int __init pcibios_init(void)
 			pci_add_resource(&resources, &iomem_resource);
 			bus = pci_scan_root_bus(NULL, 0, controller->ops,
 						controller, &resources);
+			if (bus)
+				pci_bus_add_devices(bus);
 			controller->root_bus = bus;
 			controller->last_busno = bus->busn_res.end;
 		}
Should the pci_bus_add_devices come after setting the bus numbers here?

I think it's doesn't matter, but move it backward is ok to me.

Quoted 5 lines

Arnd
.

Signature · 2 lines

Thanks!
Yijing