PPC64: vio_find_node removal?

PPC64: vio_find_node removal?

[Dmitry Torokhov]

Hi,

kset_find_obj that is now only used by vio_find_name/vio_find_node needs to
take kobject reference otherwise use of this function is generally unsafe.

I was looking at vio_find_name users and it is only used in rpaphp hotplug
driver. When creating a hotplug slot the function first tries to find already
existing vio node and if unsuccessfull tries to create a new one. The only
time when vio node would already exist if previous call to register_vio_slot
failed (the function does not do cleanup of created vio device node and it's
the only place where vio devices are created). So it seems to me that if
register_vio_slot would do proper cleanup we can get rid of vio_find_name/
vio_find_node. 

Please CC me as I am not subscribed to ppc64 list.

-- 
Dmitry

===== arch/ppc64/kernel/vio.c 1.26 vs edited =====
--- 1.26/arch/ppc64/kernel/vio.c	2004-06-29 09:44:46 -05:00
+++ edited/arch/ppc64/kernel/vio.c	2004-07-01 14:38:22 -05:00
@@ -32,8 +32,6 @@
 
 #define DBGENTER() pr_debug("%s entered\n", __FUNCTION__)
 
-extern struct subsystem devices_subsys; /* needed for vio_find_name() */
-
 static const struct vio_device_id *vio_match_device(
 		const struct vio_device_id *, const struct vio_dev *);
 
@@ -458,42 +456,6 @@
 	return get_property(vdev->dev.platform_data, (char*)which, length);
 }
 EXPORT_SYMBOL(vio_get_attribute);
