[PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Thomas Weißschuh]

Most users use this function through the BIN_ATTR_SIMPLE* macros,
they can handle the switch transparently.

This series is meant to be merged through the driver core tree.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Rebase on torvalds/master
- Drop wmi-bmof patch
- Pick up Acks from Andrii
- Link to v1: https://lore.kernel.org/r/20241205-sysfs-const-bin_attr-simple-v1-0-4a4e4ced71e3@weissschuh.net

---
Thomas Weißschuh (3):
      sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()
      btf: Switch vmlinux BTF attribute to sysfs_bin_attr_simple_read()
      btf: Switch module BTF attribute to sysfs_bin_attr_simple_read()

 arch/powerpc/platforms/powernv/opal.c |  2 +-
 fs/sysfs/file.c                       |  2 +-
 include/linux/sysfs.h                 |  4 ++--
 kernel/bpf/btf.c                      | 15 ++-------------
 kernel/bpf/sysfs_btf.c                | 12 ++----------
 kernel/module/sysfs.c                 |  2 +-
 6 files changed, 9 insertions(+), 28 deletions(-)
---
base-commit: d6ef8b40d075c425f548002d2f35ae3f06e9cf96
change-id: 20241122-sysfs-const-bin_attr-simple-7c0ddb2fcf12

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Thomas Weißschuh]

Most users use this function through the BIN_ATTR_SIMPLE* macros,
they can handle the switch transparently.
Also adapt the two non-macro users in the same change.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 arch/powerpc/platforms/powernv/opal.c | 2 +-
 fs/sysfs/file.c                       | 2 +-
 include/linux/sysfs.h                 | 4 ++--
 kernel/module/sysfs.c                 | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 5d0f35bb917ebced8c741cd3af2c511949a1d2ef..013637e2b2a8e6a4ec6b93a520f8d5d9d3245467 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -818,7 +818,7 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
 	sysfs_bin_attr_init(attr);
 	attr->attr.name = name;
 	attr->attr.mode = 0400;
-	attr->read = sysfs_bin_attr_simple_read;
+	attr->read_new = sysfs_bin_attr_simple_read;
 	attr->private = __va(vals[0]);
 	attr->size = vals[1];
 
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 785408861c01c89fc84c787848243a13c1338367..6931308876c4ac3b4c19878d5e1158ad8fe4f16f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -817,7 +817,7 @@ EXPORT_SYMBOL_GPL(sysfs_emit_at);
  * Returns number of bytes written to @buf.
  */
 ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
-				   struct bin_attribute *attr, char *buf,
+				   const struct bin_attribute *attr, char *buf,
 				   loff_t off, size_t count)
 {
 	memcpy(buf, attr->private + off, count);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 0f2fcd244523f050c5286f19d4fe1846506f9214..2205561159afdb57d0a250bb0439b28c01d9010e 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -511,7 +511,7 @@ __printf(3, 4)
 int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
 
 ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
-				   struct bin_attribute *attr, char *buf,
+				   const struct bin_attribute *attr, char *buf,
 				   loff_t off, size_t count);
 
 #else /* CONFIG_SYSFS */
@@ -774,7 +774,7 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
 
 static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
 						 struct kobject *kobj,
-						 struct bin_attribute *attr,
+						 const struct bin_attribute *attr,
 						 char *buf, loff_t off,
 						 size_t count)
 {
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 456358e1fdc43e6b5b24f383bbefa37812971174..254017b58b645d4afcf6876d29bcc2e2113a8dc4 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -196,7 +196,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
 			nattr->attr.mode = 0444;
 			nattr->size = info->sechdrs[i].sh_size;
 			nattr->private = (void *)info->sechdrs[i].sh_addr;
-			nattr->read = sysfs_bin_attr_simple_read;
+			nattr->read_new = sysfs_bin_attr_simple_read;
 			++nattr;
 		}
 		++loaded;

-- 
2.47.1

[PATCH v2 2/3] btf: Switch vmlinux BTF attribute to sysfs_bin_attr_simple_read()

[Thomas Weißschuh]

