Re: [PATCH net-next 2/2] net sysfs: simplify creating empty wireless directory

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The kernel-doc for struct attribute_group in
  include/linux/sysfs.h (line 103) still says "@bin_attrs: ... Either…

--

net sysfs: simplify creating empty wireless directory

The patch removes the dummy wireless_attrs[] list from wireless_group in
net/core/net-sysfs.c. It relies on the earlier sysfs change that lets a
named attribute group with NULL attrs create an empty directory.

> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 5c0402389c6d1..f58b59e112620 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -910,13 +910,8 @@ static const struct attribute_group netstat_group = {
>  	.attrs  = netstat_attrs,
>  };
>  
> -static struct attribute *wireless_attrs[] = {
> -	NULL
> -};
> -
>  static const struct attribute_group wireless_group = {
>  	.name = "wireless",
> -	.attrs = wireless_attrs,
>  };

[Severity: Low]
This isn't a bug, but should the kernel-doc for struct attribute_group be
updated along with this change?

After this patch, wireless_group has both .attrs and .bin_attrs set to NULL.
The comment above struct attribute_group in include/linux/sysfs.h still
says:

 * @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.

The new behaviour is only described by an inline comment in
internal_create_group() in fs/sysfs/group.c:

	/* If name is set w/o attributes, create an empty directory */
	if (!grp->name && !grp->attrs && !grp->bin_attrs) {

The paths wireless_group actually goes through (create_files(),
remove_files(), sysfs_group_attrs_change_owner()) all check for NULL attrs
and bin_attrs, so nothing goes wrong at runtime here. Other helpers do not
tolerate NULL attrs, though. For example, sysfs_merge_group() dereferences
grp->attrs without a NULL check:

	for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))

The same applies to sysfs_unmerge_group().

Could the @attrs/@bin_attrs kernel-doc be changed to say that a named group
may leave both NULL to get an empty directory? Could it also say which
helpers still require attrs to be set? Neither this patch nor the earlier
"sysfs: facilitate creating an empty directory" patch updates the
documentation, and this patch is the first in-tree user of the new
behaviour.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0a0a7987-abb2-49c8-bc16-3938233ae9c5%40gmail.com