In the SCSI transport classes (and soon to be in the AEN event
subsystem) we have a lot of need for a grouping that doesn't include all
files in the group. We basically want to show capability by which file
is present. A classic example of this is the SPI transport class
connected to the 53c700 card. It's incapable of doing all of the modern
LVD functions, so we don't show any of those capabilities in its sysfs
directory. However, we have a lot of horrible logic to generate
separate per host groupings of attributes for this. We would be able to
use the standard sysfs group attributes *if* there were a way of
filtering them so that certain attributes didn't appear.
This patch is a first pass at adding a filter function to the group
attributes, just to see how the idea flies. If everyone's OK with this,
I think the next thing that we might do is add bitmap functions (so
every bit in the bitmap has a name, but also might not appear) to
groups.
The kzalloc in kernel/params.c is to stop the system crashing on boot
with a bogus filter function pointer.
Signed-off-by: James Bottomley <[email protected]>
----
There's one final thing; if you're OK with this, can you ack it and let
me take it through the SCSI tree? we're building functionality that
will depend on this.
Thanks,
James
Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c 2007-10-29 00:55:49.000000000 -0500
@@ -16,25 +16,31 @@
#include "sysfs.h"
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
+ int i;
- for (attr = grp->attrs; *attr; attr++)
- sysfs_hash_and_remove(dir_sd, (*attr)->name);
+ for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+ if (grp->filter_show &&
+ grp->filter_show(kobj, i))
+ sysfs_hash_and_remove(dir_sd, (*attr)->name);
}
-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int error = 0;
+ int error = 0, i;
- for (attr = grp->attrs; *attr && !error; attr++)
- error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+ if (grp->filter_show &&
+ grp->filter_show(kobj, i))
+ error |=
+ sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error)
- remove_files(dir_sd, grp);
+ remove_files(dir_sd, kobj, grp);
return error;
}
@@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject *
} else
sd = kobj->sd;
sysfs_get(sd);
- error = create_files(sd, grp);
+ error = create_files(sd, kobj, grp);
if (error) {
if (grp->name)
sysfs_remove_subdir(sd);
@@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
} else
sd = sysfs_get(dir_sd);
- remove_files(sd, grp);
+ remove_files(sd, kobj, grp);
if (grp->name)
sysfs_remove_subdir(sd);
Index: BUILD-2.6/include/linux/sysfs.h
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h 2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h 2007-10-29 00:37:18.000000000 -0500
@@ -32,6 +32,7 @@ struct attribute {
struct attribute_group {
const char *name;
+ int (*filter_show)(struct kobject *, int);
struct attribute **attrs;
};
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c 2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c 2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject
sizeof(mp->grp.attrs[0]));
size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
- mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+ mp = kzalloc(size[0] + size[1], GFP_KERNEL);
if (!mp)
return ERR_PTR(-ENOMEM);
On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> In the SCSI transport classes (and soon to be in the AEN event
> subsystem) we have a lot of need for a grouping that doesn't include all
> files in the group. We basically want to show capability by which file
> is present. A classic example of this is the SPI transport class
> connected to the 53c700 card. It's incapable of doing all of the modern
> LVD functions, so we don't show any of those capabilities in its sysfs
> directory. However, we have a lot of horrible logic to generate
> separate per host groupings of attributes for this. We would be able to
> use the standard sysfs group attributes *if* there were a way of
> filtering them so that certain attributes didn't appear.
Sounds fine.
> This patch is a first pass at adding a filter function to the group
> attributes, just to see how the idea flies. If everyone's OK with this,
> I think the next thing that we might do is add bitmap functions (so
> every bit in the bitmap has a name, but also might not appear) to
> groups.
Bitmaps in the attribute groups?
> struct attribute_group {
> const char *name;
> + int (*filter_show)(struct kobject *, int);
Are you sure that you want to return an array index here, instead of the
actual attribute? Like:
int (*filter_show)(struct kobject *kobj, struct attribute *attr);
The names "show" and "store" are the ususal file-operation names, and we
are not filtering a "show" here, right? Maybe "create", or "export", or
something else might be a better name?
Thanks,
Kay
On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > In the SCSI transport classes (and soon to be in the AEN event
> > subsystem) we have a lot of need for a grouping that doesn't include all
> > files in the group. We basically want to show capability by which file
> > is present. A classic example of this is the SPI transport class
> > connected to the 53c700 card. It's incapable of doing all of the modern
> > LVD functions, so we don't show any of those capabilities in its sysfs
> > directory. However, we have a lot of horrible logic to generate
> > separate per host groupings of attributes for this. We would be able to
> > use the standard sysfs group attributes *if* there were a way of
> > filtering them so that certain attributes didn't appear.
>
> Sounds fine.
>
> > This patch is a first pass at adding a filter function to the group
> > attributes, just to see how the idea flies. If everyone's OK with this,
> > I think the next thing that we might do is add bitmap functions (so
> > every bit in the bitmap has a name, but also might not appear) to
> > groups.
>
> Bitmaps in the attribute groups?
Actually, no ... that would spoil our one group for all devices rule.
So they would be a set of helper functions for manipulating bitmaps, but
the bitmap would have to be in separate storage elsewhere.
> > struct attribute_group {
> > const char *name;
> > + int (*filter_show)(struct kobject *, int);
>
> Are you sure that you want to return an array index here, instead of the
> actual attribute? Like:
Actually, it returns a true/false value indicating whether the given
attribute should be displayed.
> int (*filter_show)(struct kobject *kobj, struct attribute *attr);
>
> The names "show" and "store" are the ususal file-operation names, and we
> are not filtering a "show" here, right? Maybe "create", or "export", or
> something else might be a better name?
how about (*attribute_is_visible)?
James
On Mon, 29 Oct 2007 11:57:51 -0500,
James Bottomley <[email protected]> wrote:
> On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > > This patch is a first pass at adding a filter function to the group
> > > attributes, just to see how the idea flies. If everyone's OK with this,
> > > I think the next thing that we might do is add bitmap functions (so
> > > every bit in the bitmap has a name, but also might not appear) to
> > > groups.
> >
> > Bitmaps in the attribute groups?
>
> Actually, no ... that would spoil our one group for all devices rule.
> So they would be a set of helper functions for manipulating bitmaps, but
> the bitmap would have to be in separate storage elsewhere.
Can you determine which subset of the attributes you want just before
actually creating the group? Then you could do something like:
create_group(grp, kobj)
{
grp->update_creation_mask(kobj);
actually_create_attrs();
}
>
> > > struct attribute_group {
> > > const char *name;
> > > + int (*filter_show)(struct kobject *, int);
> >
> > Are you sure that you want to return an array index here, instead of the
> > actual attribute? Like:
>
> Actually, it returns a true/false value indicating whether the given
> attribute should be displayed.
>
> > int (*filter_show)(struct kobject *kobj, struct attribute *attr);
I'd agree that using the attribute is better in this function.
> >
> > The names "show" and "store" are the ususal file-operation names, and we
> > are not filtering a "show" here, right? Maybe "create", or "export", or
> > something else might be a better name?
>
> how about (*attribute_is_visible)?
Well, you don't only stop the visibility, but the creation of the
attribute, so perhaps (*creation_filter)?
On Mon, 2007-10-29 at 18:18 +0100, Cornelia Huck wrote:
> On Mon, 29 Oct 2007 11:57:51 -0500,
> James Bottomley <[email protected]> wrote:
>
> > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > > > This patch is a first pass at adding a filter function to the group
> > > > attributes, just to see how the idea flies. If everyone's OK with this,
> > > > I think the next thing that we might do is add bitmap functions (so
> > > > every bit in the bitmap has a name, but also might not appear) to
> > > > groups.
> > >
> > > Bitmaps in the attribute groups?
> >
> > Actually, no ... that would spoil our one group for all devices rule.
> > So they would be a set of helper functions for manipulating bitmaps, but
> > the bitmap would have to be in separate storage elsewhere.
>
> Can you determine which subset of the attributes you want just before
> actually creating the group? Then you could do something like:
>
> create_group(grp, kobj)
> {
> grp->update_creation_mask(kobj);
> actually_create_attrs();
> }
That's actually what we currently do (at least in hand coded form) in
the current transport classes. However, it leads to one separate group
for each attached class. With the filter approach, we only need one
constructed group for every transport class.
> >
> > > > struct attribute_group {
> > > > const char *name;
> > > > + int (*filter_show)(struct kobject *, int);
> > >
> > > Are you sure that you want to return an array index here, instead of the
> > > actual attribute? Like:
> >
> > Actually, it returns a true/false value indicating whether the given
> > attribute should be displayed.
> >
> > > int (*filter_show)(struct kobject *kobj, struct attribute *attr);
>
> I'd agree that using the attribute is better in this function.
>
> > >
> > > The names "show" and "store" are the ususal file-operation names, and we
> > > are not filtering a "show" here, right? Maybe "create", or "export", or
> > > something else might be a better name?
> >
> > how about (*attribute_is_visible)?
>
> Well, you don't only stop the visibility, but the creation of the
> attribute, so perhaps (*creation_filter)?
visibility and creation are the same thing, aren't they? An invisible
attribute doesn't appear in the sysfs directory, so it's equivalent to
the file for it not being created.
James
On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> > > struct attribute_group {
> > > const char *name;
> > > + int (*filter_show)(struct kobject *, int);
> >
> > Are you sure that you want to return an array index here, instead of the
> > actual attribute? Like:
>
> Actually, it returns a true/false value indicating whether the given
> attribute should be displayed.
It isn't about the return value of the function, that's fine. You call
back with the index number (int) of the array of attributes, instead of
passing the attribute pointer (struct attribute *attr) back to ask the
device for the attribute to create.
Kay
James Bottomley wrote:
> visibility and creation are the same thing, aren't they? An invisible
> attribute doesn't appear in the sysfs directory, so it's equivalent to
> the file for it not being created.
What about the case where it's visible at creation time, but then needs
to be made selectively invisible later on?
That implies either a remove operation or dentry checks on each lookup?
Jeff
On Mon, 2007-10-29 at 18:27 +0100, Kay Sievers wrote:
> On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
>
> > > > struct attribute_group {
> > > > const char *name;
> > > > + int (*filter_show)(struct kobject *, int);
> > >
> > > Are you sure that you want to return an array index here, instead of the
> > > actual attribute? Like:
> >
> > Actually, it returns a true/false value indicating whether the given
> > attribute should be displayed.
>
> It isn't about the return value of the function, that's fine. You call
> back with the index number (int) of the array of attributes, instead of
> passing the attribute pointer (struct attribute *attr) back to ask the
> device for the attribute to create.
For bitmaps, the int is what we will want. I can add both to the
prototype if that will make you happy? so people searching on struct
attribute and not relying on the array construction order can use the
code as well.
James
On Mon, 2007-10-29 at 13:27 -0400, Jeff Garzik wrote:
> James Bottomley wrote:
> > visibility and creation are the same thing, aren't they? An invisible
> > attribute doesn't appear in the sysfs directory, so it's equivalent to
> > the file for it not being created.
>
>
> What about the case where it's visible at creation time, but then needs
> to be made selectively invisible later on?
>
> That implies either a remove operation or dentry checks on each lookup?
Yes, that comes with the bitmap manipulation code. There will be a way
to add and remove runtime visibility. I just wanted to get the basic
concept agreed to first.
James
On Mon, 2007-10-29 at 12:28 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:27 +0100, Kay Sievers wrote:
> > On Mon, 2007-10-29 at 11:57 -0500, James Bottomley wrote:
> > > On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
> > > > On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
> >
> > > > > struct attribute_group {
> > > > > const char *name;
> > > > > + int (*filter_show)(struct kobject *, int);
> > > >
> > > > Are you sure that you want to return an array index here, instead of the
> > > > actual attribute? Like:
> > >
> > > Actually, it returns a true/false value indicating whether the given
> > > attribute should be displayed.
> >
> > It isn't about the return value of the function, that's fine. You call
> > back with the index number (int) of the array of attributes, instead of
> > passing the attribute pointer (struct attribute *attr) back to ask the
> > device for the attribute to create.
>
> For bitmaps, the int is what we will want. I can add both to the
> prototype if that will make you happy? so people searching on struct
> attribute and not relying on the array construction order can use the
> code as well.
Having both parameters sounds fine. Otherwise, if the code is spread
around several files, the attribute arrays would need to be global to
handle the callbacks, which isn't too nice.
Thanks,
Kay
James Bottomley wrote:
>> > struct attribute_group {
>> > const char *name;
>> > + int (*filter_show)(struct kobject *, int);
> Actually, it returns a true/false value indicating whether the given
> attribute should be displayed.
How about this:
int (*is_visible)(...);
or
bool (*shall_be_shown)(...);
or
bool (*should_be_displayed)(...);
or whatever, so that it indicates that this function merely answers a
question, but doesn't filter nor show anything.
--
Stefan Richter
-=====-=-=== =-=- ===-=
http://arcgraph.de/sr/
On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> James Bottomley wrote:
> >> > struct attribute_group {
> >> > const char *name;
> >> > + int (*filter_show)(struct kobject *, int);
>
> > Actually, it returns a true/false value indicating whether the given
> > attribute should be displayed.
>
> How about this:
>
> int (*is_visible)(...);
> or
> bool (*shall_be_shown)(...);
> or
> bool (*should_be_displayed)(...);
>
> or whatever, so that it indicates that this function merely answers a
> question, but doesn't filter nor show anything.
Actually, as long as it's descriptive, I really don't care about the
name. Can we all agree on "is_visible"?
James
On Mon, 29 Oct 2007 12:24:06 -0500,
James Bottomley <[email protected]> wrote:
> > Can you determine which subset of the attributes you want just before
> > actually creating the group? Then you could do something like:
> >
> > create_group(grp, kobj)
> > {
> > grp->update_creation_mask(kobj);
> > actually_create_attrs();
> > }
>
> That's actually what we currently do (at least in hand coded form) in
> the current transport classes. However, it leads to one separate group
> for each attached class. With the filter approach, we only need one
> constructed group for every transport class.
I meant doing it in the core. You still have one group for all cases,
but immediately before creating the attributes, the core checks back
which ones it should create. (Of course, that doesn't solve your
problems if you dynamically want to change availability of attributes
later on. You would need a different mechanism for that.)
On Mon, 29 Oct 2007 12:29:27 -0500,
James Bottomley <[email protected]> wrote:
> On Mon, 2007-10-29 at 13:27 -0400, Jeff Garzik wrote:
> > James Bottomley wrote:
> > > visibility and creation are the same thing, aren't they? An invisible
> > > attribute doesn't appear in the sysfs directory, so it's equivalent to
> > > the file for it not being created.
> >
> >
> > What about the case where it's visible at creation time, but then needs
> > to be made selectively invisible later on?
> >
> > That implies either a remove operation or dentry checks on each lookup?
>
> Yes, that comes with the bitmap manipulation code. There will be a way
> to add and remove runtime visibility. I just wanted to get the basic
> concept agreed to first.
But visibility always comes with creation or deletion of attributes?
Perhaps what we want is to move knowledge of the attributes to the
kobject (when attributes are created/deleted), while we decide on
visibilty of the attributes (creation/deletion of sysfs entries) based
on a filter (where visibility may change, while the attribute still
exists).
On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> James Bottomley wrote:
> >> > struct attribute_group {
> >> > const char *name;
> >> > + int (*filter_show)(struct kobject *, int);
>
> > Actually, it returns a true/false value indicating whether the given
> > attribute should be displayed.
>
> How about this:
>
> int (*is_visible)(...);
OK, so is this latest revision acceptable to everyone?
James
Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c 2007-10-30 12:35:47.000000000 -0500
@@ -16,25 +16,31 @@
#include "sysfs.h"
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
+ int i;
- for (attr = grp->attrs; *attr; attr++)
- sysfs_hash_and_remove(dir_sd, (*attr)->name);
+ for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+ if (grp->is_visible &&
+ grp->is_visible(kobj, *attr, i))
+ sysfs_hash_and_remove(dir_sd, (*attr)->name);
}
-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int error = 0;
+ int error = 0, i;
- for (attr = grp->attrs; *attr && !error; attr++)
- error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+ if (grp->is_visible &&
+ grp->is_visible(kobj, *attr, i))
+ error |=
+ sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error)
- remove_files(dir_sd, grp);
+ remove_files(dir_sd, kobj, grp);
return error;
}
@@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject *
} else
sd = kobj->sd;
sysfs_get(sd);
- error = create_files(sd, grp);
+ error = create_files(sd, kobj, grp);
if (error) {
if (grp->name)
sysfs_remove_subdir(sd);
@@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
} else
sd = sysfs_get(dir_sd);
- remove_files(sd, grp);
+ remove_files(sd, kobj, grp);
if (grp->name)
sysfs_remove_subdir(sd);
Index: BUILD-2.6/include/linux/sysfs.h
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h 2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h 2007-10-30 13:24:06.000000000 -0500
@@ -32,6 +32,8 @@ struct attribute {
struct attribute_group {
const char *name;
+ int (*is_visible)(struct kobject *,
+ struct attribute *, int);
struct attribute **attrs;
};
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c 2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c 2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject
sizeof(mp->grp.attrs[0]));
size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
- mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+ mp = kzalloc(size[0] + size[1], GFP_KERNEL);
if (!mp)
return ERR_PTR(-ENOMEM);
James Bottomley wrote:
> OK, so is this latest revision acceptable to everyone?
No complaint from me. (I'm more or less by accident in this thread
anyway. Once this feature is available in mainline, I may have use for
it in drivers/firewire/ though.) Thanks,
--
Stefan Richter
-=====-=-=== =-=- ====-
http://arcgraph.de/sr/
On Tue, 2007-10-30 at 13:25 -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> > struct attribute_group {
> > >> > const char *name;
> > >> > + int (*filter_show)(struct kobject *, int);
> >
> > > Actually, it returns a true/false value indicating whether the given
> > > attribute should be displayed.
> >
> > How about this:
> >
> > int (*is_visible)(...);
>
> OK, so is this latest revision acceptable to everyone?
Acked-by: Kay Sievers <[email protected]>
Best,
Kay
Hi James:
* James Bottomley <[email protected]> [2007-10-30 13:25:43 -0500]:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> > struct attribute_group {
> > >> > const char *name;
> > >> > + int (*filter_show)(struct kobject *, int);
> >
> > > Actually, it returns a true/false value indicating whether the given
> > > attribute should be displayed.
> >
> > How about this:
> >
> > int (*is_visible)(...);
>
> OK, so is this latest revision acceptable to everyone?
I've just been hacking around in this area a bit, for a completely different
reason: there are literally 1000's of attributes in drivers/hwmon/*.c that
really want to be const, but which cannot be due to the current API. I was
about to propose some patches that move in a different direction...
> Index: BUILD-2.6/fs/sysfs/group.c
> ===================================================================
> --- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
> +++ BUILD-2.6/fs/sysfs/group.c 2007-10-30 12:35:47.000000000 -0500
> @@ -16,25 +16,31 @@
> #include "sysfs.h"
>
>
> -static void remove_files(struct sysfs_dirent *dir_sd,
> +static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> + int i;
>
> - for (attr = grp->attrs; *attr; attr++)
> - sysfs_hash_and_remove(dir_sd, (*attr)->name);
> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> + if (grp->is_visible &&
> + grp->is_visible(kobj, *attr, i))
> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
> }
>
> -static int create_files(struct sysfs_dirent *dir_sd,
> +static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> - int error = 0;
> + int error = 0, i;
>
> - for (attr = grp->attrs; *attr && !error; attr++)
> - error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
> + for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
> + if (grp->is_visible &&
> + grp->is_visible(kobj, *attr, i))
> + error |=
> + sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
> if (error)
> - remove_files(dir_sd, grp);
> + remove_files(dir_sd, kobj, grp);
> return error;
> }
>
> @@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject *
> } else
> sd = kobj->sd;
> sysfs_get(sd);
> - error = create_files(sd, grp);
> + error = create_files(sd, kobj, grp);
> if (error) {
> if (grp->name)
> sysfs_remove_subdir(sd);
> @@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
> } else
> sd = sysfs_get(dir_sd);
>
> - remove_files(sd, grp);
> + remove_files(sd, kobj, grp);
> if (grp->name)
> sysfs_remove_subdir(sd);
>
> Index: BUILD-2.6/include/linux/sysfs.h
> ===================================================================
> --- BUILD-2.6.orig/include/linux/sysfs.h 2007-10-28 17:20:06.000000000 -0500
> +++ BUILD-2.6/include/linux/sysfs.h 2007-10-30 13:24:06.000000000 -0500
> @@ -32,6 +32,8 @@ struct attribute {
>
> struct attribute_group {
> const char *name;
> + int (*is_visible)(struct kobject *,
> + struct attribute *, int);
> struct attribute **attrs;
> };
IMHO the fundamental problem is struct attribute_group itself. This structure
is nothing but a convenience for packaging the arguments to sysfs_create_group
and sysfs_remove_group. Those functions should take the contents of that
struct as direct arguments. I haven't finished the patch series to implement
this, but since I noticed your patch I thought I'd better speak up now. Here's
the first... the idea is to eventually deprecate sysfs_[create|remove]_group()
altogether.
commit 5b5bc08ae31519b7012d7fc23ff73e0c6474de53
Author: Mark M. Hoffman <[email protected]>
Date: Sun Oct 21 11:49:57 2007 -0400
sysfs: allow attribute (group) lists to be const
The current declaration of struct attribute_group prevents long lists of
attributes from being marked const. Ideally, the second argument to the
sysfs_create_group() and sysfs_remove_group() functions would be marked "deep"
const, but C has no such construct. This patch provides a parallel set of
functions with the desired decoration.
Signed-off-by: Mark M. Hoffman <[email protected]>
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index d197237..b217a8e 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -17,71 +17,84 @@
static void remove_files(struct sysfs_dirent *dir_sd,
- const struct attribute_group *grp)
+ const struct attribute * const * attrs)
{
- struct attribute *const* attr;
+ const struct attribute *const* attr;
- for (attr = grp->attrs; *attr; attr++)
+ for (attr = attrs; *attr; attr++)
sysfs_hash_and_remove(dir_sd, (*attr)->name);
}
static int create_files(struct sysfs_dirent *dir_sd,
- const struct attribute_group *grp)
+ const struct attribute * const * attrs)
{
- struct attribute *const* attr;
+ const struct attribute *const* attr;
int error = 0;
- for (attr = grp->attrs; *attr && !error; attr++)
+ for (attr = attrs; *attr && !error; attr++)
error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error)
- remove_files(dir_sd, grp);
+ remove_files(dir_sd, attrs);
return error;
}
-
-int sysfs_create_group(struct kobject * kobj,
- const struct attribute_group * grp)
+int sysfs_create_attr_group(struct kobject * kobj,
+ const char *name, const struct attribute * const * attrs)
{
struct sysfs_dirent *sd;
int error;
BUG_ON(!kobj || !kobj->sd);
- if (grp->name) {
- error = sysfs_create_subdir(kobj, grp->name, &sd);
+ if (name) {
+ error = sysfs_create_subdir(kobj, name, &sd);
if (error)
return error;
} else
sd = kobj->sd;
sysfs_get(sd);
- error = create_files(sd, grp);
- if (error) {
- if (grp->name)
- sysfs_remove_subdir(sd);
- }
+ error = create_files(sd, attrs);
+ if (error && name)
+ sysfs_remove_subdir(sd);
+
sysfs_put(sd);
return error;
}
-void sysfs_remove_group(struct kobject * kobj,
- const struct attribute_group * grp)
+int sysfs_create_group (struct kobject * kobj,
+ const struct attribute_group *grp)
+{
+ return sysfs_create_attr_group(kobj, grp->name,
+ (const struct attribute * const *)grp->attrs);
+}
+
+void sysfs_remove_attr_group(struct kobject * kobj,
+ const char *name, const struct attribute * const * attrs)
{
struct sysfs_dirent *dir_sd = kobj->sd;
struct sysfs_dirent *sd;
- if (grp->name) {
- sd = sysfs_get_dirent(dir_sd, grp->name);
+ if (name) {
+ sd = sysfs_get_dirent(dir_sd, name);
BUG_ON(!sd);
} else
sd = sysfs_get(dir_sd);
- remove_files(sd, grp);
- if (grp->name)
+ remove_files(sd, attrs);
+ if (name)
sysfs_remove_subdir(sd);
sysfs_put(sd);
}
+void sysfs_remove_group(struct kobject *kobj,
+ const struct attribute_group *grp)
+{
+ return sysfs_remove_attr_group(kobj, grp->name,
+ (const struct attribute * const *)grp->attrs);
+}
+EXPORT_SYMBOL_GPL(sysfs_create_attr_group);
+EXPORT_SYMBOL_GPL(sysfs_remove_attr_group);
EXPORT_SYMBOL_GPL(sysfs_create_group);
EXPORT_SYMBOL_GPL(sysfs_remove_group);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 149ab62..5066d50 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -101,10 +101,18 @@ int __must_check sysfs_create_link(struct kobject *kobj, struct kobject *target,
const char *name);
void sysfs_remove_link(struct kobject *kobj, const char *name);
+int __must_check sysfs_create_attr_group(struct kobject *,
+ const char * name, const struct attribute * const * attrs);
+
int __must_check sysfs_create_group(struct kobject *kobj,
- const struct attribute_group *grp);
+ const struct attribute_group *grp);
+
+void sysfs_remove_attr_group(struct kobject *,
+ const char * name, const struct attribute * const * attrs);
+
void sysfs_remove_group(struct kobject *kobj,
- const struct attribute_group *grp);
+ const struct attribute_group *grp);
+
int sysfs_add_file_to_group(struct kobject *kobj,
const struct attribute *attr, const char *group);
void sysfs_remove_file_from_group(struct kobject *kobj,
@@ -190,8 +198,19 @@ static inline int sysfs_create_group(struct kobject *kobj,
return 0;
}
-static inline void sysfs_remove_group(struct kobject *kobj,
- const struct attribute_group *grp)
+static inline int sysfs_create_attr_group(struct kobject *k,
+ const char * name, const struct attribute * const * attrs)
+{
+ return 0;
+}
+
+static inline void sysfs_remove_group(struct kobject * k, const struct attribute_group * g)
+{
+ ;
+}
+
+static inline void sysfs_remove_attr_group(struct kobject * k,
+ const char * name, const struct attribute * const * attrs)
{
;
}
Regards,
--
Mark M. Hoffman
[email protected]
On Oct 31, 2007 1:40 AM, Mark M. Hoffman <[email protected]> wrote:
> * James Bottomley <[email protected]> [2007-10-30 13:25:43 -0500]:
> > On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > > James Bottomley wrote:
> > > >> > struct attribute_group {
> > > >> > const char *name;
> > > >> > + int (*filter_show)(struct kobject *, int);
> > >
> > > > Actually, it returns a true/false value indicating whether the given
> > > > attribute should be displayed.
> > >
> > > How about this:
> > >
> > > int (*is_visible)(...);
> >
> > OK, so is this latest revision acceptable to everyone?
>
> I've just been hacking around in this area a bit, for a completely different
> reason: there are literally 1000's of attributes in drivers/hwmon/*.c that
> really want to be const, but which cannot be due to the current API. I was
> about to propose some patches that move in a different direction...
That isn't related to "dynamic attributes", right?
> IMHO the fundamental problem is struct attribute_group itself. This structure
> is nothing but a convenience for packaging the arguments to sysfs_create_group
> and sysfs_remove_group.
That "problem" is actually a good thing. If you look at the change
rate of the internal kernel API, it saves us so much trouble. Like in
this case, James can just add a callback without caring about any
(almost :)) of the current users.
> Those functions should take the contents of that
> struct as direct arguments.
I think we should move in the opposite direction. You are right, it
isn't neccessarily pretty, but having encapsulations like this saves
us a lot of trouble while interacting with so many other people and
extending API's all the time. It's a trade, and it's a good one, if
you need to maintain code that has so many callers, and so many
architectures, you can't even check that you don't break them.
> I haven't finished the patch series to implement
> this, but since I noticed your patch I thought I'd better speak up now. Here's
> the first... the idea is to eventually deprecate sysfs_[create|remove]_group()
> altogether.
Again, I don't think, that we want to get rid of the struct container
housing all the parameters and beeing open for future extensions
without changing all the callers.
> The current declaration of struct attribute_group prevents long lists of
> attributes from being marked const. Ideally, the second argument to the
> sysfs_create_group() and sysfs_remove_group() functions would be marked "deep"
> const, but C has no such construct. This patch provides a parallel set of
> functions with the desired decoration.
What do we get out of this constification compared to the current code?
Thanks,
Kay
On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > James Bottomley wrote:
> > >> > struct attribute_group {
> > >> > const char *name;
> > >> > + int (*filter_show)(struct kobject *, int);
> >
> > > Actually, it returns a true/false value indicating whether the given
> > > attribute should be displayed.
> >
> > How about this:
> >
> > int (*is_visible)(...);
>
> OK, so is this latest revision acceptable to everyone?
>
> James
>
> Index: BUILD-2.6/fs/sysfs/group.c
> ===================================================================
> --- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
> +++ BUILD-2.6/fs/sysfs/group.c 2007-10-30 12:35:47.000000000 -0500
> @@ -16,25 +16,31 @@
> #include "sysfs.h"
>
>
> -static void remove_files(struct sysfs_dirent *dir_sd,
> +static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> + int i;
>
> - for (attr = grp->attrs; *attr; attr++)
> - sysfs_hash_and_remove(dir_sd, (*attr)->name);
> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> + if (grp->is_visible &&
> + grp->is_visible(kobj, *attr, i))
> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
Hm, doesn't this break for the zillions of attribute groups that do not
have the is_visible function set?
> -static int create_files(struct sysfs_dirent *dir_sd,
> +static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> const struct attribute_group *grp)
> {
> struct attribute *const* attr;
> - int error = 0;
> + int error = 0, i;
>
> - for (attr = grp->attrs; *attr && !error; attr++)
> - error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
> + for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
> + if (grp->is_visible &&
> + grp->is_visible(kobj, *attr, i))
> + error |=
> + sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
Same problem here, if grp->is_visible is not set, sysfs_add_file() would
never be called, right?
Other than the logic problem (I think), I have no issue with this idea
at all. Care to redo this so it works?
thanks,
greg k-h
On Tue, 30 Oct 2007 20:55:06 -0700,
Greg KH <[email protected]> wrote:
> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> > Index: BUILD-2.6/fs/sysfs/group.c
> > ===================================================================
> > --- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
> > +++ BUILD-2.6/fs/sysfs/group.c 2007-10-30 12:35:47.000000000 -0500
> > @@ -16,25 +16,31 @@
> > #include "sysfs.h"
> >
> >
> > -static void remove_files(struct sysfs_dirent *dir_sd,
> > +static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> > const struct attribute_group *grp)
> > {
> > struct attribute *const* attr;
> > + int i;
> >
> > - for (attr = grp->attrs; *attr; attr++)
> > - sysfs_hash_and_remove(dir_sd, (*attr)->name);
> > + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> > + if (grp->is_visible &&
> > + grp->is_visible(kobj, *attr, i))
> > + sysfs_hash_and_remove(dir_sd, (*attr)->name);
>
> Hm, doesn't this break for the zillions of attribute groups that do not
> have the is_visible function set?
>
> > -static int create_files(struct sysfs_dirent *dir_sd,
> > +static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
> > const struct attribute_group *grp)
> > {
> > struct attribute *const* attr;
> > - int error = 0;
> > + int error = 0, i;
> >
> > - for (attr = grp->attrs; *attr && !error; attr++)
> > - error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
> > + for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
> > + if (grp->is_visible &&
> > + grp->is_visible(kobj, *attr, i))
> > + error |=
> > + sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
>
> Same problem here, if grp->is_visible is not set, sysfs_add_file() would
> never be called, right?
>
> Other than the logic problem (I think), I have no issue with this idea
> at all. Care to redo this so it works?
Would it make more sense then to turn the meaning of the callback
around?
for (...) {
if (grp->mask_out && grp->mask_out(kobj, *attr, i))
continue;
error |= sysfs_add_file(...);
}
Cornelia Huck wrote:
> Greg KH <[email protected]> wrote:
>> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
>>> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
>>> + if (grp->is_visible &&
>>> + grp->is_visible(kobj, *attr, i))
>>> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
>> Hm, doesn't this break for the zillions of attribute groups that do not
>> have the is_visible function set?
...
> Would it make more sense then to turn the meaning of the callback
> around?
>
> for (...) {
> if (grp->mask_out && grp->mask_out(kobj, *attr, i))
> continue;
> error |= sysfs_add_file(...);
> }
if (!grp->is_visible ||
grp->is_visible(kobj, *attr, i))
add or remove();
--
Stefan Richter
-=====-=-=== =-=- =====
http://arcgraph.de/sr/
On Wed, 31 Oct 2007 10:52:35 +0100,
Stefan Richter <[email protected]> wrote:
> Cornelia Huck wrote:
> > Greg KH <[email protected]> wrote:
> >> On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
> >>> + for (i = 0, attr = grp->attrs; *attr; i++, attr++)
> >>> + if (grp->is_visible &&
> >>> + grp->is_visible(kobj, *attr, i))
> >>> + sysfs_hash_and_remove(dir_sd, (*attr)->name);
> >> Hm, doesn't this break for the zillions of attribute groups that do not
> >> have the is_visible function set?
> ...
> > Would it make more sense then to turn the meaning of the callback
> > around?
> >
> > for (...) {
> > if (grp->mask_out && grp->mask_out(kobj, *attr, i))
> > continue;
> > error |= sysfs_add_file(...);
> > }
>
> if (!grp->is_visible ||
> grp->is_visible(kobj, *attr, i))
> add or remove();
>
Hm, I find that a bit harder to parse...
mask_out() would also imply that the common use case is to have all
attributes in the group created and that you need to take action to
have an attribute not created.
Cornelia Huck wrote:
> On Wed, 31 Oct 2007 10:52:35 +0100,
> Stefan Richter <[email protected]> wrote:
>> if (!grp->is_visible ||
>> grp->is_visible(kobj, *attr, i))
>> add or remove();
>>
>
> Hm, I find that a bit harder to parse...
if (grp->is_visible == NULL ||
grp->is_visible(kobj, *attr, i))
add or remove();
However, how beautiful the implementation of static void remove_files()
and static int create_files() looks doesn't matter. What's important is
that
struct attribute_group {
const char *name;
+ int (*is_visible)(struct kobject *,
+ struct attribute *, int);
struct attribute **attrs;
};
makes sense to users --- because this is the API.
[BTW, like most of the existing driver core APIs, there are kerneldoc
comments missing here.]
> mask_out() would also imply that the common use case is to have all
> attributes in the group created and that you need to take action to
> have an attribute not created.
Here you have a point. But James has a point too when he says:
| We basically want to show capability by which file is present.
Anyway, /if/ the reverse logic is preferred, it shouldn't be called
"mask_out()" but rather "is_masked()" or "is_hidden()" or the like.
--
Stefan Richter
-=====-=-=== =-=- =====
http://arcgraph.de/sr/
Hi Kay:
* Kay Sievers <[email protected]> [2007-10-31 03:01:56 +0100]:
> On Oct 31, 2007 1:40 AM, Mark M. Hoffman <[email protected]> wrote:
> > * James Bottomley <[email protected]> [2007-10-30 13:25:43 -0500]:
> > > On Mon, 2007-10-29 at 18:58 +0100, Stefan Richter wrote:
> > > > James Bottomley wrote:
> > > > >> > struct attribute_group {
> > > > >> > const char *name;
> > > > >> > + int (*filter_show)(struct kobject *, int);
> > > >
> > > > > Actually, it returns a true/false value indicating whether the given
> > > > > attribute should be displayed.
> > > >
> > > > How about this:
> > > >
> > > > int (*is_visible)(...);
> > >
> > > OK, so is this latest revision acceptable to everyone?
> >
> > I've just been hacking around in this area a bit, for a completely different
> > reason: there are literally 1000's of attributes in drivers/hwmon/*.c that
> > really want to be const, but which cannot be due to the current API. I was
> > about to propose some patches that move in a different direction...
>
> That isn't related to "dynamic attributes", right?
Right, it's not. That phrase was coined by the previous maintainer, Jean, to
describe a specific mechanism which makes for smaller code in drivers/hwmon.
> > IMHO the fundamental problem is struct attribute_group itself. This structure
> > is nothing but a convenience for packaging the arguments to sysfs_create_group
> > and sysfs_remove_group.
>
> That "problem" is actually a good thing. If you look at the change
> rate of the internal kernel API, it saves us so much trouble. Like in
> this case, James can just add a callback without caring about any
> (almost :)) of the current users.
Given my patch, he could also just add sysfs_create_attr_group_filtered(...).
This would affect current users even less than what you suggest because it
wouldn't bloat the struct that they all use.
> > Those functions should take the contents of that
> > struct as direct arguments.
>
> I think we should move in the opposite direction. You are right, it
> isn't neccessarily pretty, but having encapsulations like this saves
> us a lot of trouble while interacting with so many other people and
> extending API's all the time. It's a trade, and it's a good one, if
> you need to maintain code that has so many callers, and so many
> architectures, you can't even check that you don't break them.
Again, I don't see how it is better to bloat a struct with many users instead
of just adding a function for the few that need the extra functionality. My
patch also requires 0 change for everyone else.
> > I haven't finished the patch series to implement
> > this, but since I noticed your patch I thought I'd better speak up now. Here's
> > the first... the idea is to eventually deprecate sysfs_[create|remove]_group()
> > altogether.
>
> Again, I don't think, that we want to get rid of the struct container
> housing all the parameters and beeing open for future extensions
> without changing all the callers.
BTW: I hope you're not resisting based on the prospect of deprecating those
two functions. I don't really have time to push such a huge patch either.
It wouldn't hurt just to keep them.
> > The current declaration of struct attribute_group prevents long lists of
> > attributes from being marked const. Ideally, the second argument to the
> > sysfs_create_group() and sysfs_remove_group() functions would be marked "deep"
> > const, but C has no such construct. This patch provides a parallel set of
> > functions with the desired decoration.
>
> What do we get out of this constification compared to the current code?
Conceptual correctness, for one, but also it allows hwmon drivers to move as
much as 1K each out of the data section and into text. This is useful for the
embedded XIP scenario.
And please don't underestimate conceptual correctness: at least hwmon (and
probably many other users) really *rely* on the core not to modify the contents
of struct attribute_group (specifically, the data to which its members point).
Without the proper const qualifiers, this is not externally guaranteed.
Regards,
--
Mark M. Hoffman
[email protected]
On Wed, 31 Oct 2007 11:37:36 +0100,
Stefan Richter <[email protected]> wrote:
> > mask_out() would also imply that the common use case is to have all
> > attributes in the group created and that you need to take action to
> > have an attribute not created.
>
> Here you have a point. But James has a point too when he says:
> | We basically want to show capability by which file is present.
Currently, if you register an attribute group, all of the attributes
will show up. I find it more intuitive to have to block some attributes
explicitely if I want to have more control, but the original logic is
fine with me as well, if most people prefer that :)
> Anyway, /if/ the reverse logic is preferred, it shouldn't be called
> "mask_out()" but rather "is_masked()" or "is_hidden()" or the like.
Sure. is_masked() would be fine.
On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> OSame problem here, if grp->is_visible is not set, sysfs_add_file() would
> never be called, right?
>
> Other than the logic problem (I think), I have no issue with this idea
> at all. Care to redo this so it works?
It's a fair cop govenor ... new patch attached.
James
---
Index: BUILD-2.6/fs/sysfs/group.c
===================================================================
--- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.000000000 -0500
+++ BUILD-2.6/fs/sysfs/group.c 2007-10-31 09:29:14.000000000 -0500
@@ -16,25 +16,31 @@
#include "sysfs.h"
-static void remove_files(struct sysfs_dirent *dir_sd,
+static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
+ int i;
- for (attr = grp->attrs; *attr; attr++)
- sysfs_hash_and_remove(dir_sd, (*attr)->name);
+ for (i = 0, attr = grp->attrs; *attr; i++, attr++)
+ if (!grp->is_visible ||
+ grp->is_visible(kobj, *attr, i))
+ sysfs_hash_and_remove(dir_sd, (*attr)->name);
}
-static int create_files(struct sysfs_dirent *dir_sd,
+static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int error = 0;
+ int error = 0, i;
- for (attr = grp->attrs; *attr && !error; attr++)
- error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++)
+ if (!grp->is_visible ||
+ grp->is_visible(kobj, *attr, i))
+ error |=
+ sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
if (error)
- remove_files(dir_sd, grp);
+ remove_files(dir_sd, kobj, grp);
return error;
}
@@ -54,7 +60,7 @@ int sysfs_create_group(struct kobject *
} else
sd = kobj->sd;
sysfs_get(sd);
- error = create_files(sd, grp);
+ error = create_files(sd, kobj, grp);
if (error) {
if (grp->name)
sysfs_remove_subdir(sd);
@@ -75,7 +81,7 @@ void sysfs_remove_group(struct kobject *
} else
sd = sysfs_get(dir_sd);
- remove_files(sd, grp);
+ remove_files(sd, kobj, grp);
if (grp->name)
sysfs_remove_subdir(sd);
Index: BUILD-2.6/include/linux/sysfs.h
===================================================================
--- BUILD-2.6.orig/include/linux/sysfs.h 2007-10-28 17:20:06.000000000 -0500
+++ BUILD-2.6/include/linux/sysfs.h 2007-10-30 13:24:06.000000000 -0500
@@ -32,6 +32,8 @@ struct attribute {
struct attribute_group {
const char *name;
+ int (*is_visible)(struct kobject *,
+ struct attribute *, int);
struct attribute **attrs;
};
Index: BUILD-2.6/kernel/params.c
===================================================================
--- BUILD-2.6.orig/kernel/params.c 2007-10-29 01:18:43.000000000 -0500
+++ BUILD-2.6/kernel/params.c 2007-10-29 01:18:49.000000000 -0500
@@ -472,7 +472,7 @@ param_sysfs_setup(struct module_kobject
sizeof(mp->grp.attrs[0]));
size[1] = (valid_attrs + 1) * sizeof(mp->grp.attrs[0]);
- mp = kmalloc(size[0] + size[1], GFP_KERNEL);
+ mp = kzalloc(size[0] + size[1], GFP_KERNEL);
if (!mp)
return ERR_PTR(-ENOMEM);
On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > OSame problem here, if grp->is_visible is not set, sysfs_add_file() would
> > never be called, right?
> >
> > Other than the logic problem (I think), I have no issue with this idea
> > at all. Care to redo this so it works?
>
> It's a fair cop govenor ... new patch attached.
Much better, thanks :)
I have no problem with this, if you want to take this in your tree,
please do and feel free to add:
Signed-off-by: Greg Kroah-Hartman <[email protected]>
to it, or I can take it in mine.
thanks,
greg k-h
On Wed, 2007-10-31 at 10:29 -0700, Greg KH wrote:
> On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> > On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > > OSame problem here, if grp->is_visible is not set, sysfs_add_file() would
> > > never be called, right?
> > >
> > > Other than the logic problem (I think), I have no issue with this idea
> > > at all. Care to redo this so it works?
> >
> > It's a fair cop govenor ... new patch attached.
>
> Much better, thanks :)
>
> I have no problem with this, if you want to take this in your tree,
> please do and feel free to add:
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> to it, or I can take it in mine.
Actually, I think I have a use for it and the follow on patch (coming
soon) to do attribute bitmaps in the SCSI tree, so I'd like to add your
ack and take it through this tree.
Thanks,
James
On Sun, Nov 04, 2007 at 08:12:02AM -0600, James Bottomley wrote:
> On Wed, 2007-10-31 at 10:29 -0700, Greg KH wrote:
> > On Wed, Oct 31, 2007 at 09:38:04AM -0500, James Bottomley wrote:
> > > On Tue, 2007-10-30 at 20:55 -0700, Greg KH wrote:
> > > > OSame problem here, if grp->is_visible is not set, sysfs_add_file() would
> > > > never be called, right?
> > > >
> > > > Other than the logic problem (I think), I have no issue with this idea
> > > > at all. Care to redo this so it works?
> > >
> > > It's a fair cop govenor ... new patch attached.
> >
> > Much better, thanks :)
> >
> > I have no problem with this, if you want to take this in your tree,
> > please do and feel free to add:
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >
> > to it, or I can take it in mine.
>
> Actually, I think I have a use for it and the follow on patch (coming
> soon) to do attribute bitmaps in the SCSI tree, so I'd like to add your
> ack and take it through this tree.
That's fine. Can you CC: me on the patch that uses this code so I can
see how well it works out?
thanks,
greg k-h