The generic function from the sysfs core can replace the custom one.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
This is a replacement for [0], as Alexei was not happy about BIN_ATTR_SIMPLE_RO()

[0] https://lore.kernel.org/lkml/20241122-sysfs-const-bin_attr-bpf-v1-1-823aea399b53@weissschuh.net/
---
 kernel/bpf/sysfs_btf.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/sysfs_btf.c b/kernel/bpf/sysfs_btf.c
index fedb54c94cdb830a4890d33677dcc5a6e236c13f..81d6cf90584a7157929c50f62a5c6862e7a3d081 100644
--- a/kernel/bpf/sysfs_btf.c
+++ b/kernel/bpf/sysfs_btf.c
@@ -12,24 +12,16 @@
 extern char __start_BTF[];
 extern char __stop_BTF[];
 
-static ssize_t
-btf_vmlinux_read(struct file *file, struct kobject *kobj,
-		 struct bin_attribute *bin_attr,
-		 char *buf, loff_t off, size_t len)
-{
-	memcpy(buf, __start_BTF + off, len);
-	return len;
-}
-
 static struct bin_attribute bin_attr_btf_vmlinux __ro_after_init = {
 	.attr = { .name = "vmlinux", .mode = 0444, },
-	.read = btf_vmlinux_read,
+	.read_new = sysfs_bin_attr_simple_read,
 };
 
 struct kobject *btf_kobj;
 
 static int __init btf_vmlinux_init(void)
 {
+	bin_attr_btf_vmlinux.private = __start_BTF;
 	bin_attr_btf_vmlinux.size = __stop_BTF - __start_BTF;
 
 	if (bin_attr_btf_vmlinux.size == 0)

-- 
2.47.1

[PATCH v2 3/3] btf: Switch module BTF attribute to sysfs_bin_attr_simple_read()

[Thomas Weißschuh]

The generic function from the sysfs core can replace the custom one.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/btf.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e5a5f023cedd5c288c2774218862c69db469917f..76de36d2552002d1e25c826d701340d9cde07ba1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -8011,17 +8011,6 @@ struct btf_module {
 static LIST_HEAD(btf_modules);
 static DEFINE_MUTEX(btf_module_mutex);
 
-static ssize_t
-btf_module_read(struct file *file, struct kobject *kobj,
-		struct bin_attribute *bin_attr,
-		char *buf, loff_t off, size_t len)
-{
-	const struct btf *btf = bin_attr->private;
-
-	memcpy(buf, btf->data + off, len);
-	return len;
-}
-
 static void purge_cand_cache(struct btf *btf);
 
 static int btf_module_notify(struct notifier_block *nb, unsigned long op,
@@ -8082,8 +8071,8 @@ static int btf_module_notify(struct notifier_block *nb, unsigned long op,
 			attr->attr.name = btf->name;
 			attr->attr.mode = 0444;
 			attr->size = btf->data_size;
-			attr->private = btf;
-			attr->read = btf_module_read;
+			attr->private = btf->data;
+			attr->read_new = sysfs_bin_attr_simple_read;
 
 			err = sysfs_create_bin_file(btf_kobj, attr);
 			if (err) {

-- 
2.47.1

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Alexei Starovoitov]

On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Most users use this function through the BIN_ATTR_SIMPLE* macros,
> they can handle the switch transparently.
>
> This series is meant to be merged through the driver core tree.

hmm. why?

I'd rather take patches 2 and 3 into bpf-next to avoid
potential conflicts.
Patch 1 looks orthogonal and independent.

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Thomas Weißschuh]

On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote:
> On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > Most users use this function through the BIN_ATTR_SIMPLE* macros,
> > they can handle the switch transparently.
> >
> > This series is meant to be merged through the driver core tree.
> 
> hmm. why?

Patch 1 changes the signature of sysfs_bin_attr_simple_read().
Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the
callback member .read, after patch 1 it's .read_new.
(Both callbacks work exactly the same, except for their signature,
.read_new is only a transition mechanism and will go away again)

> I'd rather take patches 2 and 3 into bpf-next to avoid
> potential conflicts.
> Patch 1 looks orthogonal and independent.

If you pick up 2 and 3 through bpf-next you would need to adapt these
assignments. As soon as both patch 1 and the modified 2 and 3 hit
Linus' tree, the build would break due to mismatches function pointers.
(Casting function pointers to avoid the mismatch will blow up with KCFI)
Of course Linus can fix this up easily, but it somebody would need to
keep track of it and I wanted to avoid manual intervention.
Or we spread out both parts over two development cycles; maybe Greg can
even pick up patch 1 late in the 6.13 cycle.

Personally I am fine with any approach.

Re: [PATCH v2 1/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Aditya Gupta]

Looks good to me. Did boot test and reading the /sys files works.

Linux-ci tests [0] are also good (the failing tests are broken from
some time, ignoring them):

[0]: https://github.com/adi-g15-ibm/linux-ci/actions?query=branch%3Atmp-test-branch-10962+branch%3Atmp-test-branch-26310+branch%3Atmp-test-branch-23431++

Tested-by: Aditya Gupta <adityagupta@ibm.com>

Thanks,
- Aditya G

On 24/12/28 09:43AM, Thomas Weißschuh wrote:
> Most users use this function through the BIN_ATTR_SIMPLE* macros,
> they can handle the switch transparently.
> Also adapt the two non-macro users in the same change.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  arch/powerpc/platforms/powernv/opal.c | 2 +-
>  fs/sysfs/file.c                       | 2 +-
>  include/linux/sysfs.h                 | 4 ++--
>  kernel/module/sysfs.c                 | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 5d0f35bb917ebced8c741cd3af2c511949a1d2ef..013637e2b2a8e6a4ec6b93a520f8d5d9d3245467 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -818,7 +818,7 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
>  	sysfs_bin_attr_init(attr);
>  	attr->attr.name = name;
>  	attr->attr.mode = 0400;
> -	attr->read = sysfs_bin_attr_simple_read;
> +	attr->read_new = sysfs_bin_attr_simple_read;
>  	attr->private = __va(vals[0]);
>  	attr->size = vals[1];
>  
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 785408861c01c89fc84c787848243a13c1338367..6931308876c4ac3b4c19878d5e1158ad8fe4f16f 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -817,7 +817,7 @@ EXPORT_SYMBOL_GPL(sysfs_emit_at);
>   * Returns number of bytes written to @buf.
>   */
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count)
>  {
>  	memcpy(buf, attr->private + off, count);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 0f2fcd244523f050c5286f19d4fe1846506f9214..2205561159afdb57d0a250bb0439b28c01d9010e 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -511,7 +511,7 @@ __printf(3, 4)
>  int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
>  
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count);
>  
>  #else /* CONFIG_SYSFS */
> @@ -774,7 +774,7 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
>  
>  static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
>  						 struct kobject *kobj,
> -						 struct bin_attribute *attr,
> +						 const struct bin_attribute *attr,
>  						 char *buf, loff_t off,
>  						 size_t count)
>  {
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index 456358e1fdc43e6b5b24f383bbefa37812971174..254017b58b645d4afcf6876d29bcc2e2113a8dc4 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -196,7 +196,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>  			nattr->attr.mode = 0444;
>  			nattr->size = info->sechdrs[i].sh_size;
>  			nattr->private = (void *)info->sechdrs[i].sh_addr;
> -			nattr->read = sysfs_bin_attr_simple_read;
> +			nattr->read_new = sysfs_bin_attr_simple_read;
>  			++nattr;
>  		}
>  		++loaded;
> 
> -- 
> 2.47.1
> 

Re: [PATCH v2 1/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Aditya Gupta]

Hi,

Please use this Tested-by instead of the previous one i sent:

Tested-by: Aditya Gupta <adityag@linux.ibm.com>


Thanks,

- Aditya G

On 02/01/25 11:07, Aditya Gupta wrote:
> Looks good to me. Did boot test and reading the /sys files works.
>
> Linux-ci tests [0] are also good (the failing tests are broken from
> some time, ignoring them):
>
> [0]: https://github.com/adi-g15-ibm/linux-ci/actions?query=branch%3Atmp-test-branch-10962+branch%3Atmp-test-branch-26310+branch%3Atmp-test-branch-23431++
>
> Tested-by: Aditya Gupta <adityagupta@ibm.com>
>
> Thanks,
> - Aditya G
>
> On 24/12/28 09:43AM, Thomas Weißschuh wrote:
>> Most users use this function through the BIN_ATTR_SIMPLE* macros,
>> they can handle the switch transparently.
>> Also adapt the two non-macro users in the same change.
>>
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> ---
>>   arch/powerpc/platforms/powernv/opal.c | 2 +-
>>   fs/sysfs/file.c                       | 2 +-
>>   include/linux/sysfs.h                 | 4 ++--
>>   kernel/module/sysfs.c                 | 2 +-
>>   4 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
>> index 5d0f35bb917ebced8c741cd3af2c511949a1d2ef..013637e2b2a8e6a4ec6b93a520f8d5d9d3245467 100644
>> --- a/arch/powerpc/platforms/powernv/opal.c
>> +++ b/arch/powerpc/platforms/powernv/opal.c
>> @@ -818,7 +818,7 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
>>   	sysfs_bin_attr_init(attr);
>>   	attr->attr.name = name;
>>   	attr->attr.mode = 0400;
>> -	attr->read = sysfs_bin_attr_simple_read;
>> +	attr->read_new = sysfs_bin_attr_simple_read;
>>   	attr->private = __va(vals[0]);
>>   	attr->size = vals[1];
>>   
>> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
>> index 785408861c01c89fc84c787848243a13c1338367..6931308876c4ac3b4c19878d5e1158ad8fe4f16f 100644
>> --- a/fs/sysfs/file.c
>> +++ b/fs/sysfs/file.c
>> @@ -817,7 +817,7 @@ EXPORT_SYMBOL_GPL(sysfs_emit_at);
>>    * Returns number of bytes written to @buf.
>>    */
>>   ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
>> -				   struct bin_attribute *attr, char *buf,
>> +				   const struct bin_attribute *attr, char *buf,
>>   				   loff_t off, size_t count)
>>   {
>>   	memcpy(buf, attr->private + off, count);
>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>> index 0f2fcd244523f050c5286f19d4fe1846506f9214..2205561159afdb57d0a250bb0439b28c01d9010e 100644
>> --- a/include/linux/sysfs.h
>> +++ b/include/linux/sysfs.h
>> @@ -511,7 +511,7 @@ __printf(3, 4)
>>   int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
>>   
>>   ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
>> -				   struct bin_attribute *attr, char *buf,
>> +				   const struct bin_attribute *attr, char *buf,
>>   				   loff_t off, size_t count);
>>   
>>   #else /* CONFIG_SYSFS */
>> @@ -774,7 +774,7 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
>>   
>>   static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
>>   						 struct kobject *kobj,
>> -						 struct bin_attribute *attr,
>> +						 const struct bin_attribute *attr,
>>   						 char *buf, loff_t off,
>>   						 size_t count)
>>   {
>> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
>> index 456358e1fdc43e6b5b24f383bbefa37812971174..254017b58b645d4afcf6876d29bcc2e2113a8dc4 100644
>> --- a/kernel/module/sysfs.c
>> +++ b/kernel/module/sysfs.c
>> @@ -196,7 +196,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>>   			nattr->attr.mode = 0444;
>>   			nattr->size = info->sechdrs[i].sh_size;
>>   			nattr->private = (void *)info->sechdrs[i].sh_addr;
>> -			nattr->read = sysfs_bin_attr_simple_read;
>> +			nattr->read_new = sysfs_bin_attr_simple_read;
>>   			++nattr;
>>   		}
>>   		++loaded;
>>
>> -- 
>> 2.47.1
>>

