class_device_find()

class_device_find()

[Andrew Zabolotny]

Hello!

Some time ago there was a discussion about adding an class_device_find()
function. So I have a question about it.

The class_device_find() function returns a pointer to a class_device, but the
reference counter of that object is not incremented. This looks to me somewhat
racy, unless there are some details I'm missing. In my understanding,
class_device_find should be an atomic operation, and the refcount for the
returned object must be incremented prior to returning it, otherwise the
device may disappear before we do something with the returned pointer, and we
end up with a bogus pointer to nowhere.

Another thing I have noted and that looks a little wrong to me is the
sequence of code in class_device_add():

	class_dev = class_device_get(class_dev);
	if (!class_dev || !strlen(class_dev->class_id))
		return -EINVAL;

I would use rather:

	if (!class_dev || !strlen(class_dev->class_id))
		return -EINVAL;
	class_dev = class_device_get(class_dev);
	// there's no need to check class_dev here, as NULLs are detected
	// earlier, and class_device_get will crash rather than returning
	// a NULL for any other incorrect values.

Because in the first case if class_device_get succeeds, but class_id is empty,
the kobject will remain in a referenced state.

And one more thing. While preparing one more patch for submission, I realized
that the class_device_rename() function could be changed a little in order to
make it useable for setting the initial value of class_id field (I've seen
comments on drivers poking directly inside the class_device structure being a
Bad Thing):

int class_device_rename(struct class_device *class_dev, const char *new_name)
{
	char was_named = class_dev->class_id [0];

	if (was_named) {
		class_dev = class_device_get(class_dev);
		if (!class_dev)
			return -EINVAL;
		pr_debug("CLASS: renaming '%s' to '%s'\n", class_dev->class_id,
			 new_name);
	}

	strlcpy(class_dev->class_id, new_name, KOBJ_NAME_LEN);

	if (was_named) {
		kobject_rename(&class_dev->kobj, new_name);
		class_device_put(class_dev);
	}

	return 0;
}

I know patches are preferred, but our CVS is down right now and I don't have
an older copy of class.c.

And yet one more comment. What is the purpose of class_device_get at the
beginning and class_device_put at the end of the above function? I think if
someone calls class_device_rename, then it already holds a reference to the
class_device, so it can't go away while class_rename() is doing its work. And
if the caller *doesn't* hold a lock on it, it is simply wrong code since the
device may easily go away before class_rename() gets the lock.

--
Greetings,
   Andrew

P.S. Please Cc: replies to me as I'm not subscribed to this list.

P.P.S. Answering to Greg's Mar 02 2004 question about class_device_find
"Might I ask about which part of the kernel you are going to want to use this
call in?":

We (the people at handhelds.org) have implemented two subclasses for dealing
with LCD panels: the lcd class and the backlight class. The users of these
class objects (the framebuffer drivers) need some way to find the
corresponding backlight and lcd objects. We decided to give the backlight and
lcd objects the same name the framebuffer device has, e.g. framebuffer device
mq11xxfb0 uses lcd device mq11xxfb0 and backlight device mq11xxfb0. So the
class_device_find would do exactly what we need.

Re: class_device_find()

[Greg KH]

On Mon, May 24, 2004 at 10:39:21AM +0400, Andrew Zabolotny wrote:
> The class_device_find() function returns a pointer to a class_device, but the
> reference counter of that object is not incremented. This looks to me somewhat
> racy, unless there are some details I'm missing.

There were lots of problems with that function, and that's one reason
it's not in the kernel tree :)

> Because in the first case if class_device_get succeeds, but class_id is empty,
> the kobject will remain in a referenced state.

Good catch.  But your fix is not quite correct, we should call
class_device_put() on the class_dev variable before returning -EINVAL
instead.

> I know patches are preferred, but our CVS is down right now and I don't have
> an older copy of class.c.

Patches are preferred.

> And yet one more comment. What is the purpose of class_device_get at the
> beginning and class_device_put at the end of the above function? I think if
> someone calls class_device_rename, then it already holds a reference to the
> class_device, so it can't go away while class_rename() is doing its work. And
> if the caller *doesn't* hold a lock on it, it is simply wrong code since the
> device may easily go away before class_rename() gets the lock.

We are just being safe.

thanks,

greg k-h

Re: class_device_find()

[Andrew Zabolotny]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

On Mon, 24 May 2004 09:46:31 -0700
Greg KH <greg@kroah.com> wrote:

