[v3,2/6] PCI: split registration of PCI bus devices into two stages

Submitted by Jiang Liu on Jan. 8, 2013, 4:52 p.m.

Details

Message ID 1357663945-16054-3-git-send-email-jiang.liu@huawei.com
State Superseded
Headers show

Commit Message

Jiang Liu Jan. 8, 2013, 4:52 p.m.
When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device,
the notification handler can't hold reference count to the new PCI bus
because the device object for the new bus (pci_dev->subordinate->dev)
hasn't been initialized yet.

Split the registration of PCI bus device into two stages as below,
so that the event handler could hold reference count to the new PCI bus
when handling BUS_NOTIFY_ADD_DEVICE event.

1) device_initialize(&pci_dev->dev)
2) device_initialize(&pci_dev->subordinate->dev)
3) notify BUS_NOTIFY_ADD_DEVICE event for pci_dev
4) device_add(&pci_dev->dev)
5) device_add(&pci_dev->subordinate->dev)

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/bus.c    |    2 +-
 drivers/pci/probe.c  |    4 +++-
 drivers/pci/remove.c |   10 +++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

Comments

Rafael J. Wysocki Jan. 8, 2013, 11:29 p.m.
On Wednesday, January 09, 2013 12:52:21 AM Jiang Liu wrote:
> When handling BUS_NOTIFY_ADD_DEVICE event for a PCI bridge device,
> the notification handler can't hold reference count to the new PCI bus
> because the device object for the new bus (pci_dev->subordinate->dev)
> hasn't been initialized yet.
> 
> Split the registration of PCI bus device into two stages as below,
> so that the event handler could hold reference count to the new PCI bus
> when handling BUS_NOTIFY_ADD_DEVICE event.
> 
> 1) device_initialize(&pci_dev->dev)
> 2) device_initialize(&pci_dev->subordinate->dev)
> 3) notify BUS_NOTIFY_ADD_DEVICE event for pci_dev
> 4) device_add(&pci_dev->dev)
> 5) device_add(&pci_dev->subordinate->dev)
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/pci/bus.c    |    2 +-
>  drivers/pci/probe.c  |    4 +++-
>  drivers/pci/remove.c |   10 +++++-----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index ad6a8b6..2fea481 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -198,7 +198,7 @@ int pci_bus_add_child(struct pci_bus *bus)
>  	if (bus->bridge)
>  		bus->dev.parent = bus->bridge;
>  
> -	retval = device_register(&bus->dev);
> +	retval = device_add(&bus->dev);
>  	if (retval)
>  		return retval;
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e557d6f..a8315c2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -642,6 +642,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	 */
>  	child->dev.class = &pcibus_class;
>  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> +	device_initialize(&child->dev);
>  
>  	/*
>  	 * Set up the primary, secondary and subordinate
> @@ -1678,7 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	b->dev.class = &pcibus_class;
>  	b->dev.parent = b->bridge;
>  	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> -	error = device_register(&b->dev);
> +	device_initialize(&b->dev);
> +	error = device_add(&b->dev);

Well, the change here isn't really necessary, as far as I can say.

>  	if (error)
>  		goto class_dev_reg_err;
>  
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index fc38c48..a1fdd0f 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -48,11 +48,11 @@ void pci_remove_bus(struct pci_bus *bus)
>  	list_del(&bus->node);
>  	pci_bus_release_busn_res(bus);
>  	up_write(&pci_bus_sem);
> -	if (!bus->is_added)
> -		return;
> -
> -	pci_remove_legacy_files(bus);
> -	device_unregister(&bus->dev);
> +	if (bus->is_added) {
> +		pci_remove_legacy_files(bus);
> +		device_del(&bus->dev);
> +	}
> +	put_device(&bus->dev);
>  }
>  EXPORT_SYMBOL(pci_remove_bus);
>  
> 

Apart from the above

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Jiang Liu Jan. 9, 2013, 4:10 p.m.
On 01/09/2013 07:29 AM, Rafael J. Wysocki wrote:
> On Wednesday, January 09, 2013 12:52:21 AM Jiang Liu wrote:
snip
>> @@ -1678,7 +1679,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>  	b->dev.class = &pcibus_class;
>>  	b->dev.parent = b->bridge;
>>  	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
>> -	error = device_register(&b->dev);
>> +	device_initialize(&b->dev);
>> +	error = device_add(&b->dev);
> 
> Well, the change here isn't really necessary, as far as I can say.
Hi Rafael,
	The kernel documentation recommends to pair device_add() with device_del(),
so the above change is to follow that recommendation because pci_remove_bus() now
invokes device_del().

> 
>>  	if (error)
>>  		goto class_dev_reg_err;
>>  
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index fc38c48..a1fdd0f 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -48,11 +48,11 @@ void pci_remove_bus(struct pci_bus *bus)
>>  	list_del(&bus->node);
>>  	pci_bus_release_busn_res(bus);
>>  	up_write(&pci_bus_sem);
>> -	if (!bus->is_added)
>> -		return;
>> -
>> -	pci_remove_legacy_files(bus);
>> -	device_unregister(&bus->dev);
>> +	if (bus->is_added) {
>> +		pci_remove_legacy_files(bus);
>> +		device_del(&bus->dev);
>> +	}
>> +	put_device(&bus->dev);
>>  }
>>  EXPORT_SYMBOL(pci_remove_bus);
>>  
>>
> 
> Apart from the above
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Thanks for your review and Ack.

Regards!
Gerry

--
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

Patch hide | download patch | download mbox

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index ad6a8b6..2fea481 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -198,7 +198,7 @@  int pci_bus_add_child(struct pci_bus *bus)
 	if (bus->bridge)
 		bus->dev.parent = bus->bridge;
 
-	retval = device_register(&bus->dev);
+	retval = device_add(&bus->dev);
 	if (retval)
 		return retval;
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index e557d6f..a8315c2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -642,6 +642,7 @@  static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
+	device_initialize(&child->dev);
 
 	/*
 	 * Set up the primary, secondary and subordinate
@@ -1678,7 +1679,8 @@  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
 	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(&b->dev);
+	device_initialize(&b->dev);
+	error = device_add(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
 
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index fc38c48..a1fdd0f 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -48,11 +48,11 @@  void pci_remove_bus(struct pci_bus *bus)
 	list_del(&bus->node);
 	pci_bus_release_busn_res(bus);
 	up_write(&pci_bus_sem);
-	if (!bus->is_added)
-		return;
-
-	pci_remove_legacy_files(bus);
-	device_unregister(&bus->dev);
+	if (bus->is_added) {
+		pci_remove_legacy_files(bus);
+		device_del(&bus->dev);
+	}
+	put_device(&bus->dev);
 }
 EXPORT_SYMBOL(pci_remove_bus);