add module reference counts to sysfs attribute files

RFC In review Greg KH 2 participants 3 messages Jul 2, 2003 – Jul 3, 2003 Driver core

lore

Share & tools

lore

Greg KH

author

2003-07-02 21:44 UTC #0 lore raw

RFC 1/1 add module reference counts to sysfs attribute files

Hi all,

Here's a patch against 2.5.74 that adds module reference counting to sysfs attribute files. We already protect kobject from going away when attribute files are open, but we also need to protect the code that the kobject is referencing from going away too. This patch fixes that hole, and as a nice side benefit allows drivers to put attributes in kobjects that they don't necessarily own (but you still have to be careful about how you do this, grab the reference first, add the files. then on exit, delete the files and then decrement the count.)

This patch works for me on testing of files in pci devices and /sys/class/usb_host/ as well as other places in sysfs. Nice part of this patch is that nothing is needed to be changed for drivers that already use the *_ATTR() macros to create files. If you do not use them, then you have to set the .owner field on your own.

If no one has any objections to it I'll send it on to Linus in a bit.

thanks,

greg k-h

p.s. You will also need a jiffies cleanup patch which I've included at the end of this patch if you want to build this. I've told the author of that file what needs to be fixed so that shouldn't be necessary for long.

# SYSFS: add module referencing to sysfs attribute files.

fs/sysfs/file.c +9 -0

diff -Nru a/fs/sysfs/file.c b/fs/sysfs/file.c
@@ -247,6 +247,12 @@
 	if (!kobj || !attr)
 		goto Einval;
 
+	/* Grab the module reference for this attribute if we have one */
+	if (!try_module_get(attr->owner)) {
+		error = -ENODEV;
+		goto Done;
+	}
+
 	/* if the kobject has no ktype, then we assume that it is a subsystem
 	 * itself, and use ops for it.
 	 */

@@ -300,6 +306,7 @@
 	goto Done;
  Eaccess:
 	error = -EACCES;
+	module_put(attr->owner);
  Done:
 	if (error && kobj)
 		kobject_put(kobj);

@@ -314,10 +321,12 @@
 static int sysfs_release(struct inode * inode, struct file * filp)
 {
 	struct kobject * kobj = filp->f_dentry->d_parent->d_fsdata;
+	struct attribute * attr = filp->f_dentry->d_fsdata;
 	struct sysfs_buffer * buffer = filp->private_data;
 
 	if (kobj) 
 		kobject_put(kobj);
+	module_put(attr->owner);
 
 	if (buffer) {
 		if (buffer->page)

include/linux/device.h +6 -5

diff -Nru a/include/linux/device.h b/include/linux/device.h
@@ -18,6 +18,7 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/ioport.h>
+#include <linux/module.h>
 #include <asm/semaphore.h>
 #include <asm/atomic.h>

@@ -95,7 +96,7 @@
 
 #define BUS_ATTR(_name,_mode,_show,_store)	\
 struct bus_attribute bus_attr_##_name = { 		\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
 	.show	= _show,				\
 	.store	= _store,				\
 };

@@ -136,7 +137,7 @@
 
 #define DRIVER_ATTR(_name,_mode,_show,_store)	\
 struct driver_attribute driver_attr_##_name = { 		\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
 	.show	= _show,				\
 	.store	= _store,				\
 };

@@ -176,7 +177,7 @@
 
 #define CLASS_ATTR(_name,_mode,_show,_store)			\
 struct class_attribute class_attr_##_name = { 			\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
 	.show	= _show,					\
 	.store	= _store,					\
 };

@@ -226,7 +227,7 @@
 
 #define CLASS_DEVICE_ATTR(_name,_mode,_show,_store)		\
 struct class_device_attribute class_device_attr_##_name = { 	\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
 	.show	= _show,					\
 	.store	= _store,					\
 };

@@ -324,7 +325,7 @@
 
 #define DEVICE_ATTR(_name,_mode,_show,_store) \
 struct device_attribute dev_attr_##_name = { 		\
-	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.attr = {.name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
 	.show	= _show,				\
 	.store	= _store,				\
 };

include/linux/sysfs.h +2 -0

diff -Nru a/include/linux/sysfs.h b/include/linux/sysfs.h
@@ -10,9 +10,11 @@
 #define _SYSFS_H_
 
 struct kobject;
+struct module;
 
 struct attribute {
 	char			* name;
+	struct module 		* owner;
 	mode_t			mode;
 };

# fix jiffies compiler warning.

arch/i386/kernel/timers/timer_tsc.c +0 -1

diff -Nru a/arch/i386/kernel/timers/timer_tsc.c b/arch/i386/kernel/timers/timer_tsc.c
@@ -21,7 +21,6 @@
 int tsc_disable __initdata = 0;
 
 extern spinlock_t i8253_lock;
-extern unsigned long jiffies;
 
 static int use_tsc;
 /* Number of usecs that the last interrupt was delayed */

Jeff Garzik

2003-07-02 23:34 UTC #1 lore raw

inode/dcache overhead of sysfs attribute files?

Has anybody looked into the inode and dcache overhead of all this stuff, which I assume is pinned into memory a la ramfs?

I wonder if sysfs attributes could be accessed via the extended attribute VFS API? A file full of EA's can easily be considered a key-value database, or attribute-value in this case :) The EA names and values need not pin a bunch of inodes and dcache entries, either. (though viro may scream at my mention of EAs :))

Jeff

Greg KH

author

2003-07-03 16:06 UTC #2 lore raw

Re: inode/dcache overhead of sysfs attribute files?

Quoted 2 lines · Jeff Garzik

Has anybody looked into the inode and dcache overhead of all this stuff, which I assume is pinned into memory a la ramfs?

Yes, there were some numbers in an old thread about 2000 scsi disks.

Quoted 5 lines

I wonder if sysfs attributes could be accessed via the extended attribute VFS API? A file full of EA's can easily be considered a key-value database, or attribute-value in this case :) The EA names and values need not pin a bunch of inodes and dcache entries, either. (though viro may scream at my mention of EAs :))

Don't know, that could be one way. I think wli and shaggy were working on something along these lines, but haven't heard from them in a long time about it.

thanks,

greg k-h