Messages in this thread | | | Date | Mon, 24 May 2004 10:39:21 +0400 | From | Andrew Zabolotny <> | Subject | class_device_find() |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |