On 2023/08/18 8:58, Alistair Francis wrote: > When creating an attribute group, if it is named a subdirectory it is > created and the sysfs files are placed into that subdirectory. If no > files are created, normally the directory would still be present, but it > would be empty. > > This can be confusing for users, as it appears the feature is avaliable > as there is a directory, but it isn't supported by the hardware or the > kernel. > > One way to fix this is to remove directories that don't contain any > files, such as [1]. The problem with this is that there are currently > lots of users in the kernel who expect the group to remain even if > empty, as they dynamically add/merge properties later. > > The documentation for sysfs_merge_group() specifically says > > This function returns an error if the group doesn't exist or any of the > files already exist in that group, in which case none of the new files > are created. > > So just not adding the group if it's empty doesn't work, at least not > with the code we currently have. The code can be changed to support > this, but it is difficult to determine how this will affect existing use > cases. > > This approach instead adds a new function pointer, attr_is_visible(), to > `struct attribute_group` which can be set if the user wants to filter > the avaliablility of the function. > > This matches the .is_visible() function pointer that already exists and > is commonly used. This approach provides greater control over if the > directory should be visible or not. > > This will be used by the PCIe DOE sysfs attributes to kind the directory > on devices that don't support DOE. > > 1: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=debugfs_cleanup&id=f670945dfbaf353fe068544c31e3fa45575da5b5 > > Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx> > --- > v6: > - Add patch > > fs/sysfs/group.c | 12 +++++++++++- > include/linux/sysfs.h | 6 ++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index 138676463336..34afd5becdbe 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -111,6 +111,7 @@ static int internal_create_group(struct kobject *kobj, int update, > kuid_t uid; > kgid_t gid; > int error; > + umode_t mode; > > if (WARN_ON(!kobj || (!update && !kobj->sd))) > return -EINVAL; > @@ -125,6 +126,15 @@ static int internal_create_group(struct kobject *kobj, int update, > return 0; > } > > + if (grp->attr_is_visible) { > + mode = grp->attr_is_visible(kobj); > + Remove the blank line here. > + if (mode == 0) Nit: if (!mode) > + return 0; > + } else { > + mode = S_IRWXU | S_IRUGO | S_IXUGO; > + } > + > kobject_get_ownership(kobj, &uid, &gid); > if (grp->name) { > if (update) { > @@ -136,7 +146,7 @@ static int internal_create_group(struct kobject *kobj, int update, > } > } else { > kn = kernfs_create_dir_ns(kobj->sd, grp->name, > - S_IRWXU | S_IRUGO | S_IXUGO, > + mode, > uid, gid, kobj, NULL); > if (IS_ERR(kn)) { > if (PTR_ERR(kn) == -EEXIST) > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index fd3fe5c8c17f..808e7fc0ca57 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -63,6 +63,11 @@ do { \ > * @name: Optional: Attribute group name > * If specified, the attribute group will be created in > * a new subdirectory with this name. > + * @attr_is_visible: Optional: Function to return permissions Given that the existing is_visible() function apply to an attribute, naming this one "grp_is_visible" may be better. Otherwise, this is really confusing I think. > + * associated with the attribute group. Only read/write > + * permissions as well as SYSFS_PREALLOC are accepted. Must > + * return 0 if an attribute is not visible. The returned value > + * will replace static permissions defined in struct attribute. > * @is_visible: Optional: Function to return permissions associated with an > * attribute of the group. Will be called repeatedly for each > * non-binary attribute in the group. Only read/write > @@ -83,6 +88,7 @@ do { \ > */ > struct attribute_group { > const char *name; > + umode_t (*attr_is_visible)(struct kobject *); > umode_t (*is_visible)(struct kobject *, > struct attribute *, int); > umode_t (*is_bin_visible)(struct kobject *, -- Damien Le Moal Western Digital Research
