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

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@wdc.com>
---
v6:
 - Add patch

 fs/sysfs/group.c      | 12 +++++++++++-
 include/linux/sysfs.h |  6 ++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

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@wdc.com>
> ---
> 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 *,

On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> 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.

Did you try?  I'd really really really prefer we do it this way as it's
much simpler overall to have the sysfs core "do the right thing
automatically" than to force each and every bus/subsystem to have to
manually add this type of attribute.

Also, on a personal level, I want this function to work as it will allow
us to remove some code in some busses that don't really need to be there
(dynamic creation of some device attributes), which will then also allow
me to remove some apis in the driver core that should not be used at all
anymore (but keep creeping back in as there is still a few users.)

So I'll be selfish here and say "please try to get my proposed change to
work, it's really the correct thing to do here."

thanks,

greg k-h

On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > 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.
>
> Did you try?  I'd really really really prefer we do it this way as it's
> much simpler overall to have the sysfs core "do the right thing
> automatically" than to force each and every bus/subsystem to have to
> manually add this type of attribute.
>
> Also, on a personal level, I want this function to work as it will allow
> us to remove some code in some busses that don't really need to be there
> (dynamic creation of some device attributes), which will then also allow
> me to remove some apis in the driver core that should not be used at all
> anymore (but keep creeping back in as there is still a few users.)
>
> So I'll be selfish here and say "please try to get my proposed change to
> work, it's really the correct thing to do here."

I did try!

This is an attempt:
https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9

It is based on your original patch with a bunch of:

if (!parent) {
    parent = kernfs_create_dir_ns(kobj->sd, grp->name,
                  S_IRWXU | S_IRUGO | S_IXUGO,
                  uid, gid, kobj, NULL);
    ...
    }


added throughout the code base.

Very basic testing shows that it does what I need it to do and I don't
see any kernel oops on boot.

I prefer the approach I have in this mailing list patch. But if you
like the commit mentioned above I can tidy and clean it up and then
use that instead

Alistair

>
> thanks,
>
> greg k-h

On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > 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.
> >
> > Did you try?  I'd really really really prefer we do it this way as it's
> > much simpler overall to have the sysfs core "do the right thing
> > automatically" than to force each and every bus/subsystem to have to
> > manually add this type of attribute.
> >
> > Also, on a personal level, I want this function to work as it will allow
> > us to remove some code in some busses that don't really need to be there
> > (dynamic creation of some device attributes), which will then also allow
> > me to remove some apis in the driver core that should not be used at all
> > anymore (but keep creeping back in as there is still a few users.)
> >
> > So I'll be selfish here and say "please try to get my proposed change to
> > work, it's really the correct thing to do here."
> 
> I did try!
> 
> This is an attempt:
> https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> 
> It is based on your original patch with a bunch of:
> 
> if (!parent) {
>     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
>                   S_IRWXU | S_IRUGO | S_IXUGO,
>                   uid, gid, kobj, NULL);
>     ...
>     }
> 
> 
> added throughout the code base.
> 
> Very basic testing shows that it does what I need it to do and I don't
> see any kernel oops on boot.

Nice!

Mind if I take it and clean it up a bit and test with it here?  Again, I
need this functionality for other stuff as well, so getting it merged
for your feature is fine with me.

> I prefer the approach I have in this mailing list patch. But if you
> like the commit mentioned above I can tidy and clean it up and then
> use that instead

I would rather do it this way.  I can work on this on Friday if you want
me to.

thanks,

greg k-h

On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > 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.
> > >
> > > Did you try?  I'd really really really prefer we do it this way as it's
> > > much simpler overall to have the sysfs core "do the right thing
> > > automatically" than to force each and every bus/subsystem to have to
> > > manually add this type of attribute.
> > >
> > > Also, on a personal level, I want this function to work as it will allow
> > > us to remove some code in some busses that don't really need to be there
> > > (dynamic creation of some device attributes), which will then also allow
> > > me to remove some apis in the driver core that should not be used at all
> > > anymore (but keep creeping back in as there is still a few users.)
> > >
> > > So I'll be selfish here and say "please try to get my proposed change to
> > > work, it's really the correct thing to do here."
> >
> > I did try!
> >
> > This is an attempt:
> > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> >
> > It is based on your original patch with a bunch of:
> >
> > if (!parent) {
> >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> >                   S_IRWXU | S_IRUGO | S_IXUGO,
> >                   uid, gid, kobj, NULL);
> >     ...
> >     }
> >
> >
> > added throughout the code base.
> >
> > Very basic testing shows that it does what I need it to do and I don't
> > see any kernel oops on boot.
>
> Nice!
>
> Mind if I take it and clean it up a bit and test with it here?  Again, I
> need this functionality for other stuff as well, so getting it merged
> for your feature is fine with me.

Sure! Go ahead. Sorry I was travelling last week.

>
> > I prefer the approach I have in this mailing list patch. But if you
> > like the commit mentioned above I can tidy and clean it up and then
> > use that instead
>
> I would rather do it this way.  I can work on this on Friday if you want
> me to.

Yeah, that's fine with me. If you aren't able to let me know and I can
finish up the patch and send it with this series

Alistair

>
> thanks,
>
> greg k-h

On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > 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.
> > > >
> > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > much simpler overall to have the sysfs core "do the right thing
> > > > automatically" than to force each and every bus/subsystem to have to
> > > > manually add this type of attribute.
> > > >
> > > > Also, on a personal level, I want this function to work as it will allow
> > > > us to remove some code in some busses that don't really need to be there
> > > > (dynamic creation of some device attributes), which will then also allow
> > > > me to remove some apis in the driver core that should not be used at all
> > > > anymore (but keep creeping back in as there is still a few users.)
> > > >
> > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > work, it's really the correct thing to do here."
> > >
> > > I did try!
> > >
> > > This is an attempt:
> > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > >
> > > It is based on your original patch with a bunch of:
> > >
> > > if (!parent) {
> > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > >                   uid, gid, kobj, NULL);
> > >     ...
> > >     }
> > >
> > >
> > > added throughout the code base.
> > >
> > > Very basic testing shows that it does what I need it to do and I don't
> > > see any kernel oops on boot.
> >
> > Nice!
> >
> > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > need this functionality for other stuff as well, so getting it merged
> > for your feature is fine with me.
> 
> Sure! Go ahead. Sorry I was travelling last week.
> 
> >
> > > I prefer the approach I have in this mailing list patch. But if you
> > > like the commit mentioned above I can tidy and clean it up and then
> > > use that instead
> >
> > I would rather do it this way.  I can work on this on Friday if you want
> > me to.
> 
> Yeah, that's fine with me. If you aren't able to let me know and I can
> finish up the patch and send it with this series

Great, and for the email record, as github links are not stable, here's
the patch that you have above attached below.  I'll test this out and
clean it up a bit more and see how it goes...

thanks,

greg k-h


From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Wed, 24 Aug 2022 15:45:36 +0200
Subject: [PATCH] sysfs: do not create empty directories if no attributes are
 present

When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
were successfully created in the group at all.

Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 fs/sysfs/file.c  | 14 ++++++++++--
 fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index a12ac0356c69..7aab6c09662c 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
 		kernfs_get(parent);
 	}
 
-	if (!parent)
-		return -ENOENT;
+	if (!parent) {
+		parent = kernfs_create_dir_ns(kobj->sd, group,
+					  S_IRWXU | S_IRUGO | S_IXUGO,
+					  uid, gid, kobj, NULL);
+		if (IS_ERR(parent)) {
+			if (PTR_ERR(parent) == -EEXIST)
+				sysfs_warn_dup(kobj->sd, group);
+			return PTR_ERR(parent);
+		}
+
+		kernfs_get(parent);
+	}
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..013fa333cd3c 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
 			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
+/* returns -ERROR if error, or >= 0 for number of files actually created */
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 			kuid_t uid, kgid_t gid,
 			const struct attribute_group *grp, int update)
 {
 	struct attribute *const *attr;
 	struct bin_attribute *const *bin_attr;
+	int files_created = 0;
 	int error = 0, i;
 
 	if (grp->attrs) {
@@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 						       gid, NULL);
 			if (unlikely(error))
 				break;
+
+			files_created++;
 		}
 		if (error) {
 			remove_files(parent, grp);
@@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 							   NULL);
 			if (error)
 				break;
+			files_created++;
 		}
 		if (error)
 			remove_files(parent, grp);
 	}
 exit:
-	return error;
+	if (error)
+		return error;
+	return files_created;
 }
 
 
@@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				kn = kernfs_create_dir_ns(kobj->sd, grp->name,
+							  S_IRWXU | S_IRUGO | S_IXUGO,
+							  uid, gid, kobj, NULL);
+				if (IS_ERR(kn)) {
+					if (PTR_ERR(kn) == -EEXIST)
+						sysfs_warn_dup(kobj->sd, grp->name);
+					return PTR_ERR(kn);
+				}
 			}
 		} else {
 			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
@@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kernfs_get(kn);
 	error = create_files(kn, kobj, uid, gid, grp, update);
-	if (error) {
+	if (error <= 0) {
+		/*
+		 * If an error happened _OR_ if no files were created in the
+		 * attribute group, and we have a name for this group, delete
+		 * the name so there's not an empty directory.
+		 */
 		if (grp->name)
 			kernfs_remove(kn);
+	} else {
+		error = 0;
+		kernfs_put(kn);
 	}
-	kernfs_put(kn);
 
 	if (grp->name && update)
 		kernfs_put(kn);
@@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
 EXPORT_SYMBOL_GPL(sysfs_remove_groups);
 
 /**
- * sysfs_merge_group - merge files into a pre-existing attribute group.
+ * sysfs_merge_group - merge files into a attribute group.
  * @kobj:	The kobject containing the group.
  * @grp:	The files to create and the attribute group they belong to.
  *
- * 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.
+ * This function returns an error if any of the files already exist in
+ * that group, in which case none of the new files are created.
  */
 int sysfs_merge_group(struct kobject *kobj,
 		       const struct attribute_group *grp)
@@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
 	struct attribute *const *attr;
 	int i;
 
-	parent = kernfs_find_and_get(kobj->sd, grp->name);
-	if (!parent)
-		return -ENOENT;
-
 	kobject_get_ownership(kobj, &uid, &gid);
 
+	parent = kernfs_find_and_get(kobj->sd, grp->name);
+	if (!parent) {
+		parent = kernfs_create_dir_ns(kobj->sd, grp->name,
+					  S_IRWXU | S_IRUGO | S_IXUGO,
+					  uid, gid, kobj, NULL);
+		if (IS_ERR(parent)) {
+			if (PTR_ERR(parent) == -EEXIST)
+				sysfs_warn_dup(kobj->sd, grp->name);
+			return PTR_ERR(parent);
+		}
+
+		kernfs_get(parent);
+	}
+
 	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
 		error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
 					       uid, gid, NULL);

On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > 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.
> > > > >
> > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > manually add this type of attribute.
> > > > >
> > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > us to remove some code in some busses that don't really need to be there
> > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > me to remove some apis in the driver core that should not be used at all
> > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > >
> > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > work, it's really the correct thing to do here."
> > > >
> > > > I did try!
> > > >
> > > > This is an attempt:
> > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > >
> > > > It is based on your original patch with a bunch of:
> > > >
> > > > if (!parent) {
> > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > >                   uid, gid, kobj, NULL);
> > > >     ...
> > > >     }
> > > >
> > > >
> > > > added throughout the code base.
> > > >
> > > > Very basic testing shows that it does what I need it to do and I don't
> > > > see any kernel oops on boot.
> > >
> > > Nice!
> > >
> > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > need this functionality for other stuff as well, so getting it merged
> > > for your feature is fine with me.
> > 
> > Sure! Go ahead. Sorry I was travelling last week.
> > 
> > >
> > > > I prefer the approach I have in this mailing list patch. But if you
> > > > like the commit mentioned above I can tidy and clean it up and then
> > > > use that instead
> > >
> > > I would rather do it this way.  I can work on this on Friday if you want
> > > me to.
> > 
> > Yeah, that's fine with me. If you aren't able to let me know and I can
> > finish up the patch and send it with this series
> 
> Great, and for the email record, as github links are not stable, here's
> the patch that you have above attached below.  I'll test this out and
> clean it up a bit more and see how it goes...
> 
> thanks,
> 
> greg k-h
> 
> 
> From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date: Wed, 24 Aug 2022 15:45:36 +0200
> Subject: [PATCH] sysfs: do not create empty directories if no attributes are
>  present
> 
> When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> were successfully created in the group at all.
> 
> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  fs/sysfs/file.c  | 14 ++++++++++--
>  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index a12ac0356c69..7aab6c09662c 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
>  		kernfs_get(parent);
>  	}
>  
> -	if (!parent)
> -		return -ENOENT;
> +	if (!parent) {
> +		parent = kernfs_create_dir_ns(kobj->sd, group,
> +					  S_IRWXU | S_IRUGO | S_IXUGO,
> +					  uid, gid, kobj, NULL);
> +		if (IS_ERR(parent)) {
> +			if (PTR_ERR(parent) == -EEXIST)
> +				sysfs_warn_dup(kobj->sd, group);
> +			return PTR_ERR(parent);
> +		}
> +
> +		kernfs_get(parent);
> +	}
>  
>  	kobject_get_ownership(kobj, &uid, &gid);
>  	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 138676463336..013fa333cd3c 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
>  			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
>  }
>  
> +/* returns -ERROR if error, or >= 0 for number of files actually created */
>  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  			kuid_t uid, kgid_t gid,
>  			const struct attribute_group *grp, int update)
>  {
>  	struct attribute *const *attr;
>  	struct bin_attribute *const *bin_attr;
> +	int files_created = 0;
>  	int error = 0, i;
>  
>  	if (grp->attrs) {
> @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  						       gid, NULL);
>  			if (unlikely(error))
>  				break;
> +
> +			files_created++;
>  		}
>  		if (error) {
>  			remove_files(parent, grp);
> @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  							   NULL);
>  			if (error)
>  				break;
> +			files_created++;
>  		}
>  		if (error)
>  			remove_files(parent, grp);
>  	}
>  exit:
> -	return error;
> +	if (error)
> +		return error;
> +	return files_created;
>  }
>  
>  
> @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
>  		if (update) {
>  			kn = kernfs_find_and_get(kobj->sd, grp->name);
>  			if (!kn) {
> -				pr_warn("Can't update unknown attr grp name: %s/%s\n",
> -					kobj->name, grp->name);
> -				return -EINVAL;
> +				kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> +							  S_IRWXU | S_IRUGO | S_IXUGO,
> +							  uid, gid, kobj, NULL);
> +				if (IS_ERR(kn)) {
> +					if (PTR_ERR(kn) == -EEXIST)
> +						sysfs_warn_dup(kobj->sd, grp->name);
> +					return PTR_ERR(kn);
> +				}
>  			}
>  		} else {
>  			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
>  
>  	kernfs_get(kn);
>  	error = create_files(kn, kobj, uid, gid, grp, update);
> -	if (error) {
> +	if (error <= 0) {
> +		/*
> +		 * If an error happened _OR_ if no files were created in the
> +		 * attribute group, and we have a name for this group, delete
> +		 * the name so there's not an empty directory.
> +		 */
>  		if (grp->name)
>  			kernfs_remove(kn);
> +	} else {
> +		error = 0;
> +		kernfs_put(kn);
>  	}
> -	kernfs_put(kn);
>  
>  	if (grp->name && update)
>  		kernfs_put(kn);
> @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
>  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
>  
>  /**
> - * sysfs_merge_group - merge files into a pre-existing attribute group.
> + * sysfs_merge_group - merge files into a attribute group.
>   * @kobj:	The kobject containing the group.
>   * @grp:	The files to create and the attribute group they belong to.
>   *
> - * 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.
> + * This function returns an error if any of the files already exist in
> + * that group, in which case none of the new files are created.
>   */
>  int sysfs_merge_group(struct kobject *kobj,
>  		       const struct attribute_group *grp)
> @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
>  	struct attribute *const *attr;
>  	int i;
>  
> -	parent = kernfs_find_and_get(kobj->sd, grp->name);
> -	if (!parent)
> -		return -ENOENT;
> -
>  	kobject_get_ownership(kobj, &uid, &gid);
>  
> +	parent = kernfs_find_and_get(kobj->sd, grp->name);
> +	if (!parent) {
> +		parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> +					  S_IRWXU | S_IRUGO | S_IXUGO,
> +					  uid, gid, kobj, NULL);
> +		if (IS_ERR(parent)) {
> +			if (PTR_ERR(parent) == -EEXIST)
> +				sysfs_warn_dup(kobj->sd, grp->name);
> +			return PTR_ERR(parent);
> +		}
> +
> +		kernfs_get(parent);
> +	}
> +
>  	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
>  		error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
>  					       uid, gid, NULL);
> -- 
> 2.42.0
> 

And as the 0-day bot just showed, this patch isn't going to work
properly, the uid/gid stuff isn't all hooked up properly, I'll work on
fixing that up when I get some cycles...

thanks,

greg k-h