> > The class_device_find() function returns a pointer to a class_device, but
> > the reference counter of that object is not incremented. This looks to me
> > somewhat racy, unless there are some details I'm missing.
> There were lots of problems with that function, and that's one reason
> it's not in the kernel tree :)
Oops, it looks like I was looking at the wrong patch :-( My comments were true
for original Jamey's patches, but your patch already has all these issues
fixed. Mea culpa.

> > Because in the first case if class_device_get succeeds, but class_id is
> > empty, the kobject will remain in a referenced state.
> Good catch.  But your fix is not quite correct, we should call
> class_device_put() on the class_dev variable before returning -EINVAL
> instead.
That was obvious, but it's more code and no extra functionality. The same
erroneous conditions will be caught, just a bit earlier, so there's no need to
call put() since we caught it before we called get(). Unless there's no other
condition when get() returns NULL, we are safe to call get() and do not check
for the return value.

> > And yet one more comment. What is the purpose of class_device_get at the
> > beginning and class_device_put at the end of the above function?
> We are just being safe.
IMHO it is useless to try to be safe for the reasons I pointed out. If
somebody calls class_rename() without holding a reference, there's anyway a
race condition. And if it calls class_rename holding a reference, there's no
need to call get/put. Doing get/put will just minimize the chance of a race,
not remove it.

Ok, apart from this discussion, what you don't like about the
class_device_find() function? I mean your fixed implementation. The locks are
in place, the returned object has got his reference counter increased. Or,
alternatively, any other ideas how we can solve the problem I've described in
my previous message?

Also one more question. Do you think if it is okay if a subclass (such
as the lcd device class) does strcpy() directly to class_device->class_id, or
it is worth to add a function class_dev_set_name(struct class_device *, const
char*)?

I've attached your version of class_device_find() yet again, with a tiny fix
(changed name arg to "const char *" in rename() and find(), and exported
rename() which looks like it has been forgotten to be). Also I still would
remove the get()/put() from rename() but this has not been done in this patch.

--
Greetings,
   Andrew

[-- Attachment #2: class.diff --]
[-- Type: application/octet-stream, Size: 2352 bytes --]

--- linux-2.6.6/drivers/base/class.c	2004-05-10 06:33:21.000000000 +0400
+++ kernel26/drivers/base/class.c	2004-05-25 02:03:40.000000000 +0400
@@ -359,7 +359,7 @@
 	class_device_put(class_dev);
 }
 
-int class_device_rename(struct class_device *class_dev, char *new_name)
+int class_device_rename(struct class_device *class_dev, const char *new_name)
 {
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
@@ -377,6 +377,33 @@
 	return 0;
 }
 
+/**
+ * class_device_find - find a struct class_device in a specific class
+ * @class: the class to search
+ * @class_id: the class_id to search for
+ *
+ * Iterates through the list of all class devices registered to a class. If
+ * the class_id is found, its reference count is incremented and returned to
+ * the caller. If the class_id does not match any existing struct class_device
+ * registered to this struct class, then NULL is returned.
+ */
+struct class_device * class_device_find(struct class *class, const char *class_id)
+{
+	struct class_device *class_dev;
+	struct class_device *found = NULL;
+
+	down_read(&class->subsys.rwsem);
+	list_for_each_entry(class_dev, &class->children, node) {
+		if (strcmp(class_dev->class_id, class_id) == 0) {
+			found = class_device_get(class_dev);
+			break;
+		}
+	}
+	up_read(&class->subsys.rwsem);
+
+	return found;
+}
+
 struct class_device * class_device_get(struct class_device *class_dev)
 {
 	if (class_dev)
@@ -468,6 +495,8 @@
 EXPORT_SYMBOL(class_device_put);
 EXPORT_SYMBOL(class_device_create_file);
 EXPORT_SYMBOL(class_device_remove_file);
+EXPORT_SYMBOL(class_device_rename);
+EXPORT_SYMBOL(class_device_find);
 
 EXPORT_SYMBOL(class_interface_register);
 EXPORT_SYMBOL(class_interface_unregister);
--- linux-2.6.6/include/linux/device.h	2004-05-10 06:33:19.000000000 +0400
+++ kernel26/include/linux/device.h	2004-05-25 01:54:30.000000000 +0400
@@ -212,8 +212,9 @@
 extern int class_device_add(struct class_device *);
 extern void class_device_del(struct class_device *);
 
-extern int class_device_rename(struct class_device *, char *);
+extern int class_device_rename(struct class_device *, const char *);
 
+extern struct class_device * class_device_find(struct class *class, const char *class_id);
 extern struct class_device * class_device_get(struct class_device *);
 extern void class_device_put(struct class_device *);
 

Re: class_device_find()

[Greg KH]

On Tue, May 25, 2004 at 02:08:03AM +0400, Andrew Zabolotny wrote:
> Ok, apart from this discussion, what you don't like about the
> class_device_find() function? I mean your fixed implementation. The locks are
> in place, the returned object has got his reference counter increased. Or,
> alternatively, any other ideas how we can solve the problem I've described in
> my previous message?

No in-kernel code uses it.  That's my main objection.  If a patch is
submitted that needs it, I'll reconsider it based on that patch.

> Also one more question. Do you think if it is okay if a subclass (such
> as the lcd device class) does strcpy() directly to class_device->class_id, or
> it is worth to add a function class_dev_set_name(struct class_device *, const
> char*)?

strncpy() is fine to do at this time.

thanks,

greg k-h