Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to attribute_group

[Damien Le Moal]

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