sysfs_create_subdir() is used by the attribute_group code to create a
subdirectory in sysfs in which to store attributes in a more organized
fashion than in a flat directory. It works well and does exactly as
advertised.
However, I've recently run into a couple of limitations with the interface
The patch below addresses these issues.
First, two different pieces of code cannot create attributes in the same
subdirectory without sharing the attribute_group data structure. For
instance, each device has a 'power' subdirectory (for better or worse :).
It is not (easily) possible for a modular piece of code to add attributes
in the power directory, when what you really want to do is e.g.:
struct attribute group my_power_attrs = {
.name = "power",
...
};
...
sysfs_create_group(dev->kobj, &my_power_attrs);
And have them appear in the right place.
The patch addresses this problem by recognizing when a subdirectory
already exists when a group is added and not returning an error.
Secondly, there is sometimes a need to organize things a bit more than
with just one subdirectory. Sometimes, you may want to separate attributes
based on category, without creating painfully long attribute names. The
patch at:
http://kernel.org/pub/linux/kernel/people/mochel/patches/acpi-cpu-info.patch
illustrates this by putting ACPI C state information for processors into
the subdirectory acpi/pm/c1/ etc. This patch is just an example (though it
is a working one).
The patch below addresses this issue by parsing the subdirectory name and
creating any parent directories delineated by a '/'.
Thoughts?
Pat
Signed-off-by: Patrick Mochel <[email protected]>
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 59734ba..cb820ac 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -94,38 +94,127 @@ static int init_symlink(struct inode * i
}
static int create_dir(struct kobject * k, struct dentry * p,
- const char * n, struct dentry ** d)
+ struct dentry * dir)
{
int error;
umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
- down(&p->d_inode->i_sem);
- *d = lookup_one_len(n, p, strlen(n));
- if (!IS_ERR(*d)) {
- error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
+ error = sysfs_make_dirent(p->d_fsdata, dir, k, mode, SYSFS_DIR);
+ if (!error) {
+ error = sysfs_create(dir, mode, init_dir);
if (!error) {
- error = sysfs_create(*d, mode, init_dir);
- if (!error) {
- p->d_inode->i_nlink++;
- (*d)->d_op = &sysfs_dentry_ops;
- d_rehash(*d);
- }
- }
- if (error && (error != -EEXIST)) {
- sysfs_put((*d)->d_fsdata);
- d_drop(*d);
+ p->d_inode->i_nlink++;
+ dir->d_op = &sysfs_dentry_ops;
+ d_rehash(dir);
}
- dput(*d);
+ dput(dir);
+ }
+ if (error && (error != -EEXIST)) {
+ sysfs_put((dir)->d_fsdata);
+ d_drop(dir);
+ }
+
+ return error;
+}
+
+static int make_one_dir(struct kobject * k, struct dentry * parent,
+ char * name, struct dentry ** d)
+{
+ struct dentry * dir;
+ int error = 0;
+
+ down(&parent->d_inode->i_sem);
+
+ dir = lookup_one_len(name, parent, strlen(name));
+
+ if (!IS_ERR(dir)) {
+ /*
+ * Check if directory does or does not exist.
+ * If it does, then return that dentry.
+ * Otherwise go ahead and create it.
+ */
+ if (!dir->d_inode)
+ error = create_dir(k, parent, dir);
} else
- error = PTR_ERR(*d);
- up(&p->d_inode->i_sem);
+ error = PTR_ERR(dir);
+ up(&parent->d_inode->i_sem);
+
+ if (!error)
+ *d = dir;
+
return error;
}
+/**
+ * sysfs_create_subdir - Create one or more subdirectories in sysfs
+ * @k: kobject that owns the ancestral directory.
+ * @n: Directory (or directories) to be added.
+ * @d: The dentry of the bottom-most directory.
+ *
+ * This function creates one or more subdirectories in a kobject's
+ * sysfs directory, which can be used to add attributes for that
+ * kobject (in an organized fashion).
+ *
+ * The algorithm is simple: it scans @n for '/', replaces each
+ * occurence with a NULL character and creates a directory with the name
+ * of that resulting string. It continues until it reaches the end of @n.
+ *
+ * For example, if @k had a directory at /sys/devices/my-device/, and
+ * this function was called with @n == "foo/bar/baz", the resulting
+ * directory structure would look like:
+ *
+ * /sys/devices/my-device/
+ * `-- foo
+ * `-- bar
+ * `-- baz
+ *
+ * And the dentry to 'baz' would be passed back in @d.
+ *
+ * Note that this function and its helpers have recently been updated to
+ * recognize when a subdirectory has already been created and to return
+ * without an error. So, after the above example was finished, a caller
+ * could call this function with the same @k and @n == "foo". A new
+ * directory would not be created and the dentry for 'foo' would be
+ * returned in @d.
+ */
+
int sysfs_create_subdir(struct kobject * k, const char * n, struct dentry ** d)
{
- return create_dir(k,k->dentry,n,d);
+ struct dentry * parent = k->dentry;
+ struct dentry * dir;
+ char * str, * s;
+ char * next;
+ int ret;
+
+ s = str = kstrdup(n, GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ while ((next = strchr(str, '/'))) {
+ *next++ = '\0';
+
+ /*
+ * Check if it's the end of the string anyway
+ */
+ if (*next == '\0')
+ break;
+
+ ret = make_one_dir(k, parent, str, &dir);
+ if (ret)
+ goto Done;
+
+ str = next;
+ parent = dir;
+ }
+
+ /*
+ * Make the final directory (where the files will go).
+ */
+ ret = make_one_dir(k, parent, str, d);
+ Done:
+ kfree(s);
+ return ret;
}
/**
@@ -136,7 +225,8 @@ int sysfs_create_subdir(struct kobject *
int sysfs_create_dir(struct kobject * kobj)
{
- struct dentry * dentry = NULL;
+ const char * name = kobject_name(kobj);
+ struct dentry * dir;
struct dentry * parent;
int error = 0;
@@ -149,9 +239,16 @@ int sysfs_create_dir(struct kobject * ko
else
return -EFAULT;
- error = create_dir(kobj,parent,kobject_name(kobj),&dentry);
+ down(&parent->d_inode->i_sem);
+ dir = lookup_one_len(name, parent, strlen(name));
+ if (!IS_ERR(dir))
+ error = create_dir(kobj, parent, dir);
+ else
+ error = PTR_ERR(dir);
+ up(&parent->d_inode->i_sem);
+
if (!error)
- kobj->dentry = dentry;
+ kobj->dentry = dir;
return error;
}
On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
>
> The patch below addresses this issue by parsing the subdirectory name and
> creating any parent directories delineated by a '/'.
Generally I never liked parsing stuff like this in the kernel (proc and
devfs both do this). That being said, I do see the need to make subdirs
like this easier.
But what about cleanups? If I create an attribute group "foo/baz/x/" and
then remove it, will the subdirectories get cleaned up too? What about
if I had created a group "foo/baz/y/" after the "x" one? Or just
"foo/baz"?
thanks,
greg k-h
On Nov 28, 2005, at 15:49, Greg KH wrote:
> On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
>> The patch below addresses this issue by parsing the subdirectory
>> name and creating any parent directories delineated by a '/'.
>
> Generally I never liked parsing stuff like this in the kernel (proc
> and devfs both do this). That being said, I do see the need to
> make subdirs like this easier.
>
> But what about cleanups? If I create an attribute group "foo/baz/
> x/" and then remove it, will the subdirectories get cleaned up
> too? What about if I had created a group "foo/baz/y/" after the
> "x" one? Or just "foo/baz"?
If the kernel gets this, then udev needs to allow attributes with
more generic paths too. It would be nice if I could use this [Pulled
from the sample udev output halfway down this page: http://
http://www.reactivated.net/writing_udev_rules.html#identify-sysfs].
BUS="scsi", SYSFS{../../../manufacturer}="Tekom Technologies, Inc",
NAME="my_camera"
Frequently the attributes one wants to filter by are spread out
through the tree, especially when they've been subdivided for clarity
as people seem to want to do.
Cheers,
Kyle Moffett
--
I lost interest in "blade servers" when I found they didn't throw
knives at people who weren't supposed to be in your machine room.
-- Anthony de Boer
On Mon, Nov 28, 2005 at 08:10:36PM -0500, Kyle Moffett wrote:
> On Nov 28, 2005, at 15:49, Greg KH wrote:
> >On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
> >>The patch below addresses this issue by parsing the subdirectory
> >>name and creating any parent directories delineated by a '/'.
> >
> >Generally I never liked parsing stuff like this in the kernel (proc
> >and devfs both do this). That being said, I do see the need to
> >make subdirs like this easier.
> >
> >But what about cleanups? If I create an attribute group "foo/baz/
> >x/" and then remove it, will the subdirectories get cleaned up
> >too? What about if I had created a group "foo/baz/y/" after the
> >"x" one? Or just "foo/baz"?
>
> If the kernel gets this, then udev needs to allow attributes with
> more generic paths too. It would be nice if I could use this [Pulled
> from the sample udev output halfway down this page: http://
> http://www.reactivated.net/writing_udev_rules.html#identify-sysfs].
>
> BUS="scsi", SYSFS{../../../manufacturer}="Tekom Technologies, Inc",
> NAME="my_camera"
Why can't you do this today? Have you tried it?
thanks,
greg k-h
On Nov 29, 2005, at 00:44, Greg KH wrote:
> On Mon, Nov 28, 2005 at 08:10:36PM -0500, Kyle Moffett wrote:
>> If the kernel gets this, then udev needs to allow attributes with
>> more generic paths too. It would be nice if I could use this
>> [Pulled from the sample udev output halfway down this page: http://
>> http://www.reactivated.net/writing_udev_rules.html#identify-sysfs].
>>
>> BUS="scsi", SYSFS{../../../manufacturer}="Tekom Technologies,
>> Inc", NAME="my_camera"
>
> Why can't you do this today? Have you tried it?
Hmm, no, I don't suppose I have. I guess I was taking the udev docs
and other similar pages at their word that you couldn't match things
in multiple subdirs. *tries* Interesting; it appears to work, but
it would be nice if it was documented somewhere; this is _really_
handy for certain devices with partial or missing udev support. (I
can specify the 3rd proprietary tape drive in the changer even if the
drives themselves don't have any useful SCSI info attached to them.)
Thanks for pointing this out!
I do have one question, though. Is there any way to access the
"DEVICE" or "SUBSYSTEM" values of those parent sysfs nodes? I can
see that those are symlinks in sysfs to other sysfs dirs, so there's
no real way to match them with SYSFS{*}, but I'm hoping that perhaps
they could be extended to accept a path in {} brackets much as SYSFS
{} does. Thanks for the help! And btw, here are the rules I've
worked out for my PowerBook (I frequently tinker with the MAC address
when doing odd networking hackery, so it's not so useful for matching
purposes):
## The built-in ethernet
SUBSYSTEM="net", SYSFS{device/devspec}="/pci@f4000000/ethernet@f",
NAME="net_eth"
## The built-in firewire (via eth1394)
SUBSYSTEM="net", SYSFS{device/../devspec}="/pci@f4000000/firewire@e",
NAME="net_fw"
Cheers,
Kyle Moffett
--
Unix was not designed to stop people from doing stupid things,
because that would also stop them from doing clever things.
-- Doug Gwyn
On Tue, Nov 29, 2005 at 01:41:01AM -0500, Kyle Moffett wrote:
> On Nov 29, 2005, at 00:44, Greg KH wrote:
> >On Mon, Nov 28, 2005 at 08:10:36PM -0500, Kyle Moffett wrote:
> >>If the kernel gets this, then udev needs to allow attributes with
> >>more generic paths too. It would be nice if I could use this
> >>[Pulled from the sample udev output halfway down this page: http://
> >>http://www.reactivated.net/writing_udev_rules.html#identify-sysfs].
> >>
> >>BUS="scsi", SYSFS{../../../manufacturer}="Tekom Technologies,
> >>Inc", NAME="my_camera"
> >
> >Why can't you do this today? Have you tried it?
>
> Hmm, no, I don't suppose I have. I guess I was taking the udev docs
> and other similar pages at their word that you couldn't match things
> in multiple subdirs. *tries* Interesting; it appears to work, but
> it would be nice if it was documented somewhere; this is _really_
> handy for certain devices with partial or missing udev support.
Patches for documentation is always gladly accepted :)
> I do have one question, though. Is there any way to access the
> "DEVICE" or "SUBSYSTEM" values of those parent sysfs nodes? I can
> see that those are symlinks in sysfs to other sysfs dirs, so there's
> no real way to match them with SYSFS{*},
As they are symlinks, you might just be able to follow them as part of
the path. But odds are, something in libsysfs would prevent that from
working that way...
thanks,
greg k-h
On Mon, 28 Nov 2005, Greg KH wrote:
> On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
> >
> > The patch below addresses this issue by parsing the subdirectory name and
> > creating any parent directories delineated by a '/'.
>
> Generally I never liked parsing stuff like this in the kernel (proc and
> devfs both do this). That being said, I do see the need to make subdirs
> like this easier.
Heh, just because proc and devfs did it doesn't mean it's inherently
evil..
> But what about cleanups? If I create an attribute group "foo/baz/x/" and
> then remove it, will the subdirectories get cleaned up too? What about
> if I had created a group "foo/baz/y/" after the "x" one? Or just
> "foo/baz"?
The patch I sent previously did not include a way to cleanup the
subdirectories, but it's pretty straightforward and covered by this patch.
Basically, it adds a new refcount to struct sysfs_dirent (->s_refs) that
is incremented when a subdirectory is created and decremented when the
subdirectory is removed. When it reaches 0, that directory itself can be
removed.
Note that it's a bit hacky in sysfs_remove_group(), since we need the
bottom-most dentry a priori. I'm not sure the best way to do this off the
top of my head, and ideas?
If you're interested, I will break it up into 2-3 patches for application
(I'd like to also move the sysfs_dirent declaration into fs/sysfs/sysfs.h,
since they're really private and so that further modification of the
declaration doesn't preclude a nearly-full recompile of the tree).
Thanks,
Pat
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 59734ba..b6bf33d 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -42,6 +42,7 @@ static struct sysfs_dirent * sysfs_new_d
return NULL;
memset(sd, 0, sizeof(*sd));
+ atomic_set(&sd->s_refs, 1);
atomic_set(&sd->s_count, 1);
INIT_LIST_HEAD(&sd->s_children);
list_add(&sd->s_sibling, &parent_sd->s_children);
@@ -70,6 +71,7 @@ int sysfs_make_dirent(struct sysfs_diren
return 0;
}
+
static int init_dir(struct inode * inode)
{
inode->i_op = &sysfs_dir_inode_operations;
@@ -94,38 +96,134 @@ static int init_symlink(struct inode * i
}
static int create_dir(struct kobject * k, struct dentry * p,
- const char * n, struct dentry ** d)
+ struct dentry * dir)
{
int error;
umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
- down(&p->d_inode->i_sem);
- *d = lookup_one_len(n, p, strlen(n));
- if (!IS_ERR(*d)) {
- error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
+ error = sysfs_make_dirent(p->d_fsdata, dir, k, mode, SYSFS_DIR);
+ if (!error) {
+ error = sysfs_create(dir, mode, init_dir);
if (!error) {
- error = sysfs_create(*d, mode, init_dir);
- if (!error) {
- p->d_inode->i_nlink++;
- (*d)->d_op = &sysfs_dentry_ops;
- d_rehash(*d);
- }
+ p->d_inode->i_nlink++;
+ dir->d_op = &sysfs_dentry_ops;
+ d_rehash(dir);
}
- if (error && (error != -EEXIST)) {
- sysfs_put((*d)->d_fsdata);
- d_drop(*d);
+ dput(dir);
+ }
+ if (error && (error != -EEXIST)) {
+ sysfs_put((dir)->d_fsdata);
+ d_drop(dir);
+ }
+
+ return error;
+}
+
+static int make_one_dir(struct kobject * k, struct dentry * parent,
+ char * name, struct dentry ** d)
+{
+ struct sysfs_dirent * parent_sd = parent->d_fsdata;
+ struct dentry * dir;
+ int error = 0;
+
+ down(&parent->d_inode->i_sem);
+
+ dir = lookup_one_len(name, parent, strlen(name));
+
+ if (!IS_ERR(dir)) {
+ /*
+ * Check if directory does or does not exist.
+ * If it does, add a ref to the dirent and return that dentry.
+ * Otherwise go ahead and create it.
+ */
+ if (!dir->d_inode)
+ error = create_dir(k, parent, dir);
+ else {
+ struct sysfs_dirent * sd = dir->d_fsdata;
+ atomic_inc(&sd->s_refs);
}
- dput(*d);
} else
- error = PTR_ERR(*d);
- up(&p->d_inode->i_sem);
+ error = PTR_ERR(dir);
+
+ atomic_inc(&parent_sd->s_refs);
+ up(&parent->d_inode->i_sem);
+
+ if (!error)
+ *d = dir;
+
return error;
}
+/**
+ * sysfs_create_subdir - Create one or more subdirectories in sysfs
+ * @k: kobject that owns the ancestral directory.
+ * @n: Directory (or directories) to be added.
+ * @d: The dentry of the bottom-most directory.
+ *
+ * This function creates one or more subdirectories in a kobject's
+ * sysfs directory, which can be used to add attributes for that
+ * kobject (in an organized fashion).
+ *
+ * The algorithm is simple: it scans @n for '/', replaces each
+ * occurence with a NULL character and creates a directory with the name
+ * of that resulting string. It continues until it reaches the end of @n.
+ *
+ * For example, if @k had a directory at /sys/devices/my-device/, and
+ * this function was called with @n == "foo/bar/baz", the resulting
+ * directory structure would look like:
+ *
+ * /sys/devices/my-device/
+ * `-- foo
+ * `-- bar
+ * `-- baz
+ *
+ * And the dentry to 'baz' would be passed back in @d.
+ *
+ * Note that this function and its helpers have recently been updated to
+ * recognize when a subdirectory has already been created and to return
+ * without an error. So, after the above example was finished, a caller
+ * could call this function with the same @k and @n == "foo". A new
+ * directory would not be created and the dentry for 'foo' would be
+ * returned in @d.
+ */
+
int sysfs_create_subdir(struct kobject * k, const char * n, struct dentry ** d)
{
- return create_dir(k,k->dentry,n,d);
+ struct dentry * parent = k->dentry;
+ struct dentry * dir;
+ char * str, * s;
+ char * next;
+ int ret;
+
+ s = str = kstrdup(n, GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ while ((next = strchr(str, '/'))) {
+ *next++ = '\0';
+
+ /*
+ * Check if it's the end of the string anyway
+ */
+ if (*next == '\0')
+ break;
+
+ ret = make_one_dir(k, parent, str, &dir);
+ if (ret)
+ goto Done;
+
+ str = next;
+ parent = dir;
+ }
+
+ /*
+ * Make the final directory (where the files will go).
+ */
+ ret = make_one_dir(k, parent, str, d);
+ Done:
+ kfree(s);
+ return ret;
}
/**
@@ -136,7 +234,8 @@ int sysfs_create_subdir(struct kobject *
int sysfs_create_dir(struct kobject * kobj)
{
- struct dentry * dentry = NULL;
+ const char * name = kobject_name(kobj);
+ struct dentry * dir;
struct dentry * parent;
int error = 0;
@@ -149,9 +248,16 @@ int sysfs_create_dir(struct kobject * ko
else
return -EFAULT;
- error = create_dir(kobj,parent,kobject_name(kobj),&dentry);
+ down(&parent->d_inode->i_sem);
+ dir = lookup_one_len(name, parent, strlen(name));
+ if (!IS_ERR(dir))
+ error = create_dir(kobj, parent, dir);
+ else
+ error = PTR_ERR(dir);
+ up(&parent->d_inode->i_sem);
+
if (!error)
- kobj->dentry = dentry;
+ kobj->dentry = dir;
return error;
}
@@ -257,9 +363,58 @@ static void remove_dir(struct dentry * d
dput(parent);
}
-void sysfs_remove_subdir(struct dentry * d)
+static void remove_one_subdir(struct kobject * k, struct dentry * parent, char * name)
{
- remove_dir(d);
+ struct sysfs_dirent * sd;
+ struct dentry * dir;
+
+ dir = lookup_one_len(name, parent, strlen(name));
+
+ do_parent:
+ if (!IS_ERR(dir)) {
+ sd = dir->d_fsdata;
+
+ if (atomic_dec_and_test(&sd->s_refs)) {
+ remove_dir(dir);
+
+ dir = dir->d_parent;
+ goto do_parent;
+ }
+ }
+}
+
+
+void sysfs_remove_subdir(struct kobject * k, const char * n)
+{
+ struct dentry * parent = k->dentry;
+ struct dentry * dir;
+ char * str, * s;
+ char * next;
+
+ s = str = kstrdup(n, GFP_KERNEL);
+ if (!s)
+ return;
+
+ while ((next = strchr(str, '/'))) {
+ *next++ = '\0';
+
+ /*
+ * Check if it's the end of the string anyway
+ */
+ if (*next == '\0')
+ break;
+
+ remove_one_subdir(k, parent, str);
+
+ str = next;
+ parent = dir;
+ }
+
+ /*
+ * Remove the final directory (where the files were).
+ */
+ remove_one_subdir(k, parent, str);
+ kfree(s);
}
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 122145b..c998c74 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -57,7 +57,7 @@ int sysfs_create_group(struct kobject *
dir = dget(dir);
if ((error = create_files(dir,grp))) {
if (grp->name)
- sysfs_remove_subdir(dir);
+ sysfs_remove_subdir(kobj, grp->name);
}
dput(dir);
return error;
@@ -68,15 +68,37 @@ void sysfs_remove_group(struct kobject *
{
struct dentry * dir;
- if (grp->name)
- dir = lookup_one_len(grp->name, kobj->dentry,
- strlen(grp->name));
- else
+ if (grp->name) {
+ struct dentry * parent = kobj->dentry;
+ char * str, * s;
+ char * next;
+ s = str = kstrdup(grp->name, GFP_KERNEL);
+
+ if (!s)
+ return;
+
+ while ((next = strchr(str, '/'))) {
+ *next++ = '\0';
+
+ /*
+ * Check if it's the end of the string anyway
+ */
+ if (*next == '\0')
+ break;
+
+ dir = lookup_one_len(str, parent, strlen(str));
+
+ str = next;
+ parent = dir;
+ }
+ dir = lookup_one_len(str, parent, strlen(str));
+ kfree(s);
+ } else
dir = dget(kobj->dentry);
- remove_files(dir,grp);
+ remove_files(dir, grp);
if (grp->name)
- sysfs_remove_subdir(dir);
+ sysfs_remove_subdir(kobj, grp->name);
/* release the ref. taken in this routine */
dput(dir);
}
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 3f8953e..72f7dc2 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -12,7 +12,7 @@ extern int sysfs_add_file(struct dentry
extern void sysfs_hash_and_remove(struct dentry * dir, const char * name);
extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
-extern void sysfs_remove_subdir(struct dentry *);
+extern void sysfs_remove_subdir(struct kobject *, const char *);
extern const unsigned char * sysfs_get_name(struct sysfs_dirent *sd);
extern void sysfs_drop_dentry(struct sysfs_dirent *sd, struct dentry *parent);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 392da5a..86c4264 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -66,6 +66,7 @@ struct sysfs_ops {
};
struct sysfs_dirent {
+ atomic_t s_refs;
atomic_t s_count;
struct list_head s_sibling;
struct list_head s_children;
On Wed, Nov 30, 2005 at 09:05:41AM -0800, Patrick Mochel wrote:
>
> On Mon, 28 Nov 2005, Greg KH wrote:
>
> > On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
> > >
> > > The patch below addresses this issue by parsing the subdirectory name and
> > > creating any parent directories delineated by a '/'.
> >
> > Generally I never liked parsing stuff like this in the kernel (proc and
> > devfs both do this). That being said, I do see the need to make subdirs
> > like this easier.
>
> Heh, just because proc and devfs did it doesn't mean it's inherently
> evil..
I did not mean to imply that, just that putting parsers like this spread
around the kernel in different places isn't the best thing to have.
> > But what about cleanups? If I create an attribute group "foo/baz/x/" and
> > then remove it, will the subdirectories get cleaned up too? What about
> > if I had created a group "foo/baz/y/" after the "x" one? Or just
> > "foo/baz"?
>
> The patch I sent previously did not include a way to cleanup the
> subdirectories, but it's pretty straightforward and covered by this patch.
> Basically, it adds a new refcount to struct sysfs_dirent (->s_refs) that
> is incremented when a subdirectory is created and decremented when the
> subdirectory is removed. When it reaches 0, that directory itself can be
> removed.
>
> Note that it's a bit hacky in sysfs_remove_group(), since we need the
> bottom-most dentry a priori. I'm not sure the best way to do this off the
> top of my head, and ideas?
Don't know, I'd ask Maneesh, as he knows this code quite well.
> If you're interested, I will break it up into 2-3 patches for application
> (I'd like to also move the sysfs_dirent declaration into fs/sysfs/sysfs.h,
> since they're really private and so that further modification of the
> declaration doesn't preclude a nearly-full recompile of the tree).
Thanks, breaking it up into logicial pieces is a good idea so we can see
what is happening easier.
greg k-h
(cc-ing Al for his views)
On Wed, Nov 30, 2005 at 09:05:41AM -0800, Patrick Mochel wrote:
>
> On Mon, 28 Nov 2005, Greg KH wrote:
>
> > On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
> > >
> > > The patch below addresses this issue by parsing the subdirectory name and
> > > creating any parent directories delineated by a '/'.
> >
well, creating directory hierarchy for attributes in one shot could be
useful in some cases, but is it worth putting more fuel to race conditions
in sysfs? I am afraid that there could be extra efforts also needed for
userspace to manage the namespace collisions in sysfs.
Nested attributes will not be straight forward. With this scheme do all
attributes in the attrbiute tree belong to same kobject?
> > Generally I never liked parsing stuff like this in the kernel (proc and
> > devfs both do this). That being said, I do see the need to make subdirs
> > like this easier.
>
> Heh, just because proc and devfs did it doesn't mean it's inherently
> evil..
>
I don't like the current approach and agree with Greg about the path walking
code. IMO, leave the path walking for VFS to handle. The same goal of
creating the directory hierarchy for a given path name can be achieved
using __user_walk(). Though it will introduce new purpose of path walking
but it will save us from everybody writing path walking code. As of now
the __user_walk() gets to the bottom most path component and returns the
inode. With a new lookup flag, lets say LOOKUP_CREATE_DIR, it could
allow ->lookup() to keep creating direcotries for each path component walked.
> > But what about cleanups? If I create an attribute group "foo/baz/x/" and
> > then remove it, will the subdirectories get cleaned up too? What about
> > if I had created a group "foo/baz/y/" after the "x" one? Or just
> > "foo/baz"?
>
> The patch I sent previously did not include a way to cleanup the
> subdirectories, but it's pretty straightforward and covered by this patch.
> Basically, it adds a new refcount to struct sysfs_dirent (->s_refs) that
> is incremented when a subdirectory is created and decremented when the
> subdirectory is removed. When it reaches 0, that directory itself can be
> removed.
>
> Note that it's a bit hacky in sysfs_remove_group(), since we need the
> bottom-most dentry a priori.
well, it is buggy and racy. The patch is obviously untested. I don't
think a separate ref count is needed here. There are already two ref
counts (one with dentry, and one with sysfs_dirent).
> I'm not sure the best way to do this off the
> top of my head, and ideas?
>
I am not clear, what are the semantics being decided for removal. I mean
whether remove the last component, or remove whatever is possible?
> (I'd like to also move the sysfs_dirent declaration into fs/sysfs/sysfs.h,
> since they're really private and so that further modification of the
> declaration doesn't preclude a nearly-full recompile of the tree).
>
ok.. please move the SYSFS_ defines also.
Thanks
Maneesh
> Thanks,
>
>
> Pat
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 59734ba..b6bf33d 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -42,6 +42,7 @@ static struct sysfs_dirent * sysfs_new_d
> return NULL;
>
> memset(sd, 0, sizeof(*sd));
> + atomic_set(&sd->s_refs, 1);
> atomic_set(&sd->s_count, 1);
> INIT_LIST_HEAD(&sd->s_children);
> list_add(&sd->s_sibling, &parent_sd->s_children);
> @@ -70,6 +71,7 @@ int sysfs_make_dirent(struct sysfs_diren
> return 0;
> }
>
> +
> static int init_dir(struct inode * inode)
> {
> inode->i_op = &sysfs_dir_inode_operations;
> @@ -94,38 +96,134 @@ static int init_symlink(struct inode * i
> }
>
> static int create_dir(struct kobject * k, struct dentry * p,
> - const char * n, struct dentry ** d)
> + struct dentry * dir)
> {
> int error;
> umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
>
> - down(&p->d_inode->i_sem);
> - *d = lookup_one_len(n, p, strlen(n));
> - if (!IS_ERR(*d)) {
> - error = sysfs_make_dirent(p->d_fsdata, *d, k, mode, SYSFS_DIR);
> + error = sysfs_make_dirent(p->d_fsdata, dir, k, mode, SYSFS_DIR);
> + if (!error) {
> + error = sysfs_create(dir, mode, init_dir);
> if (!error) {
> - error = sysfs_create(*d, mode, init_dir);
> - if (!error) {
> - p->d_inode->i_nlink++;
> - (*d)->d_op = &sysfs_dentry_ops;
> - d_rehash(*d);
> - }
> + p->d_inode->i_nlink++;
> + dir->d_op = &sysfs_dentry_ops;
> + d_rehash(dir);
> }
> - if (error && (error != -EEXIST)) {
> - sysfs_put((*d)->d_fsdata);
> - d_drop(*d);
> + dput(dir);
> + }
> + if (error && (error != -EEXIST)) {
> + sysfs_put((dir)->d_fsdata);
> + d_drop(dir);
> + }
> +
> + return error;
> +}
> +
> +static int make_one_dir(struct kobject * k, struct dentry * parent,
> + char * name, struct dentry ** d)
> +{
> + struct sysfs_dirent * parent_sd = parent->d_fsdata;
> + struct dentry * dir;
> + int error = 0;
> +
> + down(&parent->d_inode->i_sem);
> +
> + dir = lookup_one_len(name, parent, strlen(name));
> +
> + if (!IS_ERR(dir)) {
> + /*
> + * Check if directory does or does not exist.
> + * If it does, add a ref to the dirent and return that dentry.
> + * Otherwise go ahead and create it.
> + */
> + if (!dir->d_inode)
> + error = create_dir(k, parent, dir);
> + else {
> + struct sysfs_dirent * sd = dir->d_fsdata;
> + atomic_inc(&sd->s_refs);
> }
> - dput(*d);
> } else
> - error = PTR_ERR(*d);
> - up(&p->d_inode->i_sem);
> + error = PTR_ERR(dir);
> +
> + atomic_inc(&parent_sd->s_refs);
> + up(&parent->d_inode->i_sem);
> +
> + if (!error)
> + *d = dir;
> +
> return error;
> }
>
>
> +/**
> + * sysfs_create_subdir - Create one or more subdirectories in sysfs
> + * @k: kobject that owns the ancestral directory.
> + * @n: Directory (or directories) to be added.
> + * @d: The dentry of the bottom-most directory.
> + *
> + * This function creates one or more subdirectories in a kobject's
> + * sysfs directory, which can be used to add attributes for that
> + * kobject (in an organized fashion).
> + *
> + * The algorithm is simple: it scans @n for '/', replaces each
> + * occurence with a NULL character and creates a directory with the name
> + * of that resulting string. It continues until it reaches the end of @n.
> + *
> + * For example, if @k had a directory at /sys/devices/my-device/, and
> + * this function was called with @n == "foo/bar/baz", the resulting
> + * directory structure would look like:
> + *
> + * /sys/devices/my-device/
> + * `-- foo
> + * `-- bar
> + * `-- baz
> + *
> + * And the dentry to 'baz' would be passed back in @d.
> + *
> + * Note that this function and its helpers have recently been updated to
> + * recognize when a subdirectory has already been created and to return
> + * without an error. So, after the above example was finished, a caller
> + * could call this function with the same @k and @n == "foo". A new
> + * directory would not be created and the dentry for 'foo' would be
> + * returned in @d.
> + */
> +
> int sysfs_create_subdir(struct kobject * k, const char * n, struct dentry ** d)
> {
> - return create_dir(k,k->dentry,n,d);
> + struct dentry * parent = k->dentry;
> + struct dentry * dir;
> + char * str, * s;
> + char * next;
> + int ret;
> +
> + s = str = kstrdup(n, GFP_KERNEL);
> + if (!s)
> + return -ENOMEM;
> +
> + while ((next = strchr(str, '/'))) {
> + *next++ = '\0';
> +
> + /*
> + * Check if it's the end of the string anyway
> + */
> + if (*next == '\0')
> + break;
> +
> + ret = make_one_dir(k, parent, str, &dir);
> + if (ret)
> + goto Done;
> +
> + str = next;
> + parent = dir;
> + }
> +
> + /*
> + * Make the final directory (where the files will go).
> + */
> + ret = make_one_dir(k, parent, str, d);
> + Done:
> + kfree(s);
> + return ret;
> }
>
> /**
> @@ -136,7 +234,8 @@ int sysfs_create_subdir(struct kobject *
>
> int sysfs_create_dir(struct kobject * kobj)
> {
> - struct dentry * dentry = NULL;
> + const char * name = kobject_name(kobj);
> + struct dentry * dir;
> struct dentry * parent;
> int error = 0;
>
> @@ -149,9 +248,16 @@ int sysfs_create_dir(struct kobject * ko
> else
> return -EFAULT;
>
> - error = create_dir(kobj,parent,kobject_name(kobj),&dentry);
> + down(&parent->d_inode->i_sem);
> + dir = lookup_one_len(name, parent, strlen(name));
> + if (!IS_ERR(dir))
> + error = create_dir(kobj, parent, dir);
> + else
> + error = PTR_ERR(dir);
> + up(&parent->d_inode->i_sem);
> +
> if (!error)
> - kobj->dentry = dentry;
> + kobj->dentry = dir;
> return error;
> }
>
> @@ -257,9 +363,58 @@ static void remove_dir(struct dentry * d
> dput(parent);
> }
>
> -void sysfs_remove_subdir(struct dentry * d)
> +static void remove_one_subdir(struct kobject * k, struct dentry * parent, char * name)
> {
> - remove_dir(d);
> + struct sysfs_dirent * sd;
> + struct dentry * dir;
> +
> + dir = lookup_one_len(name, parent, strlen(name));
> +
> + do_parent:
> + if (!IS_ERR(dir)) {
> + sd = dir->d_fsdata;
> +
> + if (atomic_dec_and_test(&sd->s_refs)) {
> + remove_dir(dir);
> +
> + dir = dir->d_parent;
> + goto do_parent;
> + }
> + }
> +}
> +
> +
> +void sysfs_remove_subdir(struct kobject * k, const char * n)
> +{
> + struct dentry * parent = k->dentry;
> + struct dentry * dir;
> + char * str, * s;
> + char * next;
> +
> + s = str = kstrdup(n, GFP_KERNEL);
> + if (!s)
> + return;
> +
> + while ((next = strchr(str, '/'))) {
> + *next++ = '\0';
> +
> + /*
> + * Check if it's the end of the string anyway
> + */
> + if (*next == '\0')
> + break;
> +
> + remove_one_subdir(k, parent, str);
> +
> + str = next;
> + parent = dir;
> + }
> +
> + /*
> + * Remove the final directory (where the files were).
> + */
> + remove_one_subdir(k, parent, str);
> + kfree(s);
> }
>
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 122145b..c998c74 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -57,7 +57,7 @@ int sysfs_create_group(struct kobject *
> dir = dget(dir);
> if ((error = create_files(dir,grp))) {
> if (grp->name)
> - sysfs_remove_subdir(dir);
> + sysfs_remove_subdir(kobj, grp->name);
> }
> dput(dir);
> return error;
> @@ -68,15 +68,37 @@ void sysfs_remove_group(struct kobject *
> {
> struct dentry * dir;
>
> - if (grp->name)
> - dir = lookup_one_len(grp->name, kobj->dentry,
> - strlen(grp->name));
> - else
> + if (grp->name) {
> + struct dentry * parent = kobj->dentry;
> + char * str, * s;
> + char * next;
> + s = str = kstrdup(grp->name, GFP_KERNEL);
> +
> + if (!s)
> + return;
> +
> + while ((next = strchr(str, '/'))) {
> + *next++ = '\0';
> +
> + /*
> + * Check if it's the end of the string anyway
> + */
> + if (*next == '\0')
> + break;
> +
> + dir = lookup_one_len(str, parent, strlen(str));
> +
> + str = next;
> + parent = dir;
> + }
> + dir = lookup_one_len(str, parent, strlen(str));
> + kfree(s);
> + } else
> dir = dget(kobj->dentry);
>
> - remove_files(dir,grp);
> + remove_files(dir, grp);
> if (grp->name)
> - sysfs_remove_subdir(dir);
> + sysfs_remove_subdir(kobj, grp->name);
> /* release the ref. taken in this routine */
> dput(dir);
> }
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index 3f8953e..72f7dc2 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -12,7 +12,7 @@ extern int sysfs_add_file(struct dentry
> extern void sysfs_hash_and_remove(struct dentry * dir, const char * name);
>
> extern int sysfs_create_subdir(struct kobject *, const char *, struct dentry **);
> -extern void sysfs_remove_subdir(struct dentry *);
> +extern void sysfs_remove_subdir(struct kobject *, const char *);
>
> extern const unsigned char * sysfs_get_name(struct sysfs_dirent *sd);
> extern void sysfs_drop_dentry(struct sysfs_dirent *sd, struct dentry *parent);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 392da5a..86c4264 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -66,6 +66,7 @@ struct sysfs_ops {
> };
>
> struct sysfs_dirent {
> + atomic_t s_refs;
> atomic_t s_count;
> struct list_head s_sibling;
> struct list_head s_children;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Maneesh Soni
Linux Technology Center,
IBM India Software Labs,
Bangalore, India
email: [email protected]
Phone: 91-80-25044990
On Mon, 5 Dec 2005, Maneesh Soni wrote:
>
> (cc-ing Al for his views)
>
> On Wed, Nov 30, 2005 at 09:05:41AM -0800, Patrick Mochel wrote:
> >
> > On Mon, 28 Nov 2005, Greg KH wrote:
> >
> > > On Wed, Nov 23, 2005 at 01:56:29PM -0800, Patrick Mochel wrote:
> > > >
> > > > The patch below addresses this issue by parsing the subdirectory name and
> > > > creating any parent directories delineated by a '/'.
> > >
>
> well, creating directory hierarchy for attributes in one shot could be
> useful in some cases, but is it worth putting more fuel to race conditions
> in sysfs? I am afraid that there could be extra efforts also needed for
> userspace to manage the namespace collisions in sysfs.
Yeah, after more thought, it doesn't seem necessary, nor worth it to spend
the time ironing out all of the details.
> Nested attributes will not be straight forward. With this scheme do all
> attributes in the attrbiute tree belong to same kobject?
Yes, they would all belong to the same kobject, which means they would all
have to go away when the kobject did, which could get hairy, especially
when kobjects are added/removed in rapid succession.
I think the best approach is to scrap this idea and to leverage other
existing infrastructure better..
Thanks for the commentary, and sorry about the delay in responding,
Patrick