Re: [PATCH v2 1/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Mahesh J Salgaonkar]

On 2024-12-28 09:43:41 Sat, Thomas Weißschuh wrote:
> Most users use this function through the BIN_ATTR_SIMPLE* macros,
> they can handle the switch transparently.
> Also adapt the two non-macro users in the same change.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  arch/powerpc/platforms/powernv/opal.c | 2 +-
>  fs/sysfs/file.c                       | 2 +-
>  include/linux/sysfs.h                 | 4 ++--
>  kernel/module/sysfs.c                 | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)

Looks good to me.

Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>

Thanks,
-Mahesh.

> 
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 5d0f35bb917ebced8c741cd3af2c511949a1d2ef..013637e2b2a8e6a4ec6b93a520f8d5d9d3245467 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -818,7 +818,7 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
>  	sysfs_bin_attr_init(attr);
>  	attr->attr.name = name;
>  	attr->attr.mode = 0400;
> -	attr->read = sysfs_bin_attr_simple_read;
> +	attr->read_new = sysfs_bin_attr_simple_read;
>  	attr->private = __va(vals[0]);
>  	attr->size = vals[1];
>  
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 785408861c01c89fc84c787848243a13c1338367..6931308876c4ac3b4c19878d5e1158ad8fe4f16f 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -817,7 +817,7 @@ EXPORT_SYMBOL_GPL(sysfs_emit_at);
>   * Returns number of bytes written to @buf.
>   */
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count)
>  {
>  	memcpy(buf, attr->private + off, count);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 0f2fcd244523f050c5286f19d4fe1846506f9214..2205561159afdb57d0a250bb0439b28c01d9010e 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -511,7 +511,7 @@ __printf(3, 4)
>  int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
>  
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count);
>  
>  #else /* CONFIG_SYSFS */
> @@ -774,7 +774,7 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
>  
>  static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
>  						 struct kobject *kobj,
> -						 struct bin_attribute *attr,
> +						 const struct bin_attribute *attr,
>  						 char *buf, loff_t off,
>  						 size_t count)
>  {
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index 456358e1fdc43e6b5b24f383bbefa37812971174..254017b58b645d4afcf6876d29bcc2e2113a8dc4 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -196,7 +196,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>  			nattr->attr.mode = 0444;
>  			nattr->size = info->sechdrs[i].sh_size;
>  			nattr->private = (void *)info->sechdrs[i].sh_addr;
> -			nattr->read = sysfs_bin_attr_simple_read;
> +			nattr->read_new = sysfs_bin_attr_simple_read;
>  			++nattr;
>  		}
>  		++loaded;
> 
> -- 
> 2.47.1
> 
> 