On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > 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.
> > > > > >
> > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > manually add this type of attribute.
> > > > > >
> > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > >
> > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > work, it's really the correct thing to do here."
> > > > >
> > > > > I did try!
> > > > >
> > > > > This is an attempt:
> > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > >
> > > > > It is based on your original patch with a bunch of:
> > > > >
> > > > > if (!parent) {
> > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > >                   uid, gid, kobj, NULL);
> > > > >     ...
> > > > >     }
> > > > >
> > > > >
> > > > > added throughout the code base.
> > > > >
> > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > see any kernel oops on boot.
> > > >
> > > > Nice!
> > > >
> > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > need this functionality for other stuff as well, so getting it merged
> > > > for your feature is fine with me.
> > > 
> > > Sure! Go ahead. Sorry I was travelling last week.
> > > 
> > > >
> > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > use that instead
> > > >
> > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > me to.
> > > 
> > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > finish up the patch and send it with this series
> > 
> > Great, and for the email record, as github links are not stable, here's
> > the patch that you have above attached below.  I'll test this out and
> > clean it up a bit more and see how it goes...
> > 
> > thanks,
> > 
> > greg k-h
> > 
> > 
> > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> >  present
> > 
> > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > were successfully created in the group at all.
> > 
> > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  fs/sysfs/file.c  | 14 ++++++++++--
> >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> >  2 files changed, 54 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index a12ac0356c69..7aab6c09662c 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> >  		kernfs_get(parent);
> >  	}
> >  
> > -	if (!parent)
> > -		return -ENOENT;
> > +	if (!parent) {
> > +		parent = kernfs_create_dir_ns(kobj->sd, group,
> > +					  S_IRWXU | S_IRUGO | S_IXUGO,
> > +					  uid, gid, kobj, NULL);
> > +		if (IS_ERR(parent)) {
> > +			if (PTR_ERR(parent) == -EEXIST)
> > +				sysfs_warn_dup(kobj->sd, group);
> > +			return PTR_ERR(parent);
> > +		}
> > +
> > +		kernfs_get(parent);
> > +	}
> >  
> >  	kobject_get_ownership(kobj, &uid, &gid);
> >  	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index 138676463336..013fa333cd3c 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> >  			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> >  }
> >  
> > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  			kuid_t uid, kgid_t gid,
> >  			const struct attribute_group *grp, int update)
> >  {
> >  	struct attribute *const *attr;
> >  	struct bin_attribute *const *bin_attr;
> > +	int files_created = 0;
> >  	int error = 0, i;
> >  
> >  	if (grp->attrs) {
> > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  						       gid, NULL);
> >  			if (unlikely(error))
> >  				break;
> > +
> > +			files_created++;
> >  		}
> >  		if (error) {
> >  			remove_files(parent, grp);
> > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  							   NULL);
> >  			if (error)
> >  				break;
> > +			files_created++;
> >  		}
> >  		if (error)
> >  			remove_files(parent, grp);
> >  	}
> >  exit:
> > -	return error;
> > +	if (error)
> > +		return error;
> > +	return files_created;
> >  }
> >  
> >  
> > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> >  		if (update) {
> >  			kn = kernfs_find_and_get(kobj->sd, grp->name);
> >  			if (!kn) {
> > -				pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > -					kobj->name, grp->name);
> > -				return -EINVAL;
> > +				kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > +							  S_IRWXU | S_IRUGO | S_IXUGO,
> > +							  uid, gid, kobj, NULL);
> > +				if (IS_ERR(kn)) {
> > +					if (PTR_ERR(kn) == -EEXIST)
> > +						sysfs_warn_dup(kobj->sd, grp->name);
> > +					return PTR_ERR(kn);
> > +				}
> >  			}
> >  		} else {
> >  			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> >  
> >  	kernfs_get(kn);
> >  	error = create_files(kn, kobj, uid, gid, grp, update);
> > -	if (error) {
> > +	if (error <= 0) {
> > +		/*
> > +		 * If an error happened _OR_ if no files were created in the
> > +		 * attribute group, and we have a name for this group, delete
> > +		 * the name so there's not an empty directory.
> > +		 */
> >  		if (grp->name)
> >  			kernfs_remove(kn);
> > +	} else {
> > +		error = 0;
> > +		kernfs_put(kn);
> >  	}
> > -	kernfs_put(kn);
> >  
> >  	if (grp->name && update)
> >  		kernfs_put(kn);
> > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> >  
> >  /**
> > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > + * sysfs_merge_group - merge files into a attribute group.
> >   * @kobj:	The kobject containing the group.
> >   * @grp:	The files to create and the attribute group they belong to.
> >   *
> > - * 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.
> > + * This function returns an error if any of the files already exist in
> > + * that group, in which case none of the new files are created.
> >   */
> >  int sysfs_merge_group(struct kobject *kobj,
> >  		       const struct attribute_group *grp)
> > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> >  	struct attribute *const *attr;
> >  	int i;
> >  
> > -	parent = kernfs_find_and_get(kobj->sd, grp->name);
> > -	if (!parent)
> > -		return -ENOENT;
> > -
> >  	kobject_get_ownership(kobj, &uid, &gid);
> >  
> > +	parent = kernfs_find_and_get(kobj->sd, grp->name);
> > +	if (!parent) {
> > +		parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > +					  S_IRWXU | S_IRUGO | S_IXUGO,
> > +					  uid, gid, kobj, NULL);
> > +		if (IS_ERR(parent)) {
> > +			if (PTR_ERR(parent) == -EEXIST)
> > +				sysfs_warn_dup(kobj->sd, grp->name);
> > +			return PTR_ERR(parent);
> > +		}
> > +
> > +		kernfs_get(parent);
> > +	}
> > +
> >  	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> >  		error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> >  					       uid, gid, NULL);
> > -- 
> > 2.42.0
> > 
> 
> And as the 0-day bot just showed, this patch isn't going to work
> properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> fixing that up when I get some cycles...

Oops, nope, that was my fault in applying this to my tree, sorry for the
noise...

On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > 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.
> > > > > > >
> > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > manually add this type of attribute.
> > > > > > >
> > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > >
> > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > work, it's really the correct thing to do here."
> > > > > >
> > > > > > I did try!
> > > > > >
> > > > > > This is an attempt:
> > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > >
> > > > > > It is based on your original patch with a bunch of:
> > > > > >
> > > > > > if (!parent) {
> > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > >                   uid, gid, kobj, NULL);
> > > > > >     ...
> > > > > >     }
> > > > > >
> > > > > >
> > > > > > added throughout the code base.
> > > > > >
> > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > see any kernel oops on boot.
> > > > >
> > > > > Nice!
> > > > >
> > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > need this functionality for other stuff as well, so getting it merged
> > > > > for your feature is fine with me.
> > > > 
> > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > 
> > > > >
> > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > use that instead
> > > > >
> > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > me to.
> > > > 
> > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > finish up the patch and send it with this series
> > > 
> > > Great, and for the email record, as github links are not stable, here's
> > > the patch that you have above attached below.  I'll test this out and
> > > clean it up a bit more and see how it goes...
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > > 
> > > 
> > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > >  present
> > > 
> > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > were successfully created in the group at all.
> > > 
> > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  fs/sysfs/file.c  | 14 ++++++++++--
> > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > index a12ac0356c69..7aab6c09662c 100644
> > > --- a/fs/sysfs/file.c
> > > +++ b/fs/sysfs/file.c
> > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > >  		kernfs_get(parent);
> > >  	}
> > >  
> > > -	if (!parent)
> > > -		return -ENOENT;
> > > +	if (!parent) {
> > > +		parent = kernfs_create_dir_ns(kobj->sd, group,
> > > +					  S_IRWXU | S_IRUGO | S_IXUGO,
> > > +					  uid, gid, kobj, NULL);
> > > +		if (IS_ERR(parent)) {
> > > +			if (PTR_ERR(parent) == -EEXIST)
> > > +				sysfs_warn_dup(kobj->sd, group);
> > > +			return PTR_ERR(parent);
> > > +		}
> > > +
> > > +		kernfs_get(parent);
> > > +	}
> > >  
> > >  	kobject_get_ownership(kobj, &uid, &gid);
> > >  	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > index 138676463336..013fa333cd3c 100644
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > >  			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > >  }
> > >  
> > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > >  			kuid_t uid, kgid_t gid,
> > >  			const struct attribute_group *grp, int update)
> > >  {
> > >  	struct attribute *const *attr;
> > >  	struct bin_attribute *const *bin_attr;
> > > +	int files_created = 0;
> > >  	int error = 0, i;
> > >  
> > >  	if (grp->attrs) {
> > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > >  						       gid, NULL);
> > >  			if (unlikely(error))
> > >  				break;
> > > +
> > > +			files_created++;
> > >  		}
> > >  		if (error) {
> > >  			remove_files(parent, grp);
> > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > >  							   NULL);
> > >  			if (error)
> > >  				break;
> > > +			files_created++;
> > >  		}
> > >  		if (error)
> > >  			remove_files(parent, grp);
> > >  	}
> > >  exit:
> > > -	return error;
> > > +	if (error)
> > > +		return error;
> > > +	return files_created;
> > >  }
> > >  
> > >  
> > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > >  		if (update) {
> > >  			kn = kernfs_find_and_get(kobj->sd, grp->name);
> > >  			if (!kn) {
> > > -				pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > -					kobj->name, grp->name);
> > > -				return -EINVAL;
> > > +				kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > +							  S_IRWXU | S_IRUGO | S_IXUGO,
> > > +							  uid, gid, kobj, NULL);
> > > +				if (IS_ERR(kn)) {
> > > +					if (PTR_ERR(kn) == -EEXIST)
> > > +						sysfs_warn_dup(kobj->sd, grp->name);
> > > +					return PTR_ERR(kn);
> > > +				}
> > >  			}
> > >  		} else {
> > >  			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > >  
> > >  	kernfs_get(kn);
> > >  	error = create_files(kn, kobj, uid, gid, grp, update);
> > > -	if (error) {
> > > +	if (error <= 0) {
> > > +		/*
> > > +		 * If an error happened _OR_ if no files were created in the
> > > +		 * attribute group, and we have a name for this group, delete
> > > +		 * the name so there's not an empty directory.
> > > +		 */
> > >  		if (grp->name)
> > >  			kernfs_remove(kn);
> > > +	} else {
> > > +		error = 0;
> > > +		kernfs_put(kn);
> > >  	}
> > > -	kernfs_put(kn);
> > >  
> > >  	if (grp->name && update)
> > >  		kernfs_put(kn);
> > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > >  
> > >  /**
> > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > + * sysfs_merge_group - merge files into a attribute group.
> > >   * @kobj:	The kobject containing the group.
> > >   * @grp:	The files to create and the attribute group they belong to.
> > >   *
> > > - * 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.
> > > + * This function returns an error if any of the files already exist in
> > > + * that group, in which case none of the new files are created.
> > >   */
> > >  int sysfs_merge_group(struct kobject *kobj,
> > >  		       const struct attribute_group *grp)
> > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > >  	struct attribute *const *attr;
> > >  	int i;
> > >  
> > > -	parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > -	if (!parent)
> > > -		return -ENOENT;
> > > -
> > >  	kobject_get_ownership(kobj, &uid, &gid);
> > >  
> > > +	parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > +	if (!parent) {
> > > +		parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > +					  S_IRWXU | S_IRUGO | S_IXUGO,
> > > +					  uid, gid, kobj, NULL);
> > > +		if (IS_ERR(parent)) {
> > > +			if (PTR_ERR(parent) == -EEXIST)
> > > +				sysfs_warn_dup(kobj->sd, grp->name);
> > > +			return PTR_ERR(parent);
> > > +		}
> > > +
> > > +		kernfs_get(parent);
> > > +	}
> > > +
> > >  	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > >  		error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > >  					       uid, gid, NULL);
> > > -- 
> > > 2.42.0
> > > 
> > 
> > And as the 0-day bot just showed, this patch isn't going to work
> > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > fixing that up when I get some cycles...
> 
> Oops, nope, that was my fault in applying this to my tree, sorry for the
> noise...

And I just got around to testing this, and it does not boot at all.
Below is the patch I am using, are you sure you got this to boot for
you?

thanks,

greg k-h

From db537de5a5af649b594604e2de177527f890878d Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Wed, 24 Aug 2022 15:45:36 +0200
Subject: [PATCH] sysfs: do not create empty directories if no attributes are
 present

When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
were successfully created in the group at all.

Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 fs/sysfs/file.c  | 16 +++++++++++---
 fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index a12ac0356c69..9f79ec193ffe 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -391,10 +391,20 @@ int sysfs_add_file_to_group(struct kobject *kobj,
 		kernfs_get(parent);
 	}
 
-	if (!parent)
-		return -ENOENT;
-
 	kobject_get_ownership(kobj, &uid, &gid);
+	if (!parent) {
+		parent = kernfs_create_dir_ns(kobj->sd, group,
+					  S_IRWXU | S_IRUGO | S_IXUGO,
+					  uid, gid, kobj, NULL);
+		if (IS_ERR(parent)) {
+			if (PTR_ERR(parent) == -EEXIST)
+				sysfs_warn_dup(kobj->sd, group);
+			return PTR_ERR(parent);
+		}
+
+		kernfs_get(parent);
+	}
+
 	error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
 				       NULL);
 	kernfs_put(parent);
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..013fa333cd3c 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
 			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
+/* returns -ERROR if error, or >= 0 for number of files actually created */
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 			kuid_t uid, kgid_t gid,
 			const struct attribute_group *grp, int update)
 {
 	struct attribute *const *attr;
 	struct bin_attribute *const *bin_attr;
+	int files_created = 0;
 	int error = 0, i;
 
 	if (grp->attrs) {
@@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 						       gid, NULL);
 			if (unlikely(error))
 				break;
+
+			files_created++;
 		}
 		if (error) {
 			remove_files(parent, grp);
@@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 							   NULL);
 			if (error)
 				break;
+			files_created++;
 		}
 		if (error)
 			remove_files(parent, grp);
 	}
 exit:
-	return error;
+	if (error)
+		return error;
+	return files_created;
 }
 
 
