diff mbox

[v4,01/30] PCI: Rip out pci_bus_add_devices() from pci_scan_bus()

Message ID 1424938344-4017-2-git-send-email-wangyijing@huawei.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Yijing Wang Feb. 26, 2015, 8:11 a.m. UTC
Pci_bus_add_devices() should not be placed in pci_scan_bus().
Now pci device will be added to driver core once its
creation. All things left in pci_bus_add_devices() are
driver attachment and other trivial sysfs things.
Pci_scan_bus() should be the function responsible for
scanning PCI devices, not including driver attachment.
Other, some callers(m68k,unicore32,alpha) of pci_scan_bus()
will call pci_bus_size_bridges() and pci_bus_assign_resources()
after pci_scan_bus().

E.g.
In m68k
mcf_pci_init()
	pci_scan_bus()
		...
		pci_bus_add_devices() --- try to attach driver
	pci_fixup_irqs()
	pci_bus_size_bridges()
	pci_bus_assign_resources()

Above code flow make pci scan logic obscure, resources should
be assigned correctly before attaching driver. Because
pci_scan_bus() mostly be called in system boot path, when
before drivers are loaded(in module_init), so no pci scan
issues will occur. Most pci_scan_bus() callers additionally
call pci_bus_size_bridges() and pci_bus_assign_resources()
after pci_scan_bus(), we could rip out pci_bus_add_devices()
for better code readability. After applied this patch,
pci_scan_bus() should be used like:

pci_scan_bus() (mandatory)
pci_fixup_irqs() (optional)
pci_bus_size_bridges() (optional)
pci_pci_bus_assign_resources() (optional)
pci_bus_add_devices() (mandatory)

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Guan Xuetao <gxt@mprc.pku.edu.cn>
CC: Richard Henderson <rth@twiddle.net>
CC: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
CC: Matt Turner <mattst88@gmail.com>
CC: linux-alpha@vger.kernel.org
CC: linux-m68k@lists.linux-m68k.org
CC: sparclinux@vger.kernel.org
---
 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(-)

Comments

Bjorn Helgaas March 2, 2015, 11:41 p.m. UTC | #1
On Thu, Feb 26, 2015 at 04:11:55PM +0800, Yijing Wang wrote:
> Pci_bus_add_devices() should not be placed in pci_scan_bus().
> Now pci device will be added to driver core once its
> creation. All things left in pci_bus_add_devices() are
> driver attachment and other trivial sysfs things.
> Pci_scan_bus() should be the function responsible for
> scanning PCI devices, not including driver attachment.
> Other, some callers(m68k,unicore32,alpha) of pci_scan_bus()
> will call pci_bus_size_bridges() and pci_bus_assign_resources()
> after pci_scan_bus().
> 
> E.g.
> In m68k
> mcf_pci_init()
> 	pci_scan_bus()
> 		...
> 		pci_bus_add_devices() --- try to attach driver
> 	pci_fixup_irqs()
> 	pci_bus_size_bridges()
> 	pci_bus_assign_resources()
> 
> Above code flow make pci scan logic obscure, resources should
> be assigned correctly before attaching driver. Because
> pci_scan_bus() mostly be called in system boot path, when
> before drivers are loaded(in module_init), so no pci scan
> issues will occur. Most pci_scan_bus() callers additionally
> call pci_bus_size_bridges() and pci_bus_assign_resources()
> after pci_scan_bus(), we could rip out pci_bus_add_devices()
> for better code readability. After applied this patch,
> pci_scan_bus() should be used like:
> 
> pci_scan_bus() (mandatory)
> pci_fixup_irqs() (optional)
> pci_bus_size_bridges() (optional)
> pci_pci_bus_assign_resources() (optional)
> pci_bus_add_devices() (mandatory)
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Guan Xuetao <gxt@mprc.pku.edu.cn>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> CC: Matt Turner <mattst88@gmail.com>
> CC: linux-alpha@vger.kernel.org
> CC: linux-m68k@lists.linux-m68k.org
> CC: sparclinux@vger.kernel.org
> ---
>  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(-)
> 
> diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c
> index 837c0fa..4ae4a40 100644
> --- a/arch/alpha/kernel/sys_nautilus.c
> +++ b/arch/alpha/kernel/sys_nautilus.c
> @@ -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);
>  }
>  
>  /*
> diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
> index df96792..d45f087 100644
> --- a/arch/m68k/coldfire/pci.c
> +++ b/arch/m68k/coldfire/pci.c
> @@ -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;
>  }
>  
> diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c
> index 6cc78c2..7a82fe2 100644
> --- a/arch/sparc/kernel/pcic.c
> +++ b/arch/sparc/kernel/pcic.c
> @@ -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);

This is correct, but it should look like this instead:

    if (!pbm->pci_bus)
      return;
  #if 0 /* deadwood ... */
  ...
  #endif
    pci_bus_add_devices(pbm->pci_bus);
  }