-- 
Mahesh J Salgaonkar

Re: [PATCH v2 1/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Madhavan Srinivasan]

On 12/28/24 2:13 PM, Thomas Weißschuh wrote:
> Most users use this function through the BIN_ATTR_SIMPLE* macros,
> they can handle the switch transparently.
> Also adapt the two non-macro users in the same change.

Changes looks fine to me.

Acked-by: Madhavan Srinivasan <maddy@linux.ibm.com>

> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  arch/powerpc/platforms/powernv/opal.c | 2 +-
>  fs/sysfs/file.c                       | 2 +-
>  include/linux/sysfs.h                 | 4 ++--
>  kernel/module/sysfs.c                 | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 5d0f35bb917ebced8c741cd3af2c511949a1d2ef..013637e2b2a8e6a4ec6b93a520f8d5d9d3245467 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -818,7 +818,7 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
>  	sysfs_bin_attr_init(attr);
>  	attr->attr.name = name;
>  	attr->attr.mode = 0400;
> -	attr->read = sysfs_bin_attr_simple_read;
> +	attr->read_new = sysfs_bin_attr_simple_read;
>  	attr->private = __va(vals[0]);
>  	attr->size = vals[1];
>  
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 785408861c01c89fc84c787848243a13c1338367..6931308876c4ac3b4c19878d5e1158ad8fe4f16f 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -817,7 +817,7 @@ EXPORT_SYMBOL_GPL(sysfs_emit_at);
>   * Returns number of bytes written to @buf.
>   */
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count)
>  {
>  	memcpy(buf, attr->private + off, count);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 0f2fcd244523f050c5286f19d4fe1846506f9214..2205561159afdb57d0a250bb0439b28c01d9010e 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -511,7 +511,7 @@ __printf(3, 4)
>  int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
>  
>  ssize_t sysfs_bin_attr_simple_read(struct file *file, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf,
> +				   const struct bin_attribute *attr, char *buf,
>  				   loff_t off, size_t count);
>  
>  #else /* CONFIG_SYSFS */
> @@ -774,7 +774,7 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
>  
>  static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
>  						 struct kobject *kobj,
> -						 struct bin_attribute *attr,
> +						 const struct bin_attribute *attr,
>  						 char *buf, loff_t off,
>  						 size_t count)
>  {
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index 456358e1fdc43e6b5b24f383bbefa37812971174..254017b58b645d4afcf6876d29bcc2e2113a8dc4 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -196,7 +196,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>  			nattr->attr.mode = 0444;
>  			nattr->size = info->sechdrs[i].sh_size;
>  			nattr->private = (void *)info->sechdrs[i].sh_addr;
> -			nattr->read = sysfs_bin_attr_simple_read;
> +			nattr->read_new = sysfs_bin_attr_simple_read;
>  			++nattr;
>  		}
>  		++loaded;
> 

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Alexei Starovoitov]