-
-/* vio_find_name() - internal because only vio.c knows how we formatted the
- * kobject name
- * XXX once vio_bus_type.devices is actually used as a kset in
- * drivers/base/bus.c, this function should be removed in favor of
- * "device_find(kobj_name, &vio_bus_type)"
- */
-static struct vio_dev *vio_find_name(const char *kobj_name)
-{
-	struct kobject *found;
-
-	found = kset_find_obj(&devices_subsys.kset, kobj_name);
-	if (!found)
-		return NULL;
-
-	return to_vio_dev(container_of(found, struct device, kobj));
-}
-
-/**
- * vio_find_node - find an already-registered vio_dev
- * @vnode: device_node of the virtual device we're looking for
- */
-struct vio_dev *vio_find_node(struct device_node *vnode)
-{
-	uint32_t *unit_address;
-	char kobj_name[BUS_ID_SIZE];
-
-	/* construct the kobject name from the device node */
-	unit_address = (uint32_t *)get_property(vnode, "reg", NULL);
-	if (!unit_address)
-		return NULL;
-	snprintf(kobj_name, BUS_ID_SIZE, "%x", *unit_address);
-
-	return vio_find_name(kobj_name);
-}
-EXPORT_SYMBOL(vio_find_node);
 
 /**
  * vio_build_iommu_table: - gets the dma information from OF and builds the TCE tree.
===== drivers/pci/hotplug/rpaphp_vio.c 1.4 vs edited =====
--- 1.4/drivers/pci/hotplug/rpaphp_vio.c	2004-06-29 09:44:45 -05:00
+++ edited/drivers/pci/hotplug/rpaphp_vio.c	2004-07-01 14:37:11 -05:00
@@ -85,9 +85,7 @@
 		goto exit_rc;
 	}
 	slot->dev_type = VIO_DEV;
-	slot->dev.vio_dev = vio_find_node(dn);
-	if (!slot->dev.vio_dev)
-		slot->dev.vio_dev = vio_register_device_node(dn);
+	slot->dev.vio_dev = vio_register_device_node(dn);
 	if (slot->dev.vio_dev)
 		slot->state = CONFIGURED;
 	else
@@ -99,8 +97,11 @@
 		__FUNCTION__, slot->name, slot->dev.vio_dev); 
 	rc = register_slot(slot);
 exit_rc:
-	if (rc && slot)
+	if (rc && slot) {
+		if (slot->dev.vio_dev)
+			vio_unregister_device(slot->dev.vio_dev);
 		dealloc_slot_struct(slot);
+	}
 	return (rc);
 }
 
===== include/asm-ppc64/vio.h 1.11 vs edited =====
--- 1.11/include/asm-ppc64/vio.h	2004-06-29 09:44:45 -05:00
+++ edited/include/asm-ppc64/vio.h	2004-07-01 14:39:10 -05:00
@@ -50,7 +50,6 @@
 		struct device_node *node_vdev);
 #endif
 void __devinit vio_unregister_device(struct vio_dev *dev);
-struct vio_dev *vio_find_node(struct device_node *vnode);
 
 const void * vio_get_attribute(struct vio_dev *vdev, void* which, int* length);
 int vio_get_irq(struct vio_dev *dev);

Re: PPC64: vio_find_node removal?

[Hollis Blanchard]

On Thu, 2004-07-01 at 14:54, Dmitry Torokhov wrote:

> kset_find_obj that is now only used by vio_find_name/vio_find_node needs to
> take kobject reference otherwise use of this function is generally unsafe.
> 
> I was looking at vio_find_name users and it is only used in rpaphp hotplug
> driver. When creating a hotplug slot the function first tries to find already
> existing vio node and if unsuccessfull tries to create a new one. The only
> time when vio node would already exist if previous call to register_vio_slot
> failed (the function does not do cleanup of created vio device node and it's
> the only place where vio devices are created). So it seems to me that if
> register_vio_slot would do proper cleanup we can get rid of vio_find_name/
> vio_find_node.

At boot time, all the virtual IO devices are registered and matched with
their drivers (or not). Later on (possibly when loading a module),
rpaphp initializes. rpaphp needs a reference to the already-registered
VIO devices so that it can hotplug-remove them later by calling
vio_unregister_device().

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: PPC64: vio_find_node removal?

[Dmitry Torokhov]

On Thursday 01 July 2004 05:26 pm, Hollis Blanchard wrote:
> On Thu, 2004-07-01 at 14:54, Dmitry Torokhov wrote:
> 
> > kset_find_obj that is now only used by vio_find_name/vio_find_node needs to
> > take kobject reference otherwise use of this function is generally unsafe.
> > 
> > I was looking at vio_find_name users and it is only used in rpaphp hotplug
> > driver. When creating a hotplug slot the function first tries to find already
> > existing vio node and if unsuccessfull tries to create a new one. The only
> > time when vio node would already exist if previous call to register_vio_slot
> > failed (the function does not do cleanup of created vio device node and it's
> > the only place where vio devices are created). So it seems to me that if
> > register_vio_slot would do proper cleanup we can get rid of vio_find_name/
> > vio_find_node.
> 
> At boot time, all the virtual IO devices are registered and matched with
> their drivers (or not). Later on (possibly when loading a module),
> rpaphp initializes. rpaphp needs a reference to the already-registered
> VIO devices so that it can hotplug-remove them later by calling
> vio_unregister_device().
>

Ah, I see.. I missed the call to vio_register_device_node in
probe_bus_pseries...

Ok, so if we add call to kobject_get in kset_find_obj we can just add
kobject_put right in vio_find_name because there can be only one-to-one
match between a slot and a vio device and we don't need refcounting there,
right?

-- 
Dmitry

Re: PPC64: vio_find_node removal?

[Hollis Blanchard]

On Jul 1, 2004, at 5:51 PM, Dmitry Torokhov wrote:
>
> Ok, so if we add call to kobject_get in kset_find_obj we can just add
> kobject_put right in vio_find_name because there can be only one-to-one
> match between a slot and a vio device and we don't need refcounting 
> there,
> right?

Hmm. Yes, I agree that we need kobject_get and _put between 
kset_find_obj() and vio_find_name().

As for the lack of vio_dev refcounting... I think we can get away with 
it because rpaphp_vio.c is the only user who might unregister a 
vio_dev.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: PPC64: vio_find_node removal?

[Dmitry Torokhov]

On Friday 02 July 2004 09:28 am, Hollis Blanchard wrote:
> On Jul 1, 2004, at 5:51 PM, Dmitry Torokhov wrote:
> >
> > Ok, so if we add call to kobject_get in kset_find_obj we can just add
> > kobject_put right in vio_find_name because there can be only one-to-one
> > match between a slot and a vio device and we don't need refcounting 
> > there,
> > right?
> 
> Hmm. Yes, I agree that we need kobject_get and _put between 
> kset_find_obj() and vio_find_name().

I tried putting kobject_put in vio_find_name but it felt extremely dirty.
So I moved it to the caller - rpaphp_vio::register_vio_slot(). What do
you think? (I am also CC-ing Greg KH so he can apply if there is an
agreement.)


===================================================================


ChangeSet@1.1778.5.4, 2004-07-03 21:19:34-05:00, dtor_core@ameritech.net
  Driver core: kset_find_obj should increment refcount of the found object
               so users of the function can safely use returned object
  
  Signed-off-by: Dmitry Torokhov <dtor@mail.ru>


 drivers/base/bus.c               |    2 ++
 drivers/base/core.c              |   10 ++++++++++
 drivers/pci/hotplug/rpaphp_vio.c |    9 ++++++++-
 lib/kobject.c                    |    7 ++++---
 4 files changed, 24 insertions(+), 4 deletions(-)


===================================================================



diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c	2004-07-03 21:25:37 -05:00
+++ b/drivers/base/bus.c	2004-07-03 21:25:37 -05:00
@@ -607,6 +607,8 @@
  *
  *	Call kset_find_obj() to iterate over list of buses to
  *	find a bus by name. Return bus if found.
+ *
+ *	Note that kset_find_obj increments bus' reference count.
  */
 
 struct bus_type * find_bus(char * name)
