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 <James.Bottomley@SteelEye.com>
----
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->f...
[ message continues ]

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
-

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
Actually, it returns a true/false value indicating whether the given
how about (*attribute_is_visible)?
James
-

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/

-

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(s...
[ message continues ]

Hm, doesn't this break for the zillions of attribute groups that do not
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

-

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: B...
[ message continues ]

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 <gregkh@suse.de>
to it, or I can take it in mine.
thanks,
greg k-h

-

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 Tue, 30 Oct 2007 20:55:06 -0700,
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,
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.

-

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
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/

-

On Wed, 31 Oct 2007 11:37:36 +0100,
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
Sure. is_masked() would be fine.
-

Hi James:
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
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 <mhoffman@lightlink.com>

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 <mhoffman@lightlink.com>
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)

 {...
[ message continues ]

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
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
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
What do we get out of this constification compared to the current code?
Thanks,

Kay

-

Hi Kay:
Right, it's not.  That phrase was coined by the previous maintainer, Jean, to
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
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
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.
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

mhoffman@lightlink.com
-

Acked-by: Kay Sievers <kay.sievers@vrfy.org>
Best,

Kay
-

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/

-

Actually, as long as it's descriptive, I really don't care about the

name.  Can we all agree on "is_visible"?
James
-

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
-

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
-

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
-

On Mon, 29 Oct 2007 11:57:51 -0500,
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();
Well, you don't only stop the visibility, but the creation of the

attribute, so perhaps (*creation_filter)?

-

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
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, 29 Oct 2007 12:24:06 -0500,
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.)

-

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
-

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, 29 Oct 2007 12:29:27 -0500,
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).

-