On Tue, Dec 31, 2024 at 2:30 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote:
> > On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > Most users use this function through the BIN_ATTR_SIMPLE* macros,
> > > they can handle the switch transparently.
> > >
> > > This series is meant to be merged through the driver core tree.
> >
> > hmm. why?
>
> Patch 1 changes the signature of sysfs_bin_attr_simple_read().
> Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the
> callback member .read, after patch 1 it's .read_new.
> (Both callbacks work exactly the same, except for their signature,
> .read_new is only a transition mechanism and will go away again)
>
> > I'd rather take patches 2 and 3 into bpf-next to avoid
> > potential conflicts.
> > Patch 1 looks orthogonal and independent.
>
> If you pick up 2 and 3 through bpf-next you would need to adapt these
> assignments. As soon as both patch 1 and the modified 2 and 3 hit
> Linus' tree, the build would break due to mismatches function pointers.
> (Casting function pointers to avoid the mismatch will blow up with KCFI)

I see. All these steps to constify is frankly a mess.
You're wasting cpu and memory for this read vs read_new
when const is not much more than syntactic sugar in C.
You should have done one tree wide patch without doing this _new() hack.

Anyway, rant over. Carry patches 2,3. Hopefully they won't conflict.
But I don't want to see any constification patches in bpf land
that come with such pointless runtime penalty.

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Greg Kroah-Hartman]