diff -Nru a/drivers/base/core.c b/drivers/base/core.c
--- a/drivers/base/core.c	2004-07-03 21:25:37 -05:00
+++ b/drivers/base/core.c	2004-07-03 21:25:37 -05:00
@@ -378,6 +378,16 @@
 	return error;
 }
 
+/**
+ *	device_find - locate device on a bus by name.
+ *	@name:	name of the device.
+ *	@bus:	bus to scan for the device.
+ *
+ *	Call kset_find_obj() to iterate over list of devices on
+ *	a bus to find device by name. Return device if found.
+ *
+ *	Note that kset_find_obj increments device's reference count.
+ */
 struct device *device_find(const char *name, struct bus_type *bus)
 {
 	struct kobject *k = kset_find_obj(&bus->devices, name);
diff -Nru a/drivers/pci/hotplug/rpaphp_vio.c b/drivers/pci/hotplug/rpaphp_vio.c
--- a/drivers/pci/hotplug/rpaphp_vio.c	2004-07-03 21:25:37 -05:00
+++ b/drivers/pci/hotplug/rpaphp_vio.c	2004-07-03 21:25:37 -05:00
@@ -86,7 +86,14 @@
 	}
 	slot->dev_type = VIO_DEV;
 	slot->dev.vio_dev = vio_find_node(dn);
-	if (!slot->dev.vio_dev)
+	if (slot->dev.vio_dev) {
+		/*
+		 * rpaphp is the only owner of vio devices and
+		 * does not need extra reference taken by
+		 * vio_find_node
+		 */
+		put_device(&slot->dev.vio_dev->dev);
+	} else
 		slot->dev.vio_dev = vio_register_device_node(dn);
 	if (slot->dev.vio_dev)
 		slot->state = CONFIGURED;
diff -Nru a/lib/kobject.c b/lib/kobject.c
--- a/lib/kobject.c	2004-07-03 21:25:37 -05:00
+++ b/lib/kobject.c	2004-07-03 21:25:37 -05:00
@@ -537,7 +537,8 @@
  *	@name:	object's name.
  *
  *	Lock kset via @kset->subsys, and iterate over @kset->list,
- *	looking for a matching kobject. Return object if found.
+ *	looking for a matching kobject. If matching object is found
+ *	take a reference and return the object.
  */
 
 struct kobject * kset_find_obj(struct kset * kset, const char * name)
@@ -548,8 +549,8 @@
 	down_read(&kset->subsys->rwsem);
 	list_for_each(entry,&kset->list) {
 		struct kobject * k = to_kobj(entry);
-		if (kobject_name(k) && (!strcmp(kobject_name(k),name))) {
-			ret = k;
+		if (kobject_name(k) && !strcmp(kobject_name(k),name)) {
+			ret = kobject_get(k);
 			break;
 		}
 	}