Re: [PATCH v2 09/10] sysfs: bin_attribute: add const read/write callback variants

["Thomas Weißschuh" <linux@weissschuh.net>]

On 2024-12-03 11:06:16-0500, James Bottomley wrote:
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index
> > d17c473c1ef292875475bf3bdf62d07241c13882..d713a6445a6267145a7014f308d
> > f3bb25b8c3287 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -305,8 +305,12 @@ struct bin_attribute {
> >  	struct address_space *(*f_mapping)(void);
> >  	ssize_t (*read)(struct file *, struct kobject *, struct
> > bin_attribute *,
> >  			char *, loff_t, size_t);
> > +	ssize_t (*read_new)(struct file *, struct kobject *, const
> > struct bin_attribute *,
> > +			    char *, loff_t, size_t);
> >  	ssize_t (*write)(struct file *, struct kobject *, struct
> > bin_attribute *,
> >  			 char *, loff_t, size_t);
> > +	ssize_t (*write_new)(struct file *, struct kobject *,
> > +			     const struct bin_attribute *, char *,
> > loff_t, size_t);
> >  	loff_t (*llseek)(struct file *, struct kobject *, const
> > struct bin_attribute *,
> >  			 loff_t, int);
> >  	int (*mmap)(struct file *, struct kobject *, const struct
> > bin_attribute *attr,
> > @@ -325,11 +329,28 @@ struct bin_attribute {
> >   */
> >  #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)-
> > >attr)
> >  
> > +typedef ssize_t __sysfs_bin_rw_handler_new(struct file *, struct
> > kobject *,
> > +					   const struct
> > bin_attribute *, char *, loff_t, size_t);
> > +
> >  /* macros to create static binary attributes easier */
> >  #define __BIN_ATTR(_name, _mode, _read, _write, _size)
> > {		\
> >  	.attr = { .name = __stringify(_name), .mode = _mode
> > },		\
> > -	.read	=
> > _read,						\
> > -	.write	=
> > _write,						\
> > +	.read =
> > _Generic(_read,						\
> > +		__sysfs_bin_rw_handler_new * :
> > NULL,			\
> > +		default :
> > _read						\
> > +	),							
> > 	\
> > +	.read_new =
> > _Generic(_read,					\
> > +		__sysfs_bin_rw_handler_new * :
> > _read,			\
> > +		default :
> > NULL						\
> > +	),							
> > 	\
> > +	.write =
> > _Generic(_write,					\
> > +		__sysfs_bin_rw_handler_new * :
> > NULL,			\
> > +		default :
> > _write					\
> > +	),							
> > 	\
> > +	.write_new =
> > _Generic(_write,					\
> > +		__sysfs_bin_rw_handler_new * :
> > _write,			\
> > +		default :
> > NULL						\
> > +	),							
> > 	\
> >  	.size	=
> > _size,						\
> >  }
> 
> It's probably a bit late now, but you've done this the wrong way
> around.  What you should have done is added the const to .read/.write
> then added a .read_old/.write_old with the original function prototype
> and used _Generic() to switch between them.  Then when there are no
> more non const left, you can simply remove .read_old and .write_old
> without getting Linus annoyed by having to do something like this:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e70140ba0d2b1a30467d4af6bcfe761327b9ec95

Not all users are using the macros to define their attributes.
(Nor do they want to)

These users would break with your suggestion.
Otherwise I agree.


Thomas