On Wed, Jan 08, 2025 at 09:45:45AM -0800, Alexei Starovoitov wrote:
> On Tue, Dec 31, 2024 at 2:30 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote:
> > > On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > >
> > > > Most users use this function through the BIN_ATTR_SIMPLE* macros,
> > > > they can handle the switch transparently.
> > > >
> > > > This series is meant to be merged through the driver core tree.
> > >
> > > hmm. why?
> >
> > Patch 1 changes the signature of sysfs_bin_attr_simple_read().
> > Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the
> > callback member .read, after patch 1 it's .read_new.
> > (Both callbacks work exactly the same, except for their signature,
> > .read_new is only a transition mechanism and will go away again)
> >
> > > I'd rather take patches 2 and 3 into bpf-next to avoid
> > > potential conflicts.
> > > Patch 1 looks orthogonal and independent.
> >
> > If you pick up 2 and 3 through bpf-next you would need to adapt these
> > assignments. As soon as both patch 1 and the modified 2 and 3 hit
> > Linus' tree, the build would break due to mismatches function pointers.
> > (Casting function pointers to avoid the mismatch will blow up with KCFI)
> 
> I see. All these steps to constify is frankly a mess.
> You're wasting cpu and memory for this read vs read_new
> when const is not much more than syntactic sugar in C.
> You should have done one tree wide patch without doing this _new() hack.
> 
> Anyway, rant over. Carry patches 2,3. Hopefully they won't conflict.
> But I don't want to see any constification patches in bpf land
> that come with such pointless runtime penalty.