@@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				kn = kernfs_create_dir_ns(kobj->sd, grp->name,
+							  S_IRWXU | S_IRUGO | S_IXUGO,
+							  uid, gid, kobj, NULL);
+				if (IS_ERR(kn)) {
+					if (PTR_ERR(kn) == -EEXIST)
+						sysfs_warn_dup(kobj->sd, grp->name);
+					return PTR_ERR(kn);
+				}
 			}
 		} else {
 			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
@@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kernfs_get(kn);
 	error = create_files(kn, kobj, uid, gid, grp, update);
-	if (error) {
+	if (error <= 0) {
+		/*
+		 * If an error happened _OR_ if no files were created in the
+		 * attribute group, and we have a name for this group, delete
+		 * the name so there's not an empty directory.
+		 */
 		if (grp->name)
 			kernfs_remove(kn);
+	} else {
+		error = 0;
+		kernfs_put(kn);
 	}
-	kernfs_put(kn);
 
 	if (grp->name && update)
 		kernfs_put(kn);
@@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
 EXPORT_SYMBOL_GPL(sysfs_remove_groups);
 
 /**
- * sysfs_merge_group - merge files into a pre-existing attribute group.
+ * sysfs_merge_group - merge files into a attribute group.
  * @kobj:	The kobject containing the group.
  * @grp:	The files to create and the attribute group they belong to.
  *
- * 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.
+ * This function returns an error if any of the files already exist in
+ * that group, in which case none of the new files are created.
  */
 int sysfs_merge_group(struct kobject *kobj,
 		       const struct attribute_group *grp)
@@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
 	struct attribute *const *attr;
 	int i;
 
-	parent = kernfs_find_and_get(kobj->sd, grp->name);
-	if (!parent)
-		return -ENOENT;
-
 	kobject_get_ownership(kobj, &uid, &gid);
 
+	parent = kernfs_find_and_get(kobj->sd, grp->name);
+	if (!parent) {
+		parent = kernfs_create_dir_ns(kobj->sd, grp->name,
+					  S_IRWXU | S_IRUGO | S_IXUGO,
+					  uid, gid, kobj, NULL);
+		if (IS_ERR(parent)) {
+			if (PTR_ERR(parent) == -EEXIST)
+				sysfs_warn_dup(kobj->sd, grp->name);
+			return PTR_ERR(parent);
+		}
+
+		kernfs_get(parent);
+	}
+
 	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
 		error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
 					       uid, gid, NULL);

On Thu, Oct 5, 2023 at 11:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> > On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > > manually add this type of attribute.
> > > > > > > >
> > > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > > >
> > > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > > work, it's really the correct thing to do here."
> > > > > > >
> > > > > > > I did try!
> > > > > > >
> > > > > > > This is an attempt:
> > > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > > >
> > > > > > > It is based on your original patch with a bunch of:
> > > > > > >
> > > > > > > if (!parent) {
> > > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > >                   uid, gid, kobj, NULL);
> > > > > > >     ...
> > > > > > >     }
> > > > > > >
> > > > > > >
> > > > > > > added throughout the code base.
> > > > > > >
> > > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > > see any kernel oops on boot.
> > > > > >
> > > > > > Nice!
> > > > > >
> > > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > > need this functionality for other stuff as well, so getting it merged
> > > > > > for your feature is fine with me.
> > > > >
> > > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > >
> > > > > >
> > > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > > use that instead
> > > > > >
> > > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > > me to.
> > > > >
> > > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > > finish up the patch and send it with this series
> > > >
> > > > Great, and for the email record, as github links are not stable, here's
> > > > the patch that you have above attached below.  I'll test this out and
> > > > clean it up a bit more and see how it goes...
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > >
> > > >
> > > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > > >  present
> > > >
> > > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > > were successfully created in the group at all.
> > > >
> > > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >  fs/sysfs/file.c  | 14 ++++++++++--
> > > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > > index a12ac0356c69..7aab6c09662c 100644
> > > > --- a/fs/sysfs/file.c
> > > > +++ b/fs/sysfs/file.c
> > > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > > >           kernfs_get(parent);
> > > >   }
> > > >
> > > > - if (!parent)
> > > > -         return -ENOENT;
> > > > + if (!parent) {
> > > > +         parent = kernfs_create_dir_ns(kobj->sd, group,
> > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > +                                   uid, gid, kobj, NULL);
> > > > +         if (IS_ERR(parent)) {
> > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > +                         sysfs_warn_dup(kobj->sd, group);
> > > > +                 return PTR_ERR(parent);
> > > > +         }
> > > > +
> > > > +         kernfs_get(parent);
> > > > + }
> > > >
> > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > >   error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > index 138676463336..013fa333cd3c 100644
> > > > --- a/fs/sysfs/group.c
> > > > +++ b/fs/sysfs/group.c
> > > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > > >                   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > > >  }
> > > >
> > > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > >                   kuid_t uid, kgid_t gid,
> > > >                   const struct attribute_group *grp, int update)
> > > >  {
> > > >   struct attribute *const *attr;
> > > >   struct bin_attribute *const *bin_attr;
> > > > + int files_created = 0;
> > > >   int error = 0, i;
> > > >
> > > >   if (grp->attrs) {
> > > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > >                                                  gid, NULL);
> > > >                   if (unlikely(error))
> > > >                           break;
> > > > +
> > > > +                 files_created++;
> > > >           }
> > > >           if (error) {
> > > >                   remove_files(parent, grp);
> > > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > >                                                      NULL);
> > > >                   if (error)
> > > >                           break;
> > > > +                 files_created++;
> > > >           }
> > > >           if (error)
> > > >                   remove_files(parent, grp);
> > > >   }
> > > >  exit:
> > > > - return error;
> > > > + if (error)
> > > > +         return error;
> > > > + return files_created;
> > > >  }
> > > >
> > > >
> > > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > >           if (update) {
> > > >                   kn = kernfs_find_and_get(kobj->sd, grp->name);
> > > >                   if (!kn) {
> > > > -                         pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > > -                                 kobj->name, grp->name);
> > > > -                         return -EINVAL;
> > > > +                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > +                                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > +                                                   uid, gid, kobj, NULL);
> > > > +                         if (IS_ERR(kn)) {
> > > > +                                 if (PTR_ERR(kn) == -EEXIST)
> > > > +                                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > +                                 return PTR_ERR(kn);
> > > > +                         }
> > > >                   }
> > > >           } else {
> > > >                   kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > >
> > > >   kernfs_get(kn);
> > > >   error = create_files(kn, kobj, uid, gid, grp, update);
> > > > - if (error) {
> > > > + if (error <= 0) {
> > > > +         /*
> > > > +          * If an error happened _OR_ if no files were created in the
> > > > +          * attribute group, and we have a name for this group, delete
> > > > +          * the name so there's not an empty directory.
> > > > +          */
> > > >           if (grp->name)
> > > >                   kernfs_remove(kn);
> > > > + } else {
> > > > +         error = 0;
> > > > +         kernfs_put(kn);
> > > >   }
> > > > - kernfs_put(kn);
> > > >
> > > >   if (grp->name && update)
> > > >           kernfs_put(kn);
> > > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > > >
> > > >  /**
> > > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > > + * sysfs_merge_group - merge files into a attribute group.
> > > >   * @kobj:        The kobject containing the group.
> > > >   * @grp: The files to create and the attribute group they belong to.
> > > >   *
> > > > - * 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.
> > > > + * This function returns an error if any of the files already exist in
> > > > + * that group, in which case none of the new files are created.
> > > >   */
> > > >  int sysfs_merge_group(struct kobject *kobj,
> > > >                  const struct attribute_group *grp)
> > > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > > >   struct attribute *const *attr;
> > > >   int i;
> > > >
> > > > - parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > - if (!parent)
> > > > -         return -ENOENT;
> > > > -
> > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > >
> > > > + parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > + if (!parent) {
> > > > +         parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > +                                   uid, gid, kobj, NULL);
> > > > +         if (IS_ERR(parent)) {
> > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > +                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > +                 return PTR_ERR(parent);
> > > > +         }
> > > > +
> > > > +         kernfs_get(parent);
> > > > + }
> > > > +
> > > >   for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > > >           error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > > >                                          uid, gid, NULL);
> > > > --
> > > > 2.42.0
> > > >
> > >
> > > And as the 0-day bot just showed, this patch isn't going to work
> > > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > > fixing that up when I get some cycles...
> >
> > Oops, nope, that was my fault in applying this to my tree, sorry for the
> > noise...
>
> And I just got around to testing this, and it does not boot at all.
> Below is the patch I am using, are you sure you got this to boot for
> you?

I just checked again and it boots for me. What failure are you seeing?

Alistair

>
> thanks,
>
> greg k-h
>
> From db537de5a5af649b594604e2de177527f890878d Mon Sep 17 00:00:00 2001
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date: Wed, 24 Aug 2022 15:45:36 +0200
> Subject: [PATCH] sysfs: do not create empty directories if no attributes are
>  present
>
> When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> were successfully created in the group at all.
>
> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  fs/sysfs/file.c  | 16 +++++++++++---
>  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index a12ac0356c69..9f79ec193ffe 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -391,10 +391,20 @@ int sysfs_add_file_to_group(struct kobject *kobj,
>                 kernfs_get(parent);
>         }
>
> -       if (!parent)
> -               return -ENOENT;
> -
>         kobject_get_ownership(kobj, &uid, &gid);
> +       if (!parent) {
> +               parent = kernfs_create_dir_ns(kobj->sd, group,
> +                                         S_IRWXU | S_IRUGO | S_IXUGO,
> +                                         uid, gid, kobj, NULL);
> +               if (IS_ERR(parent)) {
> +                       if (PTR_ERR(parent) == -EEXIST)
> +                               sysfs_warn_dup(kobj->sd, group);
> +                       return PTR_ERR(parent);
> +               }
> +
> +               kernfs_get(parent);
> +       }
> +
>         error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
>                                        NULL);
>         kernfs_put(parent);
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 138676463336..013fa333cd3c 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
>                         kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
>  }
>
> +/* returns -ERROR if error, or >= 0 for number of files actually created */
>  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>                         kuid_t uid, kgid_t gid,
>                         const struct attribute_group *grp, int update)
>  {
>         struct attribute *const *attr;
>         struct bin_attribute *const *bin_attr;
> +       int files_created = 0;
>         int error = 0, i;
>
>         if (grp->attrs) {
> @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>                                                        gid, NULL);
>                         if (unlikely(error))
>                                 break;
> +
> +                       files_created++;
>                 }
>                 if (error) {
>                         remove_files(parent, grp);
> @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>                                                            NULL);
>                         if (error)
>                                 break;
> +                       files_created++;
>                 }
>                 if (error)
>                         remove_files(parent, grp);
>         }
>  exit:
> -       return error;
> +       if (error)
> +               return error;
> +       return files_created;
>  }
>
>
> @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
>                 if (update) {
>                         kn = kernfs_find_and_get(kobj->sd, grp->name);
>                         if (!kn) {
> -                               pr_warn("Can't update unknown attr grp name: %s/%s\n",
> -                                       kobj->name, grp->name);
> -                               return -EINVAL;
> +                               kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> +                                                         S_IRWXU | S_IRUGO | S_IXUGO,
> +                                                         uid, gid, kobj, NULL);
> +                               if (IS_ERR(kn)) {
> +                                       if (PTR_ERR(kn) == -EEXIST)
> +                                               sysfs_warn_dup(kobj->sd, grp->name);
> +                                       return PTR_ERR(kn);
> +                               }
>                         }
>                 } else {
>                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
>
>         kernfs_get(kn);
>         error = create_files(kn, kobj, uid, gid, grp, update);
> -       if (error) {
> +       if (error <= 0) {
> +               /*
> +                * If an error happened _OR_ if no files were created in the
> +                * attribute group, and we have a name for this group, delete
> +                * the name so there's not an empty directory.
> +                */
>                 if (grp->name)
>                         kernfs_remove(kn);
> +       } else {
> +               error = 0;
> +               kernfs_put(kn);
>         }
> -       kernfs_put(kn);
>
>         if (grp->name && update)
>                 kernfs_put(kn);
> @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
>  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
>
>  /**
> - * sysfs_merge_group - merge files into a pre-existing attribute group.
> + * sysfs_merge_group - merge files into a attribute group.
>   * @kobj:      The kobject containing the group.
>   * @grp:       The files to create and the attribute group they belong to.
>   *
> - * 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.
> + * This function returns an error if any of the files already exist in
> + * that group, in which case none of the new files are created.
>   */
>  int sysfs_merge_group(struct kobject *kobj,
>                        const struct attribute_group *grp)
> @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
>         struct attribute *const *attr;
>         int i;
>
> -       parent = kernfs_find_and_get(kobj->sd, grp->name);
> -       if (!parent)
> -               return -ENOENT;
> -
>         kobject_get_ownership(kobj, &uid, &gid);
>
> +       parent = kernfs_find_and_get(kobj->sd, grp->name);
> +       if (!parent) {
> +               parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> +                                         S_IRWXU | S_IRUGO | S_IXUGO,
> +                                         uid, gid, kobj, NULL);
> +               if (IS_ERR(parent)) {
> +                       if (PTR_ERR(parent) == -EEXIST)
> +                               sysfs_warn_dup(kobj->sd, grp->name);
> +                       return PTR_ERR(parent);
> +               }
> +
> +               kernfs_get(parent);
> +       }
> +
>         for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
>                 error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
>                                                uid, gid, NULL);
> --
> 2.42.0
>

On Wed, Oct 11, 2023 at 03:10:39PM +1000, Alistair Francis wrote:
> On Thu, Oct 5, 2023 at 11:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> > > On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > > > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > > > manually add this type of attribute.
> > > > > > > > >
> > > > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > > > >
> > > > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > > > work, it's really the correct thing to do here."
> > > > > > > >
> > > > > > > > I did try!
> > > > > > > >
> > > > > > > > This is an attempt:
> > > > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > > > >
> > > > > > > > It is based on your original patch with a bunch of:
> > > > > > > >
> > > > > > > > if (!parent) {
> > > > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > >                   uid, gid, kobj, NULL);
> > > > > > > >     ...
> > > > > > > >     }
> > > > > > > >
> > > > > > > >
> > > > > > > > added throughout the code base.
> > > > > > > >
> > > > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > > > see any kernel oops on boot.
> > > > > > >
> > > > > > > Nice!
> > > > > > >
> > > > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > > > need this functionality for other stuff as well, so getting it merged
> > > > > > > for your feature is fine with me.
> > > > > >
> > > > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > > >
> > > > > > >
> > > > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > > > use that instead
> > > > > > >
> > > > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > > > me to.
> > > > > >
> > > > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > > > finish up the patch and send it with this series
> > > > >
> > > > > Great, and for the email record, as github links are not stable, here's
> > > > > the patch that you have above attached below.  I'll test this out and
> > > > > clean it up a bit more and see how it goes...
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > > >
> > > > >
> > > > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > > > >  present
> > > > >
> > > > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > > > were successfully created in the group at all.
> > > > >
> > > > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > >  fs/sysfs/file.c  | 14 ++++++++++--
> > > > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > > > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > > > index a12ac0356c69..7aab6c09662c 100644
> > > > > --- a/fs/sysfs/file.c
> > > > > +++ b/fs/sysfs/file.c
> > > > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > > > >           kernfs_get(parent);
> > > > >   }
> > > > >
> > > > > - if (!parent)
> > > > > -         return -ENOENT;
> > > > > + if (!parent) {
> > > > > +         parent = kernfs_create_dir_ns(kobj->sd, group,
> > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > +                                   uid, gid, kobj, NULL);
> > > > > +         if (IS_ERR(parent)) {
> > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > +                         sysfs_warn_dup(kobj->sd, group);
> > > > > +                 return PTR_ERR(parent);
> > > > > +         }
> > > > > +
> > > > > +         kernfs_get(parent);
> > > > > + }
> > > > >
> > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > >   error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > > index 138676463336..013fa333cd3c 100644
> > > > > --- a/fs/sysfs/group.c
> > > > > +++ b/fs/sysfs/group.c
> > > > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > > > >                   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > > > >  }
> > > > >
> > > > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > > > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > >                   kuid_t uid, kgid_t gid,
> > > > >                   const struct attribute_group *grp, int update)
> > > > >  {
> > > > >   struct attribute *const *attr;
> > > > >   struct bin_attribute *const *bin_attr;
> > > > > + int files_created = 0;
> > > > >   int error = 0, i;
> > > > >
> > > > >   if (grp->attrs) {
> > > > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > >                                                  gid, NULL);
> > > > >                   if (unlikely(error))
> > > > >                           break;
> > > > > +
> > > > > +                 files_created++;
> > > > >           }
> > > > >           if (error) {
> > > > >                   remove_files(parent, grp);
> > > > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > >                                                      NULL);
> > > > >                   if (error)
> > > > >                           break;
> > > > > +                 files_created++;
> > > > >           }
> > > > >           if (error)
> > > > >                   remove_files(parent, grp);
> > > > >   }
> > > > >  exit:
> > > > > - return error;
> > > > > + if (error)
> > > > > +         return error;
> > > > > + return files_created;
> > > > >  }
> > > > >
> > > > >
> > > > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > >           if (update) {
> > > > >                   kn = kernfs_find_and_get(kobj->sd, grp->name);
> > > > >                   if (!kn) {
> > > > > -                         pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > > > -                                 kobj->name, grp->name);
> > > > > -                         return -EINVAL;
> > > > > +                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > +                                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > +                                                   uid, gid, kobj, NULL);
> > > > > +                         if (IS_ERR(kn)) {
> > > > > +                                 if (PTR_ERR(kn) == -EEXIST)
> > > > > +                                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > +                                 return PTR_ERR(kn);
> > > > > +                         }
> > > > >                   }
> > > > >           } else {
> > > > >                   kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > >
> > > > >   kernfs_get(kn);
> > > > >   error = create_files(kn, kobj, uid, gid, grp, update);
> > > > > - if (error) {
> > > > > + if (error <= 0) {
> > > > > +         /*
> > > > > +          * If an error happened _OR_ if no files were created in the
> > > > > +          * attribute group, and we have a name for this group, delete
> > > > > +          * the name so there's not an empty directory.
> > > > > +          */
> > > > >           if (grp->name)
> > > > >                   kernfs_remove(kn);
> > > > > + } else {
> > > > > +         error = 0;
> > > > > +         kernfs_put(kn);
> > > > >   }
> > > > > - kernfs_put(kn);
> > > > >
> > > > >   if (grp->name && update)
> > > > >           kernfs_put(kn);
> > > > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > > > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > > > >
> > > > >  /**
> > > > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > > > + * sysfs_merge_group - merge files into a attribute group.
> > > > >   * @kobj:        The kobject containing the group.
> > > > >   * @grp: The files to create and the attribute group they belong to.
> > > > >   *
> > > > > - * 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.
> > > > > + * This function returns an error if any of the files already exist in
> > > > > + * that group, in which case none of the new files are created.
> > > > >   */
> > > > >  int sysfs_merge_group(struct kobject *kobj,
> > > > >                  const struct attribute_group *grp)
> > > > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > > > >   struct attribute *const *attr;
> > > > >   int i;
> > > > >
> > > > > - parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > - if (!parent)
> > > > > -         return -ENOENT;
> > > > > -
> > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > >
> > > > > + parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > + if (!parent) {
> > > > > +         parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > +                                   uid, gid, kobj, NULL);
> > > > > +         if (IS_ERR(parent)) {
> > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > +                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > +                 return PTR_ERR(parent);
> > > > > +         }
> > > > > +
> > > > > +         kernfs_get(parent);
> > > > > + }
> > > > > +
> > > > >   for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > > > >           error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > > > >                                          uid, gid, NULL);
> > > > > --
> > > > > 2.42.0
> > > > >
> > > >
> > > > And as the 0-day bot just showed, this patch isn't going to work
> > > > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > > > fixing that up when I get some cycles...
> > >
> > > Oops, nope, that was my fault in applying this to my tree, sorry for the
> > > noise...
> >
> > And I just got around to testing this, and it does not boot at all.
> > Below is the patch I am using, are you sure you got this to boot for
> > you?
> 
> I just checked again and it boots for me. What failure are you seeing?

Block devices were not created properly in sysfs.

Did you test your patch, or my modified one that I attached on this
email?  That might be the difference.

thanks,

greg k-h

