| Message ID | 20241103-sysfs-const-bin_attr-v2-9-71110628844c@weissschuh.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | sysfs: constify struct bin_attribute (Part 1) | expand |
> 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 Regards, James
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
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 6d39696b43069010b0ad0bdaadcf9002cb70c92c..785408861c01c89fc84c787848243a13c1338367 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -91,9 +91,12 @@ static ssize_t sysfs_kf_bin_read(struct kernfs_open_file *of, char *buf, count = size - pos; } - if (!battr->read) + if (!battr->read && !battr->read_new) return -EIO; + if (battr->read_new) + return battr->read_new(of->file, kobj, battr, buf, pos, count); + return battr->read(of->file, kobj, battr, buf, pos, count); } @@ -152,9 +155,12 @@ static ssize_t sysfs_kf_bin_write(struct kernfs_open_file *of, char *buf, if (!count) return 0; - if (!battr->write) + if (!battr->write && !battr->write_new) return -EIO; + if (battr->write_new) + return battr->write_new(of->file, kobj, battr, buf, pos, count); + return battr->write(of->file, kobj, battr, buf, pos, count); } @@ -323,13 +329,19 @@ int sysfs_add_bin_file_mode_ns(struct kernfs_node *parent, const struct kernfs_ops *ops; struct kernfs_node *kn; + if (battr->read && battr->read_new) + return -EINVAL; + + if (battr->write && battr->write_new) + return -EINVAL; + if (battr->mmap) ops = &sysfs_bin_kfops_mmap; - else if (battr->read && battr->write) + else if ((battr->read || battr->read_new) && (battr->write || battr->write_new)) ops = &sysfs_bin_kfops_rw; - else if (battr->read) + else if (battr->read || battr->read_new) ops = &sysfs_bin_kfops_ro; - else if (battr->write) + else if (battr->write || battr->write_new) ops = &sysfs_bin_kfops_wo; else ops = &sysfs_file_kfops_empty; diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index d17c473c1ef292875475bf3bdf62d07241c13882..d713a6445a6267145a7014f308df3bb25b8c3287 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, \ }
To make it possible to put struct bin_attribute into read-only memory, the sysfs core has to stop passing mutable pointers to the read() and write() callbacks. As there are numerous implementors of these callbacks throughout the tree it's not possible to change all of them at once. To enable a step-by-step transition, add new variants of the read() and write() callbacks which differ only in the constness of the struct bin_attribute argument. As most binary attributes are defined through macros, extend these macros to transparently handle both variants of callbacks to minimize the churn during the transition. As soon as all handlers are switch to the const variant, the non-const one can be removed together with the transition machinery. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- fs/sysfs/file.c | 22 +++++++++++++++++----- include/linux/sysfs.h | 25 +++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 7 deletions(-)