The "pointless" penalty will go away once we convert all instances, and
really, it's just one pointer check, sysfs files should NOT be a hot
path for anything real, and one more pointer check should be cached and
not measurable compared to the real logic behind the binary data coming
from the hardware/kernel, right?

sysfs is NOT tuned for speed at all, so adding more checks like this
should be fine.

thanks,

greg k-h

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Christoph Hellwig]

On Thu, Jan 09, 2025 at 08:56:37AM +0100, Greg Kroah-Hartman wrote:
> The "pointless" penalty will go away once we convert all instances, and
> really, it's just one pointer check, sysfs files should NOT be a hot
> path for anything real, and one more pointer check should be cached and
> not measurable compared to the real logic behind the binary data coming
> from the hardware/kernel, right?
> 
> sysfs is NOT tuned for speed at all, so adding more checks like this
> should be fine.

Hey, when I duplicated the method to convert sysfs over to a proper
seq_file based approach that avoids buffer overflows you basically
came up with the same line that Alexei had here.  And that is a lot
more useful than constification. Not that I mind the latter, but it
would be better if it could be done without leaving both variants
in for long.

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Greg Kroah-Hartman]

On Thu, Jan 09, 2025 at 12:06:09AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 09, 2025 at 08:56:37AM +0100, Greg Kroah-Hartman wrote:
> > The "pointless" penalty will go away once we convert all instances, and
> > really, it's just one pointer check, sysfs files should NOT be a hot
> > path for anything real, and one more pointer check should be cached and
> > not measurable compared to the real logic behind the binary data coming
> > from the hardware/kernel, right?
> > 
> > sysfs is NOT tuned for speed at all, so adding more checks like this
> > should be fine.
> 
> Hey, when I duplicated the method to convert sysfs over to a proper
> seq_file based approach that avoids buffer overflows you basically
> came up with the same line that Alexei had here.

I did?  Sorry about that, I don't remember that.

> And that is a lot
> more useful than constification. Not that I mind the latter, but it
> would be better if it could be done without leaving both variants
> in for long.

I agree, we should get the read_new stuff out in the next kernel cycle I
hope.

As for seq_file for sysfs, is that for binary attributes only, or for
all?  I can't recall that at all.

thanks,

greg k-h

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Christoph Hellwig]

On Thu, Jan 09, 2025 at 09:12:03AM +0100, Greg Kroah-Hartman wrote:
> > Hey, when I duplicated the method to convert sysfs over to a proper
> > seq_file based approach that avoids buffer overflows you basically
> > came up with the same line that Alexei had here.
> 
> I did?  Sorry about that, I don't remember that.

It's been a while..

> As for seq_file for sysfs, is that for binary attributes only, or for
> all?  I can't recall that at all.

Non-binary ones.

Re: [PATCH v2 0/3] sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read()

[Greg Kroah-Hartman]

On Thu, Jan 09, 2025 at 12:23:01AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 09, 2025 at 09:12:03AM +0100, Greg Kroah-Hartman wrote:
> > > Hey, when I duplicated the method to convert sysfs over to a proper
> > > seq_file based approach that avoids buffer overflows you basically
> > > came up with the same line that Alexei had here.
> > 
> > I did?  Sorry about that, I don't remember that.
> 
> It's been a while..
> 
> > As for seq_file for sysfs, is that for binary attributes only, or for
> > all?  I can't recall that at all.
> 
> Non-binary ones.

Ah, yeah, well the churn for "one single value" sysfs files would be
rough and seq_file doesn't really do much, if anything, for them as they
should be all simple strings that never overflow or are complex.

Yes, there are exceptions, so maybe for just them?  I don't want to make
it easier to abuse sysfs files, but if you feel it would really help
out, I'm willing to reconsider it.

thanks,

greg k-h


>