On Wed, Oct 11, 2023 at 4:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 11, 2023 at 03:10:39PM +1000, Alistair Francis wrote:
> > On Thu, Oct 5, 2023 at 11:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> > > > On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > > > > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > > > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > > > > manually add this type of attribute.
> > > > > > > > > >
> > > > > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > > > > >
> > > > > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > > > > work, it's really the correct thing to do here."
> > > > > > > > >
> > > > > > > > > I did try!
> > > > > > > > >
> > > > > > > > > This is an attempt:
> > > > > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > > > > >
> > > > > > > > > It is based on your original patch with a bunch of:
> > > > > > > > >
> > > > > > > > > if (!parent) {
> > > > > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > >                   uid, gid, kobj, NULL);
> > > > > > > > >     ...
> > > > > > > > >     }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > added throughout the code base.
> > > > > > > > >
> > > > > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > > > > see any kernel oops on boot.
> > > > > > > >
> > > > > > > > Nice!
> > > > > > > >
> > > > > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > > > > need this functionality for other stuff as well, so getting it merged
> > > > > > > > for your feature is fine with me.
> > > > > > >
> > > > > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > > > >
> > > > > > > >
> > > > > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > > > > use that instead
> > > > > > > >
> > > > > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > > > > me to.
> > > > > > >
> > > > > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > > > > finish up the patch and send it with this series
> > > > > >
> > > > > > Great, and for the email record, as github links are not stable, here's
> > > > > > the patch that you have above attached below.  I'll test this out and
> > > > > > clean it up a bit more and see how it goes...
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > > >
> > > > > >
> > > > > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > > > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > > > > >  present
> > > > > >
> > > > > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > > > > were successfully created in the group at all.
> > > > > >
> > > > > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > >  fs/sysfs/file.c  | 14 ++++++++++--
> > > > > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > > > > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > > > > index a12ac0356c69..7aab6c09662c 100644
> > > > > > --- a/fs/sysfs/file.c
> > > > > > +++ b/fs/sysfs/file.c
> > > > > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > > > > >           kernfs_get(parent);
> > > > > >   }
> > > > > >
> > > > > > - if (!parent)
> > > > > > -         return -ENOENT;
> > > > > > + if (!parent) {
> > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, group,
> > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > +         if (IS_ERR(parent)) {
> > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > +                         sysfs_warn_dup(kobj->sd, group);
> > > > > > +                 return PTR_ERR(parent);
> > > > > > +         }
> > > > > > +
> > > > > > +         kernfs_get(parent);
> > > > > > + }
> > > > > >
> > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > >   error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > > > index 138676463336..013fa333cd3c 100644
> > > > > > --- a/fs/sysfs/group.c
> > > > > > +++ b/fs/sysfs/group.c
> > > > > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > > > > >                   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > > > > >  }
> > > > > >
> > > > > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > > > > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > >                   kuid_t uid, kgid_t gid,
> > > > > >                   const struct attribute_group *grp, int update)
> > > > > >  {
> > > > > >   struct attribute *const *attr;
> > > > > >   struct bin_attribute *const *bin_attr;
> > > > > > + int files_created = 0;
> > > > > >   int error = 0, i;
> > > > > >
> > > > > >   if (grp->attrs) {
> > > > > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > >                                                  gid, NULL);
> > > > > >                   if (unlikely(error))
> > > > > >                           break;
> > > > > > +
> > > > > > +                 files_created++;
> > > > > >           }
> > > > > >           if (error) {
> > > > > >                   remove_files(parent, grp);
> > > > > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > >                                                      NULL);
> > > > > >                   if (error)
> > > > > >                           break;
> > > > > > +                 files_created++;
> > > > > >           }
> > > > > >           if (error)
> > > > > >                   remove_files(parent, grp);
> > > > > >   }
> > > > > >  exit:
> > > > > > - return error;
> > > > > > + if (error)
> > > > > > +         return error;
> > > > > > + return files_created;
> > > > > >  }
> > > > > >
> > > > > >
> > > > > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > >           if (update) {
> > > > > >                   kn = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > >                   if (!kn) {
> > > > > > -                         pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > > > > -                                 kobj->name, grp->name);
> > > > > > -                         return -EINVAL;
> > > > > > +                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > +                                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > +                                                   uid, gid, kobj, NULL);
> > > > > > +                         if (IS_ERR(kn)) {
> > > > > > +                                 if (PTR_ERR(kn) == -EEXIST)
> > > > > > +                                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > +                                 return PTR_ERR(kn);
> > > > > > +                         }
> > > > > >                   }
> > > > > >           } else {
> > > > > >                   kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > >
> > > > > >   kernfs_get(kn);
> > > > > >   error = create_files(kn, kobj, uid, gid, grp, update);
> > > > > > - if (error) {
> > > > > > + if (error <= 0) {
> > > > > > +         /*
> > > > > > +          * If an error happened _OR_ if no files were created in the
> > > > > > +          * attribute group, and we have a name for this group, delete
> > > > > > +          * the name so there's not an empty directory.
> > > > > > +          */
> > > > > >           if (grp->name)
> > > > > >                   kernfs_remove(kn);
> > > > > > + } else {
> > > > > > +         error = 0;
> > > > > > +         kernfs_put(kn);
> > > > > >   }
> > > > > > - kernfs_put(kn);
> > > > > >
> > > > > >   if (grp->name && update)
> > > > > >           kernfs_put(kn);
> > > > > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > > > > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > > > > >
> > > > > >  /**
> > > > > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > > > > + * sysfs_merge_group - merge files into a attribute group.
> > > > > >   * @kobj:        The kobject containing the group.
> > > > > >   * @grp: The files to create and the attribute group they belong to.
> > > > > >   *
> > > > > > - * 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.
> > > > > > + * This function returns an error if any of the files already exist in
> > > > > > + * that group, in which case none of the new files are created.
> > > > > >   */
> > > > > >  int sysfs_merge_group(struct kobject *kobj,
> > > > > >                  const struct attribute_group *grp)
> > > > > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > > > > >   struct attribute *const *attr;
> > > > > >   int i;
> > > > > >
> > > > > > - parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > - if (!parent)
> > > > > > -         return -ENOENT;
> > > > > > -
> > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > >
> > > > > > + parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > + if (!parent) {
> > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > +         if (IS_ERR(parent)) {
> > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > +                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > +                 return PTR_ERR(parent);
> > > > > > +         }
> > > > > > +
> > > > > > +         kernfs_get(parent);
> > > > > > + }
> > > > > > +
> > > > > >   for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > > > > >           error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > > > > >                                          uid, gid, NULL);
> > > > > > --
> > > > > > 2.42.0
> > > > > >
> > > > >
> > > > > And as the 0-day bot just showed, this patch isn't going to work
> > > > > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > > > > fixing that up when I get some cycles...
> > > >
> > > > Oops, nope, that was my fault in applying this to my tree, sorry for the
> > > > noise...
> > >
> > > And I just got around to testing this, and it does not boot at all.
> > > Below is the patch I am using, are you sure you got this to boot for
> > > you?
> >
> > I just checked again and it boots for me. What failure are you seeing?
>
> Block devices were not created properly in sysfs.

Strange. I have tested this on a QEMU virtual machine and a x86 PC and
I don't see any issues on either.

>
> Did you test your patch, or my modified one that I attached on this
> email?  That might be the difference.

I'm using your modified one.

Are you able to provide logs? There must be some driver that is
causing the issue. I can try to reproduce it if I know where it's
failing.

Alistair

>
> thanks,
>
> greg k-h

On Thu, Oct 12, 2023 at 2:31 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Oct 11, 2023 at 4:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 11, 2023 at 03:10:39PM +1000, Alistair Francis wrote:
> > > On Thu, Oct 5, 2023 at 11:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> > > > > On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > > > > > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > > > > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > > > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > >
> > > > > > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > > > > > manually add this type of attribute.
> > > > > > > > > > >
> > > > > > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > > > > > >
> > > > > > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > > > > > work, it's really the correct thing to do here."
> > > > > > > > > >
> > > > > > > > > > I did try!
> > > > > > > > > >
> > > > > > > > > > This is an attempt:
> > > > > > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > > > > > >
> > > > > > > > > > It is based on your original patch with a bunch of:
> > > > > > > > > >
> > > > > > > > > > if (!parent) {
> > > > > > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > > >                   uid, gid, kobj, NULL);
> > > > > > > > > >     ...
> > > > > > > > > >     }
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > added throughout the code base.
> > > > > > > > > >
> > > > > > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > > > > > see any kernel oops on boot.
> > > > > > > > >
> > > > > > > > > Nice!
> > > > > > > > >
> > > > > > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > > > > > need this functionality for other stuff as well, so getting it merged
> > > > > > > > > for your feature is fine with me.
> > > > > > > >
> > > > > > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > > > > > use that instead
> > > > > > > > >
> > > > > > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > > > > > me to.
> > > > > > > >
> > > > > > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > > > > > finish up the patch and send it with this series
> > > > > > >
> > > > > > > Great, and for the email record, as github links are not stable, here's
> > > > > > > the patch that you have above attached below.  I'll test this out and
> > > > > > > clean it up a bit more and see how it goes...
> > > > > > >
> > > > > > > thanks,
> > > > > > >
> > > > > > > greg k-h
> > > > > > >
> > > > > > >
> > > > > > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > > > > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > > > > > >  present
> > > > > > >
> > > > > > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > > > > > were successfully created in the group at all.
> > > > > > >
> > > > > > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > > ---
> > > > > > >  fs/sysfs/file.c  | 14 ++++++++++--
> > > > > > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > > > > > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > > > > > index a12ac0356c69..7aab6c09662c 100644
> > > > > > > --- a/fs/sysfs/file.c
> > > > > > > +++ b/fs/sysfs/file.c
> > > > > > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > > > > > >           kernfs_get(parent);
> > > > > > >   }
> > > > > > >
> > > > > > > - if (!parent)
> > > > > > > -         return -ENOENT;
> > > > > > > + if (!parent) {
> > > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, group,
> > > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > > +         if (IS_ERR(parent)) {
> > > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > > +                         sysfs_warn_dup(kobj->sd, group);
> > > > > > > +                 return PTR_ERR(parent);
> > > > > > > +         }
> > > > > > > +
> > > > > > > +         kernfs_get(parent);
> > > > > > > + }
> > > > > > >
> > > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > > >   error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > > > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > > > > index 138676463336..013fa333cd3c 100644
> > > > > > > --- a/fs/sysfs/group.c
> > > > > > > +++ b/fs/sysfs/group.c
> > > > > > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > > > > > >                   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > > > > > >  }
> > > > > > >
> > > > > > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > > > > > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > >                   kuid_t uid, kgid_t gid,
> > > > > > >                   const struct attribute_group *grp, int update)
> > > > > > >  {
> > > > > > >   struct attribute *const *attr;
> > > > > > >   struct bin_attribute *const *bin_attr;
> > > > > > > + int files_created = 0;
> > > > > > >   int error = 0, i;
> > > > > > >
> > > > > > >   if (grp->attrs) {
> > > > > > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > >                                                  gid, NULL);
> > > > > > >                   if (unlikely(error))
> > > > > > >                           break;
> > > > > > > +
> > > > > > > +                 files_created++;
> > > > > > >           }
> > > > > > >           if (error) {
> > > > > > >                   remove_files(parent, grp);
> > > > > > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > >                                                      NULL);
> > > > > > >                   if (error)
> > > > > > >                           break;
> > > > > > > +                 files_created++;
> > > > > > >           }
> > > > > > >           if (error)
> > > > > > >                   remove_files(parent, grp);
> > > > > > >   }
> > > > > > >  exit:
> > > > > > > - return error;
> > > > > > > + if (error)
> > > > > > > +         return error;
> > > > > > > + return files_created;
> > > > > > >  }
> > > > > > >
> > > > > > >
> > > > > > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > > >           if (update) {
> > > > > > >                   kn = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > >                   if (!kn) {
> > > > > > > -                         pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > > > > > -                                 kobj->name, grp->name);
> > > > > > > -                         return -EINVAL;
> > > > > > > +                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > +                                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > +                                                   uid, gid, kobj, NULL);
> > > > > > > +                         if (IS_ERR(kn)) {
> > > > > > > +                                 if (PTR_ERR(kn) == -EEXIST)
> > > > > > > +                                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > > +                                 return PTR_ERR(kn);
> > > > > > > +                         }
> > > > > > >                   }
> > > > > > >           } else {
> > > > > > >                   kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > > >
> > > > > > >   kernfs_get(kn);
> > > > > > >   error = create_files(kn, kobj, uid, gid, grp, update);
> > > > > > > - if (error) {
> > > > > > > + if (error <= 0) {
> > > > > > > +         /*
> > > > > > > +          * If an error happened _OR_ if no files were created in the
> > > > > > > +          * attribute group, and we have a name for this group, delete
> > > > > > > +          * the name so there's not an empty directory.
> > > > > > > +          */
> > > > > > >           if (grp->name)
> > > > > > >                   kernfs_remove(kn);
> > > > > > > + } else {
> > > > > > > +         error = 0;
> > > > > > > +         kernfs_put(kn);
> > > > > > >   }
> > > > > > > - kernfs_put(kn);
> > > > > > >
> > > > > > >   if (grp->name && update)
> > > > > > >           kernfs_put(kn);
> > > > > > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > > > > > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > > > > > >
> > > > > > >  /**
> > > > > > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > > > > > + * sysfs_merge_group - merge files into a attribute group.
> > > > > > >   * @kobj:        The kobject containing the group.
> > > > > > >   * @grp: The files to create and the attribute group they belong to.
> > > > > > >   *
> > > > > > > - * 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.
> > > > > > > + * This function returns an error if any of the files already exist in
> > > > > > > + * that group, in which case none of the new files are created.
> > > > > > >   */
> > > > > > >  int sysfs_merge_group(struct kobject *kobj,
> > > > > > >                  const struct attribute_group *grp)
> > > > > > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > > > > > >   struct attribute *const *attr;
> > > > > > >   int i;
> > > > > > >
> > > > > > > - parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > > - if (!parent)
> > > > > > > -         return -ENOENT;
> > > > > > > -
> > > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > > >
> > > > > > > + parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > > + if (!parent) {
> > > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > > +         if (IS_ERR(parent)) {
> > > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > > +                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > > +                 return PTR_ERR(parent);
> > > > > > > +         }
> > > > > > > +
> > > > > > > +         kernfs_get(parent);
> > > > > > > + }
> > > > > > > +
> > > > > > >   for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > > > > > >           error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > > > > > >                                          uid, gid, NULL);
> > > > > > > --
> > > > > > > 2.42.0
> > > > > > >
> > > > > >
> > > > > > And as the 0-day bot just showed, this patch isn't going to work
> > > > > > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > > > > > fixing that up when I get some cycles...
> > > > >
> > > > > Oops, nope, that was my fault in applying this to my tree, sorry for the
> > > > > noise...
> > > >
> > > > And I just got around to testing this, and it does not boot at all.
> > > > Below is the patch I am using, are you sure you got this to boot for
> > > > you?
> > >
> > > I just checked again and it boots for me. What failure are you seeing?
> >
> > Block devices were not created properly in sysfs.
>
> Strange. I have tested this on a QEMU virtual machine and a x86 PC and
> I don't see any issues on either.
>
> >
> > Did you test your patch, or my modified one that I attached on this
> > email?  That might be the difference.
>
> I'm using your modified one.
>
> Are you able to provide logs? There must be some driver that is
> causing the issue. I can try to reproduce it if I know where it's
> failing.

Hey Greg,

I wanted to follow up on this and see if you are able to provide more
details for reproducing or if you are able to look into it?

Alistair

>
> Alistair
>
> >
> > thanks,
> >
> > greg k-h

On Tue, Jan 23, 2024 at 02:04:14PM +1000, Alistair Francis wrote:
> On Thu, Oct 12, 2023 at 2:31 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Oct 11, 2023 at 4:44 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 03:10:39PM +1000, Alistair Francis wrote:
> > > > On Thu, Oct 5, 2023 at 11:05 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Fri, Sep 01, 2023 at 11:00:59PM +0200, Greg KH wrote:
> > > > > > On Thu, Aug 31, 2023 at 04:36:13PM +0200, Greg KH wrote:
> > > > > > > On Thu, Aug 31, 2023 at 10:31:07AM +0200, Greg KH wrote:
> > > > > > > > On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> > > > > > > > > On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > > > > > > > > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > >
> > > > > > > > > > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > > > > > > > > > much simpler overall to have the sysfs core "do the right thing
> > > > > > > > > > > > automatically" than to force each and every bus/subsystem to have to
> > > > > > > > > > > > manually add this type of attribute.
> > > > > > > > > > > >
> > > > > > > > > > > > Also, on a personal level, I want this function to work as it will allow
> > > > > > > > > > > > us to remove some code in some busses that don't really need to be there
> > > > > > > > > > > > (dynamic creation of some device attributes), which will then also allow
> > > > > > > > > > > > me to remove some apis in the driver core that should not be used at all
> > > > > > > > > > > > anymore (but keep creeping back in as there is still a few users.)
> > > > > > > > > > > >
> > > > > > > > > > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > > > > > > > > > work, it's really the correct thing to do here."
> > > > > > > > > > >
> > > > > > > > > > > I did try!
> > > > > > > > > > >
> > > > > > > > > > > This is an attempt:
> > > > > > > > > > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > > > > > > > > > >
> > > > > > > > > > > It is based on your original patch with a bunch of:
> > > > > > > > > > >
> > > > > > > > > > > if (!parent) {
> > > > > > > > > > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > > > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > > > >                   uid, gid, kobj, NULL);
> > > > > > > > > > >     ...
> > > > > > > > > > >     }
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > added throughout the code base.
> > > > > > > > > > >
> > > > > > > > > > > Very basic testing shows that it does what I need it to do and I don't
> > > > > > > > > > > see any kernel oops on boot.
> > > > > > > > > >
> > > > > > > > > > Nice!
> > > > > > > > > >
> > > > > > > > > > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > > > > > > > > > need this functionality for other stuff as well, so getting it merged
> > > > > > > > > > for your feature is fine with me.
> > > > > > > > >
> > > > > > > > > Sure! Go ahead. Sorry I was travelling last week.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > I prefer the approach I have in this mailing list patch. But if you
> > > > > > > > > > > like the commit mentioned above I can tidy and clean it up and then
> > > > > > > > > > > use that instead
> > > > > > > > > >
> > > > > > > > > > I would rather do it this way.  I can work on this on Friday if you want
> > > > > > > > > > me to.
> > > > > > > > >
> > > > > > > > > Yeah, that's fine with me. If you aren't able to let me know and I can
> > > > > > > > > finish up the patch and send it with this series
> > > > > > > >
> > > > > > > > Great, and for the email record, as github links are not stable, here's
> > > > > > > > the patch that you have above attached below.  I'll test this out and
> > > > > > > > clean it up a bit more and see how it goes...
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > >
> > > > > > > > greg k-h
> > > > > > > >
> > > > > > > >
> > > > > > > > From 2929d17b58d02dcf52d0345fa966c616e09a5afa Mon Sep 17 00:00:00 2001
> > > > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > > Date: Wed, 24 Aug 2022 15:45:36 +0200
> > > > > > > > Subject: [PATCH] sysfs: do not create empty directories if no attributes are
> > > > > > > >  present
> > > > > > > >
> > > > > > > > When creating an attribute group, if it is named a subdirectory 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.  Clean this up by removing the directory if no files
> > > > > > > > were successfully created in the group at all.
> > > > > > > >
> > > > > > > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > > Co-developed-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > > > ---
> > > > > > > >  fs/sysfs/file.c  | 14 ++++++++++--
> > > > > > > >  fs/sysfs/group.c | 56 ++++++++++++++++++++++++++++++++++++------------
> > > > > > > >  2 files changed, 54 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > > > > > > index a12ac0356c69..7aab6c09662c 100644
> > > > > > > > --- a/fs/sysfs/file.c
> > > > > > > > +++ b/fs/sysfs/file.c
> > > > > > > > @@ -391,8 +391,18 @@ int sysfs_add_file_to_group(struct kobject *kobj,
> > > > > > > >           kernfs_get(parent);
> > > > > > > >   }
> > > > > > > >
> > > > > > > > - if (!parent)
> > > > > > > > -         return -ENOENT;
> > > > > > > > + if (!parent) {
> > > > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, group,
> > > > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > > > +         if (IS_ERR(parent)) {
> > > > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > > > +                         sysfs_warn_dup(kobj->sd, group);
> > > > > > > > +                 return PTR_ERR(parent);
> > > > > > > > +         }
> > > > > > > > +
> > > > > > > > +         kernfs_get(parent);
> > > > > > > > + }
> > > > > > > >
> > > > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > > > >   error = sysfs_add_file_mode_ns(parent, attr, attr->mode, uid, gid,
> > > > > > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > > > > > index 138676463336..013fa333cd3c 100644
> > > > > > > > --- a/fs/sysfs/group.c
> > > > > > > > +++ b/fs/sysfs/group.c
> > > > > > > > @@ -31,12 +31,14 @@ static void remove_files(struct kernfs_node *parent,
> > > > > > > >                   kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/* returns -ERROR if error, or >= 0 for number of files actually created */
> > > > > > > >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > > >                   kuid_t uid, kgid_t gid,
> > > > > > > >                   const struct attribute_group *grp, int update)
> > > > > > > >  {
> > > > > > > >   struct attribute *const *attr;
> > > > > > > >   struct bin_attribute *const *bin_attr;
> > > > > > > > + int files_created = 0;
> > > > > > > >   int error = 0, i;
> > > > > > > >
> > > > > > > >   if (grp->attrs) {
> > > > > > > > @@ -65,6 +67,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > > >                                                  gid, NULL);
> > > > > > > >                   if (unlikely(error))
> > > > > > > >                           break;
> > > > > > > > +
> > > > > > > > +                 files_created++;
> > > > > > > >           }
> > > > > > > >           if (error) {
> > > > > > > >                   remove_files(parent, grp);
> > > > > > > > @@ -95,12 +99,15 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > > > > > > >                                                      NULL);
> > > > > > > >                   if (error)
> > > > > > > >                           break;
> > > > > > > > +                 files_created++;
> > > > > > > >           }
> > > > > > > >           if (error)
> > > > > > > >                   remove_files(parent, grp);
> > > > > > > >   }
> > > > > > > >  exit:
> > > > > > > > - return error;
> > > > > > > > + if (error)
> > > > > > > > +         return error;
> > > > > > > > + return files_created;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >
> > > > > > > > @@ -130,9 +137,14 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > > > >           if (update) {
> > > > > > > >                   kn = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > > >                   if (!kn) {
> > > > > > > > -                         pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > > > > > > > -                                 kobj->name, grp->name);
> > > > > > > > -                         return -EINVAL;
> > > > > > > > +                         kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > +                                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > +                                                   uid, gid, kobj, NULL);
> > > > > > > > +                         if (IS_ERR(kn)) {
> > > > > > > > +                                 if (PTR_ERR(kn) == -EEXIST)
> > > > > > > > +                                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > > > +                                 return PTR_ERR(kn);
> > > > > > > > +                         }
> > > > > > > >                   }
> > > > > > > >           } else {
> > > > > > > >                   kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > @@ -150,11 +162,18 @@ static int internal_create_group(struct kobject *kobj, int update,
> > > > > > > >
> > > > > > > >   kernfs_get(kn);
> > > > > > > >   error = create_files(kn, kobj, uid, gid, grp, update);
> > > > > > > > - if (error) {
> > > > > > > > + if (error <= 0) {
> > > > > > > > +         /*
> > > > > > > > +          * If an error happened _OR_ if no files were created in the
> > > > > > > > +          * attribute group, and we have a name for this group, delete
> > > > > > > > +          * the name so there's not an empty directory.
> > > > > > > > +          */
> > > > > > > >           if (grp->name)
> > > > > > > >                   kernfs_remove(kn);
> > > > > > > > + } else {
> > > > > > > > +         error = 0;
> > > > > > > > +         kernfs_put(kn);
> > > > > > > >   }
> > > > > > > > - kernfs_put(kn);
> > > > > > > >
> > > > > > > >   if (grp->name && update)
> > > > > > > >           kernfs_put(kn);
> > > > > > > > @@ -318,13 +337,12 @@ void sysfs_remove_groups(struct kobject *kobj,
> > > > > > > >  EXPORT_SYMBOL_GPL(sysfs_remove_groups);
> > > > > > > >
> > > > > > > >  /**
> > > > > > > > - * sysfs_merge_group - merge files into a pre-existing attribute group.
> > > > > > > > + * sysfs_merge_group - merge files into a attribute group.
> > > > > > > >   * @kobj:        The kobject containing the group.
> > > > > > > >   * @grp: The files to create and the attribute group they belong to.
> > > > > > > >   *
> > > > > > > > - * 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.
> > > > > > > > + * This function returns an error if any of the files already exist in
> > > > > > > > + * that group, in which case none of the new files are created.
> > > > > > > >   */
> > > > > > > >  int sysfs_merge_group(struct kobject *kobj,
> > > > > > > >                  const struct attribute_group *grp)
> > > > > > > > @@ -336,12 +354,22 @@ int sysfs_merge_group(struct kobject *kobj,
> > > > > > > >   struct attribute *const *attr;
> > > > > > > >   int i;
> > > > > > > >
> > > > > > > > - parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > > > - if (!parent)
> > > > > > > > -         return -ENOENT;
> > > > > > > > -
> > > > > > > >   kobject_get_ownership(kobj, &uid, &gid);
> > > > > > > >
> > > > > > > > + parent = kernfs_find_and_get(kobj->sd, grp->name);
> > > > > > > > + if (!parent) {
> > > > > > > > +         parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > > > > > > > +                                   S_IRWXU | S_IRUGO | S_IXUGO,
> > > > > > > > +                                   uid, gid, kobj, NULL);
> > > > > > > > +         if (IS_ERR(parent)) {
> > > > > > > > +                 if (PTR_ERR(parent) == -EEXIST)
> > > > > > > > +                         sysfs_warn_dup(kobj->sd, grp->name);
> > > > > > > > +                 return PTR_ERR(parent);
> > > > > > > > +         }
> > > > > > > > +
> > > > > > > > +         kernfs_get(parent);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > >   for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
> > > > > > > >           error = sysfs_add_file_mode_ns(parent, *attr, (*attr)->mode,
> > > > > > > >                                          uid, gid, NULL);
> > > > > > > > --
> > > > > > > > 2.42.0
> > > > > > > >
> > > > > > >
> > > > > > > And as the 0-day bot just showed, this patch isn't going to work
> > > > > > > properly, the uid/gid stuff isn't all hooked up properly, I'll work on
> > > > > > > fixing that up when I get some cycles...
> > > > > >
> > > > > > Oops, nope, that was my fault in applying this to my tree, sorry for the
> > > > > > noise...
> > > > >
> > > > > And I just got around to testing this, and it does not boot at all.
> > > > > Below is the patch I am using, are you sure you got this to boot for
> > > > > you?
> > > >
> > > > I just checked again and it boots for me. What failure are you seeing?
> > >
> > > Block devices were not created properly in sysfs.
> >
> > Strange. I have tested this on a QEMU virtual machine and a x86 PC and
> > I don't see any issues on either.
> >
> > >
> > > Did you test your patch, or my modified one that I attached on this
> > > email?  That might be the difference.
> >
> > I'm using your modified one.
> >
> > Are you able to provide logs? There must be some driver that is
> > causing the issue. I can try to reproduce it if I know where it's
> > failing.
> 
> Hey Greg,
> 
> I wanted to follow up on this and see if you are able to provide more
> details for reproducing or if you are able to look into it?

Last I tried this, it still crashed and would not boot either on my
laptop or my workstation.  I don't know how it is working properly for
you, what systems have you tried it on?

I'm not going to be able to look at this for many weeks due to
conference stuff, so if you want to take the series and test it and
hopefully catch my error, that would be great, I'd love to move forward
and get this merged someday.

thanks,

greg k-h

Greg KH wrote:
[..]
> > 
> > Hey Greg,
> > 
> > I wanted to follow up on this and see if you are able to provide more
> > details for reproducing or if you are able to look into it?
> 
> Last I tried this, it still crashed and would not boot either on my
> laptop or my workstation.  I don't know how it is working properly for
> you, what systems have you tried it on?
> 
> I'm not going to be able to look at this for many weeks due to
> conference stuff, so if you want to take the series and test it and
> hopefully catch my error, that would be great, I'd love to move forward
> and get this merged someday.

I mentioned to Lukas that I was working on a "sysfs group visibility"
patch and he pointed me to this thread. I will note that I tried to make
the "hide group if all attributes are invisible" approach work, but
reverted to a "new is_group_visible() callback" approach. I did read
through the thread and try to improve the argument in the changelog
accordingly.

I do admit to liking the cleanliness (not touching 'struct
attribute_group') of the "hide if no visible attribute" approch, but see
the criticism of that alternative below, and let me know if it is
convincing. I tested it locally with the following hack to make the
group disappear every other sysfs_update_group() event:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3e817a6f94c6..a5c4e8f3e93b 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2022,9 +2022,20 @@ static umode_t cxl_region_target_visible(struct kobject *kobj,
        return 0;
 }
 
+static umode_t cxl_region_target_group_visible(struct kobject *kobj)
+{
+       static u32 visible;
+
+       if (visible++ & 1)
+               return 0755;
+       return 0;
+}
+
 static const struct attribute_group cxl_region_target_group = {
+       .name = "target_group",
        .attrs = target_attrs,
        .is_visible = cxl_region_target_visible,
+       .is_group_visible = cxl_region_target_group_visible,
 };
 
 static const struct attribute_group *get_cxl_region_target_group(void)


-- >8 --
From 18d6fdf1465f4ce5f561a35797c1313276993af0 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Tue, 23 Jan 2024 20:20:39 -0800
Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups

Add a method to 'struct attribute_group' to determine the visibility of
an entire named sysfs group relative to the state of its parent kobject.

The motivation for this capability is to centralize PCI device
authentication in the PCI core with a named sysfs group while keeping
that group hidden for devices and platforms that do not meet the
requirements. In a PCI topology, most devices will not support
authentication, a small subset will support just PCI CMA (Component
Measurement and Authentication), a smaller subset will support PCI CMA +
PCIe IDE (Link Integrity and Encryption), and only next generation
server hosts will start to include a platform TSM (TEE Security
Manager).

Without this capability the alternatives are:

- Check if all attributes are invisible and if so, hide the directory.
  Beyond trouble getting this to work [1], this is an ABI change for
  scenarios if userspace happens to depend on group visibility absent any
  attributes. I.e. this new capability avoids regression since it does
  not retroactively apply to existing cases.

- Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI
  devices (i.e. for the case when TSM platform support is present, but
  device support is absent). Unfortunate that this will be a vestigial
  empty directory in the vast majority of cases.

- Reintroduce usage of runtime calls to sysfs_{create,remove}_group()
  in the PCI core. Bjorn has already indicated that he does not want to
  see any growth of pci_sysfs_init() [2].

- Drop the named group and simulate a directory by prefixing all
  TSM-related attributes with "tsm_". Unfortunate to not use the naming
  capability of a sysfs group as intended.

The downside of the alternatives seem worse than the maintenance burden
of this new capability. Otherwise, this facility also brings support for
changing permissions on sysfs directories from the 0755 default for
potential cases where only root is expected to be able to enumerate.
That may prove useful as PCI sysfs picks up more security sensitive
enumeration.

The size increase of 'struct attribute_group' is a concern. Longer term
this could be reduced by consolidating the 3 is_visible() callbacks into
one that takes a parameter for "attr", "bin_attr", or "group". For now
the support is gated behind CONFIG_SYSFS_GROUP_VISIBLE so it can be
compiled out when not needed.

Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1]
Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/sysfs/Kconfig      |  3 +++
 fs/sysfs/group.c      | 50 +++++++++++++++++++++++++++++++++++--------
 include/linux/sysfs.h | 34 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 9 deletions(-)

diff --git a/fs/sysfs/Kconfig b/fs/sysfs/Kconfig
index b0fe1cce33a0..d5de8e54a06f 100644
--- a/fs/sysfs/Kconfig
+++ b/fs/sysfs/Kconfig
@@ -23,3 +23,6 @@ config SYSFS
 	example, "root=03:01" for /dev/hda1.
 
 	Designers of embedded systems may wish to say N here to conserve space.
+
+config SYSFS_GROUP_VISIBLE
+	bool
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..0a977064e118 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -127,16 +127,36 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	if (grp->name) {
+		umode_t mode = S_IRWXU | S_IRUGO | S_IXUGO;
+
+		if (has_group_visible(grp))
+			mode = is_group_visible(grp, kobj);
+
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				if (!has_group_visible(grp)) {
+					pr_warn("Can't update unknown attr grp name: %s/%s\n",
+						kobj->name, grp->name);
+					return -EINVAL;
+				}
+				/* may have been invisible prior to this update */
+				update = 0;
+			} else {
+				/* need to change the visibility of the entire group */
+				sysfs_remove_group(kobj, grp);
+				if (mode == 0) {
+					kernfs_put(kn);
+					return 0;
+				}
+				update = 0;
 			}
-		} else {
-			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
-						  S_IRWXU | S_IRUGO | S_IXUGO,
+		}
+
+		if (!update) {
+			if (mode == 0)
+				return 0;
+			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
 						  uid, gid, kobj, NULL);
 			if (IS_ERR(kn)) {
 				if (PTR_ERR(kn) == -EEXIST)
@@ -262,6 +282,20 @@ int sysfs_update_group(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_update_group);
 
+static void warn_group_not_found(const struct attribute_group *grp,
+				 struct kobject *kobj)
+{
+	if (has_group_visible(grp)) {
+		/* may have never been created */
+		pr_debug("sysfs group '%s' not found for kobject '%s'\n",
+			 grp->name, kobject_name(kobj));
+		return;
+	}
+
+	WARN(1, KERN_WARNING "sysfs group '%s' not found for kobject '%s'\n",
+	     grp->name, kobject_name(kobj));
+}
+
 /**
  * sysfs_remove_group: remove a group from a kobject
  * @kobj:	kobject to remove the group from
@@ -279,9 +313,7 @@ void sysfs_remove_group(struct kobject *kobj,
 	if (grp->name) {
 		kn = kernfs_find_and_get(parent, grp->name);
 		if (!kn) {
-			WARN(!kn, KERN_WARNING
-			     "sysfs group '%s' not found for kobject '%s'\n",
-			     grp->name, kobject_name(kobj));
+			warn_group_not_found(grp, kobj);
 			return;
 		}
 	} else {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b717a70219f6..31f3dd6fc946 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -77,6 +77,12 @@ do {							\
  *		return 0 if a binary attribute is not visible. The returned
  *		value will replace static permissions defined in
  *		struct bin_attribute.
+ * @is_group_visible:
+ * 		Optional: Function to return permissions associated with
+ * 		directory created for named groups. When this returns 0
+ * 		all attributes are removed on update, or otherwise the
+ * 		directory is not created in the first instance. When not
+ * 		specified the default permissions of 0755 are applied.
  * @attrs:	Pointer to NULL terminated list of attributes.
  * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
  *		Either attrs or bin_attrs or both must be provided.
@@ -87,10 +93,38 @@ struct attribute_group {
 					      struct attribute *, int);
 	umode_t			(*is_bin_visible)(struct kobject *,
 						  struct bin_attribute *, int);
+#ifdef CONFIG_SYSFS_GROUP_VISIBLE
+	umode_t			(*is_group_visible)(struct kobject *);
+#endif
+
 	struct attribute	**attrs;
 	struct bin_attribute	**bin_attrs;
 };
 
+#ifdef CONFIG_SYSFS_GROUP_VISIBLE
+static inline bool has_group_visible(const struct attribute_group *grp)
+{
+	return grp->is_group_visible != NULL;
+}
+
+static inline umode_t is_group_visible(const struct attribute_group *grp,
+				       struct kobject *kobj)
+{
+	return grp->is_group_visible(kobj);
+}
+#else
+static inline bool has_group_visible(const struct attribute_group *grp)
+{
+	return false;
+}
+
+static inline umode_t is_group_visible(const struct attribute_group *grp,
+				       struct kobject *kobj)
+{
+	return 0755;
+}
+#endif
+
 /*
  * Use these macros to make defining attributes easier.
  * See include/linux/device.h for examples..

Dan Williams wrote:
> Greg KH wrote:
> [..]
> > > 
> > > Hey Greg,
> > > 
> > > I wanted to follow up on this and see if you are able to provide more
> > > details for reproducing or if you are able to look into it?
> > 
> > Last I tried this, it still crashed and would not boot either on my
> > laptop or my workstation.  I don't know how it is working properly for
> > you, what systems have you tried it on?
> > 
> > I'm not going to be able to look at this for many weeks due to
> > conference stuff, so if you want to take the series and test it and
> > hopefully catch my error, that would be great, I'd love to move forward
> > and get this merged someday.
> 
> I mentioned to Lukas that I was working on a "sysfs group visibility"
> patch and he pointed me to this thread. I will note that I tried to make
> the "hide group if all attributes are invisible" approach work, but
> reverted to a "new is_group_visible() callback" approach. I did read
> through the thread and try to improve the argument in the changelog
> accordingly.
> 
> I do admit to liking the cleanliness (not touching 'struct
> attribute_group') of the "hide if no visible attribute" approch, but see
> the criticism of that alternative below, and let me know if it is
> convincing. I tested it locally with the following hack to make the
> group disappear every other sysfs_update_group() event:

Hey Greg,

Ignore this version:

---
From: Dan Williams <dan.j.williams@intel.com>
Date: Tue, 23 Jan 2024 20:20:39 -0800
Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
---

I am going back to your approach without a new callback, and some fixups
to avoid unintended directory removal. I will post that shortly with its
consumer.

On Fri, Jan 26, 2024 at 10:58:07AM -0800, Dan Williams wrote:
> Dan Williams wrote:
> > Greg KH wrote:
> > [..]
> > > > 
> > > > Hey Greg,
> > > > 
> > > > I wanted to follow up on this and see if you are able to provide more
> > > > details for reproducing or if you are able to look into it?
> > > 
> > > Last I tried this, it still crashed and would not boot either on my
> > > laptop or my workstation.  I don't know how it is working properly for
> > > you, what systems have you tried it on?
> > > 
> > > I'm not going to be able to look at this for many weeks due to
> > > conference stuff, so if you want to take the series and test it and
> > > hopefully catch my error, that would be great, I'd love to move forward
> > > and get this merged someday.
> > 
> > I mentioned to Lukas that I was working on a "sysfs group visibility"
> > patch and he pointed me to this thread. I will note that I tried to make
> > the "hide group if all attributes are invisible" approach work, but
> > reverted to a "new is_group_visible() callback" approach. I did read
> > through the thread and try to improve the argument in the changelog
> > accordingly.
> > 
> > I do admit to liking the cleanliness (not touching 'struct
> > attribute_group') of the "hide if no visible attribute" approch, but see
> > the criticism of that alternative below, and let me know if it is
> > convincing. I tested it locally with the following hack to make the
> > group disappear every other sysfs_update_group() event:
> 
> Hey Greg,
> 
> Ignore this version:
> 
> ---
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Tue, 23 Jan 2024 20:20:39 -0800
> Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
> ---
> 
> I am going back to your approach without a new callback, and some fixups
> to avoid unintended directory removal. I will post that shortly with its
> consumer.

Ignore it?  I was just about to write an email that said "maybe this is
the right way forward" :)

What happened to cause it to not be ok?  And if you can find the bug in
the posted patch here, that would be great as well.

thanks,

greg k-h

Greg KH wrote:
> On Fri, Jan 26, 2024 at 10:58:07AM -0800, Dan Williams wrote:
> > Dan Williams wrote:
> > > Greg KH wrote:
> > > [..]
> > > > > 
> > > > > Hey Greg,
> > > > > 
> > > > > I wanted to follow up on this and see if you are able to provide more
> > > > > details for reproducing or if you are able to look into it?
> > > > 
> > > > Last I tried this, it still crashed and would not boot either on my
> > > > laptop or my workstation.  I don't know how it is working properly for
> > > > you, what systems have you tried it on?
> > > > 
> > > > I'm not going to be able to look at this for many weeks due to
> > > > conference stuff, so if you want to take the series and test it and
> > > > hopefully catch my error, that would be great, I'd love to move forward
> > > > and get this merged someday.
> > > 
> > > I mentioned to Lukas that I was working on a "sysfs group visibility"
> > > patch and he pointed me to this thread. I will note that I tried to make
> > > the "hide group if all attributes are invisible" approach work, but
> > > reverted to a "new is_group_visible() callback" approach. I did read
> > > through the thread and try to improve the argument in the changelog
> > > accordingly.
> > > 
> > > I do admit to liking the cleanliness (not touching 'struct
> > > attribute_group') of the "hide if no visible attribute" approch, but see
> > > the criticism of that alternative below, and let me know if it is
> > > convincing. I tested it locally with the following hack to make the
> > > group disappear every other sysfs_update_group() event:
> > 
> > Hey Greg,
> > 
> > Ignore this version:
> > 
> > ---
> > From: Dan Williams <dan.j.williams@intel.com>
> > Date: Tue, 23 Jan 2024 20:20:39 -0800
> > Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
> > ---
> > 
> > I am going back to your approach without a new callback, and some fixups
> > to avoid unintended directory removal. I will post that shortly with its
> > consumer.
> 
> Ignore it?  I was just about to write an email that said "maybe this is
> the right way forward" :)
> 
> What happened to cause it to not be ok? And if you can find the bug in
> the posted patch here, that would be great as well.

It was an aha moment this morning that I could rely on something like:

#define SYSFS_GROUP_INVISIBLE ((umode_t)-1)

...as the return value from either is_visible() or attr_is_visible() and
not need to define a new is_group_visible() callback.

The only downside I can see is that there is no way to know if the
is_visible() handler might return SYSFS_GROUP_INVISIBLE to decide when
to warn about deletion not finding anything to delete, or update not
finding the existing node to update.

I'll type it up and see how it looks, but if you're not worried about
the is_group_visible() addition to 'struct atttribute_group' I think
that way is less hacky than the above.

Dan Williams wrote:
> > > Hey Greg,
> > > 
> > > Ignore this version:
> > > 
> > > ---
> > > From: Dan Williams <dan.j.williams@intel.com>
> > > Date: Tue, 23 Jan 2024 20:20:39 -0800
> > > Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
> > > ---
> > > 
> > > I am going back to your approach without a new callback, and some fixups
> > > to avoid unintended directory removal. I will post that shortly with its
> > > consumer.
> > 
> > Ignore it?  I was just about to write an email that said "maybe this is
> > the right way forward" :)
> > 
> > What happened to cause it to not be ok? And if you can find the bug in
> > the posted patch here, that would be great as well.
> 
> It was an aha moment this morning that I could rely on something like:
> 
> #define SYSFS_GROUP_INVISIBLE ((umode_t)-1)
> 
> ...as the return value from either is_visible() or attr_is_visible() and
> not need to define a new is_group_visible() callback.
> 
> The only downside I can see is that there is no way to know if the
> is_visible() handler might return SYSFS_GROUP_INVISIBLE to decide when
> to warn about deletion not finding anything to delete, or update not
> finding the existing node to update.
> 
> I'll type it up and see how it looks, but if you're not worried about
> the is_group_visible() addition to 'struct atttribute_group' I think
> that way is less hacky than the above.

Ok, here it is (below the scissors line). I ended up including a way to
determine if an attribute_group is opting into this new capability, and
I do think this is more in line with your direction to just reuse
existing is_visible() callbacks. This was tested with the following hack
to a dynamic visibility group in CXL:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3e817a6f94c6..286b91e29c88 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2010,8 +2010,8 @@ static struct attribute *target_attrs[] = {
 	NULL,
 };
 
-static umode_t cxl_region_target_visible(struct kobject *kobj,
-					 struct attribute *a, int n)
+static umode_t cxl_region_target_attr_visible(struct kobject *kobj,
+					      struct attribute *a, int n)
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct cxl_region *cxlr = to_cxl_region(dev);
@@ -2022,9 +2022,22 @@ static umode_t cxl_region_target_visible(struct kobject *kobj,
 	return 0;
 }
 
+static bool cxl_region_target_group_visible(struct kobject *kobj)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_region_params *p = &cxlr->params;
+
+	if (!p->interleave_ways || p->interleave_ways > 2)
+		return false;
+	return true;
+}
+DEFINE_SYSFS_GROUP_VISIBLE(cxl_region_target);
+
 static const struct attribute_group cxl_region_target_group = {
+	.name = "target_group",
 	.attrs = target_attrs,
-	.is_visible = cxl_region_target_visible,
+	.is_visible = SYSFS_GROUP_VISIBLE(cxl_region_target),
 };
 
 static const struct attribute_group *get_cxl_region_target_group(void)

-- >8 --
From 293a9b1d451cba5e7f95897de8c980fddead43ab Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Fri, 26 Jan 2024 18:17:53 -0800
Subject: [PATCH v2] sysfs: Introduce a mechanism to hide static attribute_groups

Add a mechanism for named attribute_groups to hide their directory at
sysfs_update_group() time, or otherwise skip emitting the group
directory when the group is first registered. It piggybacks on
is_visible() in a similar manner as SYSFS_PREALLOC, i.e. special flags
in the upper bits of the returned mode. To use it, specify a symbol
prefix to DEFINE_SYSFS_GROUP_VISIBLE(), and then pass that same prefix
to SYSFS_GROUP_VISIBLE() when assigning the @is_visible() callback:

	DEFINE_SYSFS_GROUP_VISIBLE($prefix)

	struct attribute_group $prefix_group = {
		.name = $name,
		.is_visible = SYSFS_GROUP_VISIBLE($prefix),
	};

SYSFS_GROUP_VISIBLE() expects a definition of $prefix_group_visible()
and $prefix_attr_visible(), where $prefix_group_visible() just returns
true / false and $prefix_attr_visible() behaves as normal.

The motivation for this capability is to centralize PCI device
authentication in the PCI core with a named sysfs group while keeping
that group hidden for devices and platforms that do not meet the
requirements. In a PCI topology, most devices will not support
authentication, a small subset will support just PCI CMA (Component
Measurement and Authentication), a smaller subset will support PCI CMA +
PCIe IDE (Link Integrity and Encryption), and only next generation
server hosts will start to include a platform TSM (TEE Security
Manager).

Without this capability the alternatives are:

* Check if all attributes are invisible and if so, hide the directory.
  Beyond trouble getting this to work [1], this is an ABI change for
  scenarios if userspace happens to depend on group visibility absent any
  attributes. I.e. this new capability avoids regression since it does
  not retroactively apply to existing cases.

* Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI
  devices (i.e. for the case when TSM platform support is present, but
  device support is absent). Unfortunate that this will be a vestigial
  empty directory in the vast majority of cases.

* Reintroduce usage of runtime calls to sysfs_{create,remove}_group()
  in the PCI core. Bjorn has already indicated that he does not want to
  see any growth of pci_sysfs_init() [2].

* Drop the named group and simulate a directory by prefixing all
  TSM-related attributes with "tsm_". Unfortunate to not use the naming
  capability of a sysfs group as intended.

In comparison, there is a small potential for regression if for some
reason an @is_visible() callback had dependencies on how many times it
was called.

Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1]
Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/sysfs/group.c      | 64 ++++++++++++++++++++++++++++++++------
 include/linux/sysfs.h | 71 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 114 insertions(+), 21 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..90dd98c82776 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -31,6 +31,17 @@ static void remove_files(struct kernfs_node *parent,
 			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
+static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
+{
+	if (grp->attrs && grp->is_visible)
+		return grp->is_visible(kobj, grp->attrs[0], 0);
+
+	if (grp->bin_attrs && grp->is_bin_visible)
+		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
+
+	return 0;
+}
+
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 			kuid_t uid, kgid_t gid,
 			const struct attribute_group *grp, int update)
@@ -52,6 +63,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 				kernfs_remove_by_name(parent, (*attr)->name);
 			if (grp->is_visible) {
 				mode = grp->is_visible(kobj, *attr, i);
+				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
 				if (!mode)
 					continue;
 			}
@@ -81,6 +93,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 						(*bin_attr)->attr.name);
 			if (grp->is_bin_visible) {
 				mode = grp->is_bin_visible(kobj, *bin_attr, i);
+				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
 				if (!mode)
 					continue;
 			}
@@ -127,16 +140,35 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	if (grp->name) {
+		umode_t mode = __first_visible(grp, kobj);
+		bool has_group_visible = mode & SYSFS_HAS_GROUP_VISIBLE;
+
+		if (has_group_visible && (mode & SYSFS_GROUP_INVISIBLE))
+			mode = 0;
+		else
+			mode = S_IRWXU | S_IRUGO | S_IXUGO;
+
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				if (!has_group_visible) {
+					pr_warn("Can't update unknown attr grp name: %s/%s\n",
+						kobj->name, grp->name);
+					return -EINVAL;
+				}
+				/* may have been invisible prior to this update */
+				update = 0;
+			} else if (!mode) {
+				sysfs_remove_group(kobj, grp);
+				kernfs_put(kn);
+				return 0;
 			}
-		} else {
-			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
-						  S_IRWXU | S_IRUGO | S_IXUGO,
+		}
+
+		if (!update) {
+			if (!mode)
+				return 0;
+			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
 						  uid, gid, kobj, NULL);
 			if (IS_ERR(kn)) {
 				if (PTR_ERR(kn) == -EEXIST)
@@ -262,6 +294,22 @@ int sysfs_update_group(struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(sysfs_update_group);
 
+static void warn_group_not_found(const struct attribute_group *grp,
+				 struct kobject *kobj)
+{
+	umode_t mode = __first_visible(grp, kobj);
+
+	if (mode & SYSFS_HAS_GROUP_VISIBLE) {
+		/* may have never been created */
+		pr_debug("sysfs group '%s' not found for kobject '%s'\n",
+			 grp->name, kobject_name(kobj));
+		return;
+	}
+
+	WARN(1, KERN_WARNING "sysfs group '%s' not found for kobject '%s'\n",
+	     grp->name, kobject_name(kobj));
+}
+
 /**
  * sysfs_remove_group: remove a group from a kobject
  * @kobj:	kobject to remove the group from
@@ -279,9 +327,7 @@ void sysfs_remove_group(struct kobject *kobj,
 	if (grp->name) {
 		kn = kernfs_find_and_get(parent, grp->name);
 		if (!kn) {
-			WARN(!kn, KERN_WARNING
-			     "sysfs group '%s' not found for kobject '%s'\n",
-			     grp->name, kobject_name(kobj));
+			warn_group_not_found(grp, kobj);
 			return;
 		}
 	} else {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b717a70219f6..4fb4f4da003a 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -62,21 +62,32 @@ do {							\
  * struct attribute_group - data structure used to declare an attribute group.
  * @name:	Optional: Attribute group name
  *		If specified, the attribute group will be created in
- *		a new subdirectory with this name.
+ *		a new subdirectory with this name. Additionally when a
+ *		group is named, @is_visible and @is_bin_visible may
+ *		return SYSFS_HAS_GROUP_VISIBLE | SYSFS_GROUP_INVISIBLE
+ *		to control visibility of the directory itself. If
+ *		SYSFS_GROUP_INVISIBLE is ever to be returned,
+ *		SYSFS_HAS_GROUP_VISIBLE must always be included in the
+ *		return value from @is_visible and @is_bin_visible. See
+ *		the DEFINE_SYSFS_GROUP_VISIBLE() helper.
  * @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
- *		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.
+ *		attribute of the group. Will be called repeatedly for
+ *		each non-binary attribute in the group. Only read/write
+ *		permissions as well as SYSFS_PREALLOC (and the
+ *		visibility flags for named groups) are accepted. Must
+ *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if an
+ *		attribute is not visible. The returned value will
+ *		replace static permissions defined in struct attribute.
  * @is_bin_visible:
  *		Optional: Function to return permissions associated with a
  *		binary attribute of the group. Will be called repeatedly
  *		for each binary attribute in the group. Only read/write
- *		permissions as well as SYSFS_PREALLOC are accepted. Must
- *		return 0 if a binary attribute is not visible. The returned
- *		value will replace static permissions defined in
- *		struct bin_attribute.
+ *		permissions as well as SYSFS_PREALLOC (and the
+ *		visibility flags for named groups) are accepted. Must
+ *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if a binary
+ *		attribute is not visible. The returned value will
+ *		replace static permissions defined in struct
+ *		bin_attribute.
  * @attrs:	Pointer to NULL terminated list of attributes.
  * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
  *		Either attrs or bin_attrs or both must be provided.
@@ -91,13 +102,49 @@ struct attribute_group {
 	struct bin_attribute	**bin_attrs;
 };
 
+#define SYSFS_PREALLOC		010000
+#define SYSFS_HAS_GROUP_VISIBLE 020000
+#define SYSFS_GROUP_INVISIBLE	040000
+#define SYSFS_GROUP_VISIBLE_MASK (SYSFS_HAS_GROUP_VISIBLE|SYSFS_GROUP_INVISIBLE)
+
+static inline umode_t sysfs_group_visible(umode_t mode)
+{
+	return mode | SYSFS_HAS_GROUP_VISIBLE;
+}
+
+/*
+ * The first call to is_visible() in the create / update path may
+ * indicate visibility for the entire group
+ */
+#define DEFINE_SYSFS_GROUP_VISIBLE(name)                                \
+static inline umode_t sysfs_group_visible_##name(                       \
+	struct kobject *kobj, struct attribute *attr, int n)            \
+{                                                                       \
+	if (n == 0 && !name##_group_visible(kobj))			\
+		return sysfs_group_visible(SYSFS_GROUP_INVISIBLE);      \
+	return sysfs_group_visible(name##_attr_visible(kobj, attr, n)); \
+}
+
+/*
+ * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary
+ * attributes
+ */
+#define DEFINE_SYSFS_BIN_GROUP_VISIBLE(name)                            \
+static inline umode_t sysfs_group_visible_##name(                       \
+	struct kobject *kobj, struct bin_attribute *attr, int n)        \
+{                                                                       \
+	if (n == 0 && !name##_group_visible(kobj))		        \
+		return sysfs_group_visible(SYSFS_GROUP_INVISIBLE);      \
+	return sysfs_group_visible(name##_attr_visible(kobj, attr, n)); \
+}
+
+#define SYSFS_GROUP_VISIBLE(fn) sysfs_group_visible_##fn
+
 /*
  * Use these macros to make defining attributes easier.
  * See include/linux/device.h for examples..
  */
 
-#define SYSFS_PREALLOC 010000
-
 #define __ATTR(_name, _mode, _show, _store) {				\
 	.attr = {.name = __stringify(_name),				\
 		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\

On Fri, Jan 26, 2024 at 07:00:30PM -0800, Dan Williams wrote:
> > I'll type it up and see how it looks, but if you're not worried about
> > the is_group_visible() addition to 'struct atttribute_group' I think
> > that way is less hacky than the above.
> 
> Ok, here it is (below the scissors line). I ended up including a way to
> determine if an attribute_group is opting into this new capability, and
> I do think this is more in line with your direction to just reuse
> existing is_visible() callbacks. This was tested with the following hack
> to a dynamic visibility group in CXL:

At first glance, I like this, but your example here:

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3e817a6f94c6..286b91e29c88 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2010,8 +2010,8 @@ static struct attribute *target_attrs[] = {
>  	NULL,
>  };
>  
> -static umode_t cxl_region_target_visible(struct kobject *kobj,
> -					 struct attribute *a, int n)
> +static umode_t cxl_region_target_attr_visible(struct kobject *kobj,
> +					      struct attribute *a, int n)
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct cxl_region *cxlr = to_cxl_region(dev);
> @@ -2022,9 +2022,22 @@ static umode_t cxl_region_target_visible(struct kobject *kobj,
>  	return 0;
>  }
>  
> +static bool cxl_region_target_group_visible(struct kobject *kobj)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	struct cxl_region_params *p = &cxlr->params;
> +
> +	if (!p->interleave_ways || p->interleave_ways > 2)
> +		return false;
> +	return true;
> +}
> +DEFINE_SYSFS_GROUP_VISIBLE(cxl_region_target);
> +
>  static const struct attribute_group cxl_region_target_group = {
> +	.name = "target_group",
>  	.attrs = target_attrs,
> -	.is_visible = cxl_region_target_visible,
> +	.is_visible = SYSFS_GROUP_VISIBLE(cxl_region_target),
>  };
>  
>  static const struct attribute_group *get_cxl_region_target_group(void)
> 

Seems to not match the documentation in the patch itself, but maybe I'm
reading it wrong.

If all that is needed is the above type of change, I'm all for this,
comments on the implementation below:

> -- >8 --
> >From 293a9b1d451cba5e7f95897de8c980fddead43ab Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Fri, 26 Jan 2024 18:17:53 -0800
> Subject: [PATCH v2] sysfs: Introduce a mechanism to hide static attribute_groups
> 
> Add a mechanism for named attribute_groups to hide their directory at
> sysfs_update_group() time, or otherwise skip emitting the group
> directory when the group is first registered. It piggybacks on
> is_visible() in a similar manner as SYSFS_PREALLOC, i.e. special flags
> in the upper bits of the returned mode. To use it, specify a symbol
> prefix to DEFINE_SYSFS_GROUP_VISIBLE(), and then pass that same prefix
> to SYSFS_GROUP_VISIBLE() when assigning the @is_visible() callback:
> 
> 	DEFINE_SYSFS_GROUP_VISIBLE($prefix)
> 
> 	struct attribute_group $prefix_group = {
> 		.name = $name,
> 		.is_visible = SYSFS_GROUP_VISIBLE($prefix),
> 	};
> 
> SYSFS_GROUP_VISIBLE() expects a definition of $prefix_group_visible()
> and $prefix_attr_visible(), where $prefix_group_visible() just returns
> true / false and $prefix_attr_visible() behaves as normal.
> 
> The motivation for this capability is to centralize PCI device
> authentication in the PCI core with a named sysfs group while keeping
> that group hidden for devices and platforms that do not meet the
> requirements. In a PCI topology, most devices will not support
> authentication, a small subset will support just PCI CMA (Component
> Measurement and Authentication), a smaller subset will support PCI CMA +
> PCIe IDE (Link Integrity and Encryption), and only next generation
> server hosts will start to include a platform TSM (TEE Security
> Manager).
> 
> Without this capability the alternatives are:
> 
> * Check if all attributes are invisible and if so, hide the directory.
>   Beyond trouble getting this to work [1], this is an ABI change for
>   scenarios if userspace happens to depend on group visibility absent any
>   attributes. I.e. this new capability avoids regression since it does
>   not retroactively apply to existing cases.
> 
> * Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI
>   devices (i.e. for the case when TSM platform support is present, but
>   device support is absent). Unfortunate that this will be a vestigial
>   empty directory in the vast majority of cases.
> 
> * Reintroduce usage of runtime calls to sysfs_{create,remove}_group()
>   in the PCI core. Bjorn has already indicated that he does not want to
>   see any growth of pci_sysfs_init() [2].
> 
> * Drop the named group and simulate a directory by prefixing all
>   TSM-related attributes with "tsm_". Unfortunate to not use the naming
>   capability of a sysfs group as intended.
> 
> In comparison, there is a small potential for regression if for some
> reason an @is_visible() callback had dependencies on how many times it
> was called.

Great changelog, thanks for all of that.

> 
> Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1]
> Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  fs/sysfs/group.c      | 64 ++++++++++++++++++++++++++++++++------
>  include/linux/sysfs.h | 71 +++++++++++++++++++++++++++++++++++--------
>  2 files changed, 114 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 138676463336..90dd98c82776 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -31,6 +31,17 @@ static void remove_files(struct kernfs_node *parent,
>  			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
>  }
>  
> +static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> +{
> +	if (grp->attrs && grp->is_visible)
> +		return grp->is_visible(kobj, grp->attrs[0], 0);
> +
> +	if (grp->bin_attrs && grp->is_bin_visible)
> +		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);

This kind of makes sense, but why the first attribute only?  I guess we
have to pick one?

> +
> +	return 0;
> +}
> +
>  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  			kuid_t uid, kgid_t gid,
>  			const struct attribute_group *grp, int update)
> @@ -52,6 +63,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  				kernfs_remove_by_name(parent, (*attr)->name);
>  			if (grp->is_visible) {
>  				mode = grp->is_visible(kobj, *attr, i);
> +				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
>  				if (!mode)
>  					continue;
>  			}
> @@ -81,6 +93,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  						(*bin_attr)->attr.name);
>  			if (grp->is_bin_visible) {
>  				mode = grp->is_bin_visible(kobj, *bin_attr, i);
> +				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
>  				if (!mode)
>  					continue;
>  			}
> @@ -127,16 +140,35 @@ static int internal_create_group(struct kobject *kobj, int update,
>  
>  	kobject_get_ownership(kobj, &uid, &gid);
>  	if (grp->name) {
> +		umode_t mode = __first_visible(grp, kobj);
> +		bool has_group_visible = mode & SYSFS_HAS_GROUP_VISIBLE;
> +
> +		if (has_group_visible && (mode & SYSFS_GROUP_INVISIBLE))

These new SYSFS_*_GROUP values are confusing me, more below:

> +			mode = 0;
> +		else
> +			mode = S_IRWXU | S_IRUGO | S_IXUGO;
> +
>  		if (update) {
>  			kn = kernfs_find_and_get(kobj->sd, grp->name);
>  			if (!kn) {
> -				pr_warn("Can't update unknown attr grp name: %s/%s\n",
> -					kobj->name, grp->name);
> -				return -EINVAL;
> +				if (!has_group_visible) {
> +					pr_warn("Can't update unknown attr grp name: %s/%s\n",
> +						kobj->name, grp->name);
> +					return -EINVAL;
> +				}
> +				/* may have been invisible prior to this update */
> +				update = 0;
> +			} else if (!mode) {
> +				sysfs_remove_group(kobj, grp);
> +				kernfs_put(kn);
> +				return 0;
>  			}
> -		} else {
> -			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> -						  S_IRWXU | S_IRUGO | S_IXUGO,
> +		}
> +
> +		if (!update) {
> +			if (!mode)
> +				return 0;
> +			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
>  						  uid, gid, kobj, NULL);
>  			if (IS_ERR(kn)) {
>  				if (PTR_ERR(kn) == -EEXIST)
> @@ -262,6 +294,22 @@ int sysfs_update_group(struct kobject *kobj,
>  }
>  EXPORT_SYMBOL_GPL(sysfs_update_group);
>  
> +static void warn_group_not_found(const struct attribute_group *grp,
> +				 struct kobject *kobj)
> +{
> +	umode_t mode = __first_visible(grp, kobj);
> +
> +	if (mode & SYSFS_HAS_GROUP_VISIBLE) {
> +		/* may have never been created */
> +		pr_debug("sysfs group '%s' not found for kobject '%s'\n",
> +			 grp->name, kobject_name(kobj));
> +		return;

So if the group is visible somehow, but not found, then it's ok?  But:


> +	}
> +
> +	WARN(1, KERN_WARNING "sysfs group '%s' not found for kobject '%s'\n",
> +	     grp->name, kobject_name(kobj));

We crash the box if it was not visible and not found?


> +}
> +
>  /**
>   * sysfs_remove_group: remove a group from a kobject
>   * @kobj:	kobject to remove the group from
> @@ -279,9 +327,7 @@ void sysfs_remove_group(struct kobject *kobj,
>  	if (grp->name) {
>  		kn = kernfs_find_and_get(parent, grp->name);
>  		if (!kn) {
> -			WARN(!kn, KERN_WARNING
> -			     "sysfs group '%s' not found for kobject '%s'\n",
> -			     grp->name, kobject_name(kobj));
> +			warn_group_not_found(grp, kobj);

We really should not WARN(), but I guess we don't ever get reports of
this so it's ok.

>  			return;
>  		}
>  	} else {
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index b717a70219f6..4fb4f4da003a 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -62,21 +62,32 @@ do {							\
>   * struct attribute_group - data structure used to declare an attribute group.
>   * @name:	Optional: Attribute group name
>   *		If specified, the attribute group will be created in
> - *		a new subdirectory with this name.
> + *		a new subdirectory with this name. Additionally when a
> + *		group is named, @is_visible and @is_bin_visible may
> + *		return SYSFS_HAS_GROUP_VISIBLE | SYSFS_GROUP_INVISIBLE
> + *		to control visibility of the directory itself. If
> + *		SYSFS_GROUP_INVISIBLE is ever to be returned,
> + *		SYSFS_HAS_GROUP_VISIBLE must always be included in the
> + *		return value from @is_visible and @is_bin_visible. See
> + *		the DEFINE_SYSFS_GROUP_VISIBLE() helper.

Here it gets confusing for me.  Why are you saying things like
SYSFS_HAS_GROUP_VISIBLE and SYSFS_GROUP_INVISIBLE, when your example
shows none of that at all?  Shouldn't this all be internal-to-sysfs
stuff?  Why list it here?


>   * @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
> - *		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.
> + *		attribute of the group. Will be called repeatedly for
> + *		each non-binary attribute in the group. Only read/write
> + *		permissions as well as SYSFS_PREALLOC (and the
> + *		visibility flags for named groups) are accepted. Must
> + *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if an
> + *		attribute is not visible. The returned value will
> + *		replace static permissions defined in struct attribute.
>   * @is_bin_visible:
>   *		Optional: Function to return permissions associated with a
>   *		binary attribute of the group. Will be called repeatedly
>   *		for each binary attribute in the group. Only read/write
> - *		permissions as well as SYSFS_PREALLOC are accepted. Must
> - *		return 0 if a binary attribute is not visible. The returned
> - *		value will replace static permissions defined in
> - *		struct bin_attribute.
> + *		permissions as well as SYSFS_PREALLOC (and the
> + *		visibility flags for named groups) are accepted. Must
> + *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if a binary
> + *		attribute is not visible. The returned value will
> + *		replace static permissions defined in struct
> + *		bin_attribute.
>   * @attrs:	Pointer to NULL terminated list of attributes.
>   * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
>   *		Either attrs or bin_attrs or both must be provided.
> @@ -91,13 +102,49 @@ struct attribute_group {
>  	struct bin_attribute	**bin_attrs;
>  };
>  
> +#define SYSFS_PREALLOC		010000
> +#define SYSFS_HAS_GROUP_VISIBLE 020000

Nit, forgot a tab :(

> +#define SYSFS_GROUP_INVISIBLE	040000
> +#define SYSFS_GROUP_VISIBLE_MASK (SYSFS_HAS_GROUP_VISIBLE|SYSFS_GROUP_INVISIBLE)

Ackward defines, "HAS_GROUP_VISABLE" vs. "GROUP_INVISIBLE"?  Why not
just "GROUP_VISIBLE" and "GROUP_INVISIBLE"?


> +
> +static inline umode_t sysfs_group_visible(umode_t mode)
> +{
> +	return mode | SYSFS_HAS_GROUP_VISIBLE;

So if mode is 0, then we return visible?  is that really correct?

> +}
> +
> +/*
> + * The first call to is_visible() in the create / update path may
> + * indicate visibility for the entire group
> + */
> +#define DEFINE_SYSFS_GROUP_VISIBLE(name)                                \
> +static inline umode_t sysfs_group_visible_##name(                       \
> +	struct kobject *kobj, struct attribute *attr, int n)            \
> +{                                                                       \
> +	if (n == 0 && !name##_group_visible(kobj))			\
> +		return sysfs_group_visible(SYSFS_GROUP_INVISIBLE);      \

This reads really funny, we are passing in "invisible" to a "visible"
function call :)

Why pass anything in here?  I really have a hard time parsing this,
maybe because of the negative of the "*_group_visible()" call?


> +	return sysfs_group_visible(name##_attr_visible(kobj, attr, n)); \

But you only call this on the first attribute, right?  I kind of
understand that, but documenting it a bit better here might help?

Anyway, basic structure I like, changes for a driver to use this I love,
but implementation confuses me, and if I have to maintain it for the
next 20+ years, and can't understand it on day 1, I'm going to be in
trouble soon, which makes me wary to take this as-is.

thanks,

greg k-h

Greg KH wrote:
[..]
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index 138676463336..90dd98c82776 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -31,6 +31,17 @@ static void remove_files(struct kernfs_node *parent,
> >  			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
> >  }
> >  
> > +static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
> > +{
> > +	if (grp->attrs && grp->is_visible)
> > +		return grp->is_visible(kobj, grp->attrs[0], 0);
> > +
> > +	if (grp->bin_attrs && grp->is_bin_visible)
> > +		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
> 
> This kind of makes sense, but why the first attribute only?  I guess we
> have to pick one?

Yeah, to not confuse existing implementations is_visible() must always be
passed a valid @attr argument, so might as well pick first available.

> > +
> > +	return 0;
> > +}
> > +
> >  static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  			kuid_t uid, kgid_t gid,
> >  			const struct attribute_group *grp, int update)
> > @@ -52,6 +63,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  				kernfs_remove_by_name(parent, (*attr)->name);
> >  			if (grp->is_visible) {
> >  				mode = grp->is_visible(kobj, *attr, i);
> > +				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
> >  				if (!mode)
> >  					continue;
> >  			}
> > @@ -81,6 +93,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  						(*bin_attr)->attr.name);
> >  			if (grp->is_bin_visible) {
> >  				mode = grp->is_bin_visible(kobj, *bin_attr, i);
> > +				mode &= ~SYSFS_GROUP_VISIBLE_MASK;
> >  				if (!mode)
> >  					continue;
> >  			}
> > @@ -127,16 +140,35 @@ static int internal_create_group(struct kobject *kobj, int update,
> >  
> >  	kobject_get_ownership(kobj, &uid, &gid);
> >  	if (grp->name) {
> > +		umode_t mode = __first_visible(grp, kobj);
> > +		bool has_group_visible = mode & SYSFS_HAS_GROUP_VISIBLE;
> > +
> > +		if (has_group_visible && (mode & SYSFS_GROUP_INVISIBLE))
> 
> These new SYSFS_*_GROUP values are confusing me, more below:

Ok, SYSFS_HAS_GROUP_VISIBLE is mainly about preserving the long standing
WARN() behavior that flags broken usage of sysfs. If you are open to
dropping strictness I think this can be simplified.

> 
> > +			mode = 0;
> > +		else
> > +			mode = S_IRWXU | S_IRUGO | S_IXUGO;
> > +
> >  		if (update) {
> >  			kn = kernfs_find_and_get(kobj->sd, grp->name);
> >  			if (!kn) {
> > -				pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > -					kobj->name, grp->name);
> > -				return -EINVAL;
> > +				if (!has_group_visible) {
> > +					pr_warn("Can't update unknown attr grp name: %s/%s\n",
> > +						kobj->name, grp->name);
> > +					return -EINVAL;
> > +				}
> > +				/* may have been invisible prior to this update */
> > +				update = 0;
> > +			} else if (!mode) {
> > +				sysfs_remove_group(kobj, grp);
> > +				kernfs_put(kn);
> > +				return 0;
> >  			}
> > -		} else {
> > -			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
> > -						  S_IRWXU | S_IRUGO | S_IXUGO,
> > +		}
> > +
> > +		if (!update) {
> > +			if (!mode)
> > +				return 0;
> > +			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
> >  						  uid, gid, kobj, NULL);
> >  			if (IS_ERR(kn)) {
> >  				if (PTR_ERR(kn) == -EEXIST)
> > @@ -262,6 +294,22 @@ int sysfs_update_group(struct kobject *kobj,
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_update_group);
> >  
> > +static void warn_group_not_found(const struct attribute_group *grp,
> > +				 struct kobject *kobj)
> > +{
> > +	umode_t mode = __first_visible(grp, kobj);
> > +
> > +	if (mode & SYSFS_HAS_GROUP_VISIBLE) {
> > +		/* may have never been created */
> > +		pr_debug("sysfs group '%s' not found for kobject '%s'\n",
> > +			 grp->name, kobject_name(kobj));
> > +		return;
> 
> So if the group is visible somehow, but not found, then it's ok?  But:
> 
> 
> > +	}
> > +
> > +	WARN(1, KERN_WARNING "sysfs group '%s' not found for kobject '%s'\n",
> > +	     grp->name, kobject_name(kobj));
> 
> We crash the box if it was not visible and not found?

Yes, but only for preservation of legacy strictness. In other words,
without the new "has" flag this is as an unambiguous misuse of sysfs
APIs. With the "has" flag it is no long definitive that someone messed
up.

My preference is delete the WARN() and the "has" flag if you're open to
that.

> > +}
> > +
> >  /**
> >   * sysfs_remove_group: remove a group from a kobject
> >   * @kobj:	kobject to remove the group from
> > @@ -279,9 +327,7 @@ void sysfs_remove_group(struct kobject *kobj,
> >  	if (grp->name) {
> >  		kn = kernfs_find_and_get(parent, grp->name);
> >  		if (!kn) {
> > -			WARN(!kn, KERN_WARNING
> > -			     "sysfs group '%s' not found for kobject '%s'\n",
> > -			     grp->name, kobject_name(kobj));
> > +			warn_group_not_found(grp, kobj);
> 
> We really should not WARN(), but I guess we don't ever get reports of
> this so it's ok.

It's one of those "should only fire on the kernel developer's system
during development" WARN()s, and per above if it's not adding value, it
is now adding complexity for this group visibility feature.

> 
> >  			return;
> >  		}
> >  	} else {
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index b717a70219f6..4fb4f4da003a 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -62,21 +62,32 @@ do {							\
> >   * struct attribute_group - data structure used to declare an attribute group.
> >   * @name:	Optional: Attribute group name
> >   *		If specified, the attribute group will be created in
> > - *		a new subdirectory with this name.
> > + *		a new subdirectory with this name. Additionally when a
> > + *		group is named, @is_visible and @is_bin_visible may
> > + *		return SYSFS_HAS_GROUP_VISIBLE | SYSFS_GROUP_INVISIBLE
> > + *		to control visibility of the directory itself. If
> > + *		SYSFS_GROUP_INVISIBLE is ever to be returned,
> > + *		SYSFS_HAS_GROUP_VISIBLE must always be included in the
> > + *		return value from @is_visible and @is_bin_visible. See
> > + *		the DEFINE_SYSFS_GROUP_VISIBLE() helper.
> 
> Here it gets confusing for me.  Why are you saying things like
> SYSFS_HAS_GROUP_VISIBLE and SYSFS_GROUP_INVISIBLE, when your example
> shows none of that at all?  Shouldn't this all be internal-to-sysfs
> stuff?  Why list it here?

True. I took my cue from the fact that SYSFS_PREALLOC is mentioned in
the documentation even though it is only ever used internally by the
attribute definition macros. I can trim to only the user documentation.

> >   * @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
> > - *		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.
> > + *		attribute of the group. Will be called repeatedly for
> > + *		each non-binary attribute in the group. Only read/write
> > + *		permissions as well as SYSFS_PREALLOC (and the
> > + *		visibility flags for named groups) are accepted. Must
> > + *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if an
> > + *		attribute is not visible. The returned value will
> > + *		replace static permissions defined in struct attribute.
> >   * @is_bin_visible:
> >   *		Optional: Function to return permissions associated with a
> >   *		binary attribute of the group. Will be called repeatedly
> >   *		for each binary attribute in the group. Only read/write
> > - *		permissions as well as SYSFS_PREALLOC are accepted. Must
> > - *		return 0 if a binary attribute is not visible. The returned
> > - *		value will replace static permissions defined in
> > - *		struct bin_attribute.
> > + *		permissions as well as SYSFS_PREALLOC (and the
> > + *		visibility flags for named groups) are accepted. Must
> > + *		return 0 (or just SYSFS_HAS_GROUP_VISIBLE) if a binary
> > + *		attribute is not visible. The returned value will
> > + *		replace static permissions defined in struct
> > + *		bin_attribute.
> >   * @attrs:	Pointer to NULL terminated list of attributes.
> >   * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
> >   *		Either attrs or bin_attrs or both must be provided.
> > @@ -91,13 +102,49 @@ struct attribute_group {
> >  	struct bin_attribute	**bin_attrs;
> >  };
> >  
> > +#define SYSFS_PREALLOC		010000
> > +#define SYSFS_HAS_GROUP_VISIBLE 020000
> 
> Nit, forgot a tab :(

Fixed.

> > +#define SYSFS_GROUP_INVISIBLE	040000
> > +#define SYSFS_GROUP_VISIBLE_MASK (SYSFS_HAS_GROUP_VISIBLE|SYSFS_GROUP_INVISIBLE)
> 
> Ackward defines, "HAS_GROUP_VISABLE" vs. "GROUP_INVISIBLE"?  Why not
> just "GROUP_VISIBLE" and "GROUP_INVISIBLE"?

Yeah, this "has" thing is only about preserving the WARN regime, the
other is reflecting the state.

> > +
> > +static inline umode_t sysfs_group_visible(umode_t mode)
> > +{
> > +	return mode | SYSFS_HAS_GROUP_VISIBLE;
> 
> So if mode is 0, then we return visible?  is that really correct?

If the mode is zero we return, "oh by the way, this attribute group
potentially has variable visibility, so don't assume that failures to
find the directory already created at sysfs_update_group() time are
fatal"

Just explaining it that way convinces me it is time for those failure
cases to go.

> > +/*
> > + * The first call to is_visible() in the create / update path may
> > + * indicate visibility for the entire group
> > + */
> > +#define DEFINE_SYSFS_GROUP_VISIBLE(name)                                \
> > +static inline umode_t sysfs_group_visible_##name(                       \
> > +	struct kobject *kobj, struct attribute *attr, int n)            \
> > +{                                                                       \
> > +	if (n == 0 && !name##_group_visible(kobj))			\
> > +		return sysfs_group_visible(SYSFS_GROUP_INVISIBLE);      \
> 
> This reads really funny, we are passing in "invisible" to a "visible"
> function call :)
> 
> Why pass anything in here?  I really have a hard time parsing this,
> maybe because of the negative of the "*_group_visible()" call?
> 
> 
> > +	return sysfs_group_visible(name##_attr_visible(kobj, attr, n)); \
> 
> But you only call this on the first attribute, right?  I kind of
> understand that, but documenting it a bit better here might help?
> 
> Anyway, basic structure I like, changes for a driver to use this I love,
> but implementation confuses me, and if I have to maintain it for the
> next 20+ years, and can't understand it on day 1, I'm going to be in
> trouble soon, which makes me wary to take this as-is.

Here it is again without being beholden to keeping the old
WARN()/pr_warn() regime when the group directory is not found.

-- >8 --
From 8fe87d28bb9517e4010d567ec7025acf2b8b2440 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
Date: Sat, 27 Jan 2024 17:37:44 -0800
Subject: [PATCH v3] sysfs: Introduce a mechanism to hide static attribute_groups

Add a mechanism for named attribute_groups to hide their directory at
sysfs_update_group() time, or otherwise skip emitting the group
directory when the group is first registered. It piggybacks on
is_visible() in a similar manner as SYSFS_PREALLOC, i.e. special flags
in the upper bits of the returned mode. To use it, specify a symbol
prefix to DEFINE_SYSFS_GROUP_VISIBLE(), and then pass that same prefix
to SYSFS_GROUP_VISIBLE() when assigning the @is_visible() callback:

	DEFINE_SYSFS_GROUP_VISIBLE($prefix)

	struct attribute_group $prefix_group = {
		.name = $name,
		.is_visible = SYSFS_GROUP_VISIBLE($prefix),
	};

SYSFS_GROUP_VISIBLE() expects a definition of $prefix_group_visible()
and $prefix_attr_visible(), where $prefix_group_visible() just returns
true / false and $prefix_attr_visible() behaves as normal.

The motivation for this capability is to centralize PCI device
authentication in the PCI core with a named sysfs group while keeping
that group hidden for devices and platforms that do not meet the
requirements. In a PCI topology, most devices will not support
authentication, a small subset will support just PCI CMA (Component
Measurement and Authentication), a smaller subset will support PCI CMA +
PCIe IDE (Link Integrity and Encryption), and only next generation
server hosts will start to include a platform TSM (TEE Security
Manager).

Without this capability the alternatives are:

* Check if all attributes are invisible and if so, hide the directory.
  Beyond trouble getting this to work [1], this is an ABI change for
  scenarios if userspace happens to depend on group visibility absent any
  attributes. I.e. this new capability avoids regression since it does
  not retroactively apply to existing cases.

* Publish an empty /sys/bus/pci/devices/$pdev/tsm/ directory for all PCI
  devices (i.e. for the case when TSM platform support is present, but
  device support is absent). Unfortunate that this will be a vestigial
  empty directory in the vast majority of cases.

* Reintroduce usage of runtime calls to sysfs_{create,remove}_group()
  in the PCI core. Bjorn has already indicated that he does not want to
  see any growth of pci_sysfs_init() [2].

* Drop the named group and simulate a directory by prefixing all
  TSM-related attributes with "tsm_". Unfortunate to not use the naming
  capability of a sysfs group as intended.

In comparison, there is a small potential for regression if for some
reason an @is_visible() callback had dependencies on how many times it
was called. Additionally, it is no longer an error to update a group
that does not have its directory already present, and it is no longer a
WARN() to remove a group that was never visible.

Link: https://lore.kernel.org/all/2024012321-envious-procedure-4a58@gregkh/ [1]
Link: https://lore.kernel.org/linux-pci/20231019200110.GA1410324@bhelgaas/ [2]
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/sysfs/group.c      | 45 ++++++++++++++++++++++++-------
 include/linux/sysfs.h | 63 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 87 insertions(+), 21 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 138676463336..ccb275cdabcb 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -31,6 +31,17 @@ static void remove_files(struct kernfs_node *parent,
 			kernfs_remove_by_name(parent, (*bin_attr)->attr.name);
 }
 
+static umode_t __first_visible(const struct attribute_group *grp, struct kobject *kobj)
+{
+	if (grp->attrs && grp->is_visible)
+		return grp->is_visible(kobj, grp->attrs[0], 0);
+
+	if (grp->bin_attrs && grp->is_bin_visible)
+		return grp->is_bin_visible(kobj, grp->bin_attrs[0], 0);
+
+	return 0;
+}
+
 static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 			kuid_t uid, kgid_t gid,
 			const struct attribute_group *grp, int update)
@@ -52,6 +63,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 				kernfs_remove_by_name(parent, (*attr)->name);
 			if (grp->is_visible) {
 				mode = grp->is_visible(kobj, *attr, i);
+				mode &= ~SYSFS_GROUP_INVISIBLE;
 				if (!mode)
 					continue;
 			}
@@ -81,6 +93,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 						(*bin_attr)->attr.name);
 			if (grp->is_bin_visible) {
 				mode = grp->is_bin_visible(kobj, *bin_attr, i);
+				mode &= ~SYSFS_GROUP_INVISIBLE;
 				if (!mode)
 					continue;
 			}
@@ -127,16 +140,31 @@ static int internal_create_group(struct kobject *kobj, int update,
 
 	kobject_get_ownership(kobj, &uid, &gid);
 	if (grp->name) {
+		umode_t mode = __first_visible(grp, kobj);
+
+		if (mode & SYSFS_GROUP_INVISIBLE)
+			mode = 0;
+		else
+			mode = S_IRWXU | S_IRUGO | S_IXUGO;
+
 		if (update) {
 			kn = kernfs_find_and_get(kobj->sd, grp->name);
 			if (!kn) {
-				pr_warn("Can't update unknown attr grp name: %s/%s\n",
-					kobj->name, grp->name);
-				return -EINVAL;
+				pr_debug("attr grp %s/%s not created yet\n",
+					 kobj->name, grp->name);
+				/* may have been invisible prior to this update */
+				update = 0;
+			} else if (!mode) {
+				sysfs_remove_group(kobj, grp);
+				kernfs_put(kn);
+				return 0;
 			}
-		} else {
-			kn = kernfs_create_dir_ns(kobj->sd, grp->name,
-						  S_IRWXU | S_IRUGO | S_IXUGO,
+		}
+
+		if (!update) {
+			if (!mode)
+				return 0;
+			kn = kernfs_create_dir_ns(kobj->sd, grp->name, mode,
 						  uid, gid, kobj, NULL);
 			if (IS_ERR(kn)) {
 				if (PTR_ERR(kn) == -EEXIST)
@@ -279,9 +307,8 @@ void sysfs_remove_group(struct kobject *kobj,
 	if (grp->name) {
 		kn = kernfs_find_and_get(parent, grp->name);
 		if (!kn) {
-			WARN(!kn, KERN_WARNING
-			     "sysfs group '%s' not found for kobject '%s'\n",
-			     grp->name, kobject_name(kobj));
+			pr_debug("sysfs group '%s' not found for kobject '%s'\n",
+				 grp->name, kobject_name(kobj));
 			return;
 		}
 	} else {
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index b717a70219f6..a42642b277dd 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -61,22 +61,32 @@ do {							\
 /**
  * struct attribute_group - data structure used to declare an attribute group.
  * @name:	Optional: Attribute group name
- *		If specified, the attribute group will be created in
- *		a new subdirectory with this name.
+ *		If specified, the attribute group will be created in a
+ *		new subdirectory with this name. Additionally when a
+ *		group is named, @is_visible and @is_bin_visible may
+ *		return SYSFS_GROUP_INVISIBLE to control visibility of
+ *		the directory itself.
  * @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
+ *		attribute of the group. Will be called repeatedly for
+ *		each non-binary attribute in the 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.
+ *		return 0 if an attribute is not visible. The returned
+ *		value will replace static permissions defined in struct
+ *		attribute. Use SYSFS_GROUP_VISIBLE() when assigning this
+ *		callback to specify separate _group_visible() and
+ *		_attr_visible() handlers.
  * @is_bin_visible:
  *		Optional: Function to return permissions associated with a
  *		binary attribute of the group. Will be called repeatedly
  *		for each binary attribute in the group. Only read/write
- *		permissions as well as SYSFS_PREALLOC are accepted. Must
- *		return 0 if a binary attribute is not visible. The returned
- *		value will replace static permissions defined in
- *		struct bin_attribute.
+ *		permissions as well as SYSFS_PREALLOC (and the
+ *		visibility flags for named groups) are accepted. Must
+ *		return 0 if a binary attribute is not visible. The
+ *		returned value will replace static permissions defined
+ *		in struct bin_attribute. If @is_visible is not set, Use
+ *		SYSFS_GROUP_VISIBLE() when assigning this callback to
+ *		specify separate _group_visible() and _attr_visible()
+ *		handlers.
  * @attrs:	Pointer to NULL terminated list of attributes.
  * @bin_attrs:	Pointer to NULL terminated list of binary attributes.
  *		Either attrs or bin_attrs or both must be provided.
@@ -91,13 +101,42 @@ struct attribute_group {
 	struct bin_attribute	**bin_attrs;
 };
 
+#define SYSFS_PREALLOC		010000
+#define SYSFS_GROUP_INVISIBLE	020000
+
+/*
+ * The first call to is_visible() in the create / update path may
+ * indicate visibility for the entire group
+ */
+#define DEFINE_SYSFS_GROUP_VISIBLE(name)                             \
+	static inline umode_t sysfs_group_visible_##name(            \
+		struct kobject *kobj, struct attribute *attr, int n) \
+	{                                                            \
+		if (n == 0 && !name##_group_visible(kobj))           \
+			return SYSFS_GROUP_INVISIBLE;                \
+		return name##_attr_visible(kobj, attr, n);           \
+	}
+
+/*
+ * Same as DEFINE_SYSFS_GROUP_VISIBLE, but for groups with only binary
+ * attributes
+ */
+#define DEFINE_SYSFS_BIN_GROUP_VISIBLE(name)                             \
+	static inline umode_t sysfs_group_visible_##name(                \
+		struct kobject *kobj, struct bin_attribute *attr, int n) \
+	{                                                                \
+		if (n == 0 && !name##_group_visible(kobj))               \
+			return SYSFS_GROUP_INVISIBLE;                    \
+		return name##_attr_visible(kobj, attr, n);               \
+	}
+
+#define SYSFS_GROUP_VISIBLE(fn) sysfs_group_visible_##fn
+
 /*
  * Use these macros to make defining attributes easier.
  * See include/linux/device.h for examples..
  */
 
-#define SYSFS_PREALLOC 010000
-
 #define __ATTR(_name, _mode, _show, _store) {				\
 	.attr = {.name = __stringify(_name),				\
 		 .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },		\

On Sat, Jan 27, 2024 at 05:42:54PM -0800, Dan Williams wrote:
> Here it is again without being beholden to keeping the old
> WARN()/pr_warn() regime when the group directory is not found.

Thanks for all of the response to my questions, much appreciated.  I
think this looks great, let me test it here and add it to the patch
series which caused me to want to do this in the first place and see how
it goes.  Give me a day or so, thanks so much I owe you a lot for
digging into this!

greg k-h