That way the mainline code is the normal path, and pci_bus_add_devices() is
in the correct place even if the "deadwood" code is resurrected.


> diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c
> index 374a055..3d82024 100644
> --- a/arch/unicore32/kernel/pci.c
> +++ b/arch/unicore32/kernel/pci.c
> @@ -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);
> diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
> index 96c5c72..86e3bfd 100644
> --- a/drivers/pci/hotplug/ibmphp_core.c
> +++ b/drivers/pci/hotplug/ibmphp_core.c
> @@ -738,7 +738,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;
>  
> @@ -765,7 +765,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);

Use this:

    if (!b)
      continue;
    pci_bus_add_devices(b);

for the same reason as above.

>  			break;
>  		}
>  	}
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8d2f400..88604f2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2123,7 +2123,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);
>  	}
> -- 
> 1.7.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yijing Wang March 3, 2015, 1:12 a.m. UTC | #2
>>  	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);
> 
> This is correct, but it should look like this instead:
> 
>     if (!pbm->pci_bus)
>       return;
>   #if 0 /* deadwood ... */
>   ...
>   #endif
>     pci_bus_add_devices(pbm->pci_bus);
>   }
> 
> That way the mainline code is the normal path, and pci_bus_add_devices() is
> in the correct place even if the "deadwood" code is resurrected.
> 

Yes, you are right, thanks!

> 
>> diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c
>> index 374a055..3d82024 100644
>> --- a/arch/unicore32/kernel/pci.c
>> +++ b/arch/unicore32/kernel/pci.c
>> @@ -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);
>> diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
>> index 96c5c72..86e3bfd 100644
>> --- a/drivers/pci/hotplug/ibmphp_core.c
>> +++ b/drivers/pci/hotplug/ibmphp_core.c
>> @@ -738,7 +738,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;
>>  
>> @@ -765,7 +765,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);
> 
> Use this:
> 
>     if (!b)
>       continue;
>     pci_bus_add_devices(b);
> 
> for the same reason as above.


Will update, thanks!

Thanks!
Yijing.

> 
>>  			break;
>>  		}
>>  	}
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 8d2f400..88604f2 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2123,7 +2123,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);
>>  	}
>> -- 
>> 1.7.1
>>
> 
> .
>
diff mbox

Patch

diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c
index 837c0fa..4ae4a40 100644
--- a/arch/alpha/kernel/sys_nautilus.c
+++ b/arch/alpha/kernel/sys_nautilus.c
@@ -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);
 }
 
 /*
diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c
index df96792..d45f087 100644
--- a/arch/m68k/coldfire/pci.c
+++ b/arch/m68k/coldfire/pci.c
@@ -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;
 }
 
diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c
index 6cc78c2..7a82fe2 100644
--- a/arch/sparc/kernel/pcic.c
+++ b/arch/sparc/kernel/pcic.c
@@ -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);
diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c
index 374a055..3d82024 100644
--- a/arch/unicore32/kernel/pci.c
+++ b/arch/unicore32/kernel/pci.c
@@ -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);
diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c
index 96c5c72..86e3bfd 100644
--- a/drivers/pci/hotplug/ibmphp_core.c
+++ b/drivers/pci/hotplug/ibmphp_core.c
@@ -738,7 +738,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;
 
@@ -765,7 +765,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;
 		}
 	}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8d2f400..88604f2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2123,7 +2123,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);
 	}