| Message ID | 20241103-sysfs-const-bin_attr-v2-2-71110628844c@weissschuh.net |
|---|---|
| State | New |
| Headers | show |
| Series | sysfs: constify struct bin_attribute (Part 1) | expand |
On Sun, Nov 03, 2024 at 05:03:31PM +0000, Thomas Weißschuh wrote: > Several drivers need to dynamically calculate the size of an binary > attribute. Currently this is done by assigning attr->size from the > is_bin_visible() callback. s/an binary/a binary/ > This has drawbacks: > * It is not documented. > * A single attribute can be instantiated multiple times, overwriting the > shared size field. > * It prevents the structure to be moved to read-only memory. > > Introduce a new dedicated callback to calculate the size of the > attribute. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > fs/sysfs/group.c | 2 ++ > include/linux/sysfs.h | 8 ++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index 45b2e92941da1f49dcc71af3781317c61480c956..8b01a7eda5fb3239e138372417d01967c7a3f122 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -98,6 +98,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj, > if (!mode) > continue; > } > + if (grp->bin_size) > + size = grp->bin_size(kobj, *bin_attr, i); > > WARN(mode & ~(SYSFS_PREALLOC | 0664), > "Attribute %s: Invalid permissions 0%o\n", > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index c4e64dc112063f7cb89bf66059d0338716089e87..4746cccb95898b24df6f53de9421ea7649b5568f 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -87,6 +87,11 @@ do { \ > * SYSFS_GROUP_VISIBLE() when assigning this callback to > * specify separate _group_visible() and _attr_visible() > * handlers. > + * @bin_size: > + * Optional: Function to return the size of a binary attribute > + * of the group. Will be called repeatedly for each binary > + * attribute in the group. Overwrites the size field embedded > + * inside the attribute itself. "Overwrites" suggests that we write over the size field in the single shared attribute. But that's not what create_files() does. create_files() instantiates sysfs files from the attribute template. Previously each instance used the size from the shared attribute. With this patch, if ->bin_size() exists, its return value is the size of this particular instance, over*riding* the default size from the shared attribute. This description follows the language of other function pointers, which was the right approach. But I think the existing language would be more helpful if it called out the difference between the attribute itself (a potentially read-only singleton structure shared by all kobjects with this attribute) and the instantiation of that attribute for each kobject. For example, @bin_size: Optional: Function to return the size of this kobject's instantiation of a binary attribute. If present, it is called for each bin_attribute in the group and overrides the default size from the bin_attribute template. This is nice work, thanks for doing it! > * @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. > @@ -97,6 +102,9 @@ struct attribute_group { > struct attribute *, int); > umode_t (*is_bin_visible)(struct kobject *, > struct bin_attribute *, int); > + size_t (*bin_size)(struct kobject *, > + const struct bin_attribute *, > + int); > struct attribute **attrs; > struct bin_attribute **bin_attrs; > }; > > -- > 2.47.0 >
Am 03.11.24 um 18:03 schrieb Thomas Weißschuh: > Several drivers need to dynamically calculate the size of an binary > attribute. Currently this is done by assigning attr->size from the > is_bin_visible() callback. Hi, i really like your idea of introducing this new callback, it will be very useful for the wmi-bmof driver :). Thanks, Armin Wolf > > This has drawbacks: > * It is not documented. > * A single attribute can be instantiated multiple times, overwriting the > shared size field. > * It prevents the structure to be moved to read-only memory. > > Introduce a new dedicated callback to calculate the size of the > attribute. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > fs/sysfs/group.c | 2 ++ > include/linux/sysfs.h | 8 ++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index 45b2e92941da1f49dcc71af3781317c61480c956..8b01a7eda5fb3239e138372417d01967c7a3f122 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -98,6 +98,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj, > if (!mode) > continue; > } > + if (grp->bin_size) > + size = grp->bin_size(kobj, *bin_attr, i); > > WARN(mode & ~(SYSFS_PREALLOC | 0664), > "Attribute %s: Invalid permissions 0%o\n", > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h > index c4e64dc112063f7cb89bf66059d0338716089e87..4746cccb95898b24df6f53de9421ea7649b5568f 100644 > --- a/include/linux/sysfs.h > +++ b/include/linux/sysfs.h > @@ -87,6 +87,11 @@ do { \ > * SYSFS_GROUP_VISIBLE() when assigning this callback to > * specify separate _group_visible() and _attr_visible() > * handlers. > + * @bin_size: > + * Optional: Function to return the size of a binary attribute > + * of the group. Will be called repeatedly for each binary > + * attribute in the group. Overwrites the size field embedded > + * inside the attribute itself. > * @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. > @@ -97,6 +102,9 @@ struct attribute_group { > struct attribute *, int); > umode_t (*is_bin_visible)(struct kobject *, > struct bin_attribute *, int); > + size_t (*bin_size)(struct kobject *, > + const struct bin_attribute *, > + int); > struct attribute **attrs; > struct bin_attribute **bin_attrs; > }; >
Hello, > Several drivers need to dynamically calculate the size of an binary > attribute. Currently this is done by assigning attr->size from the > is_bin_visible() callback. > > This has drawbacks: > * It is not documented. > * A single attribute can be instantiated multiple times, overwriting the > shared size field. > * It prevents the structure to be moved to read-only memory. > > Introduce a new dedicated callback to calculate the size of the > attribute. Would it be possible to have a helper that when run against a specific kobject reference, then it would refresh or re-run the size callbacks? We have an use case where we resize BARs on demand via sysfs, and currently the only way to update the size of each resource sysfs object is to remove and added them again, which is a bit crude, and can also be unsafe. Hence the question. There exist the sysfs_update_groups(), but the BAR resource sysfs objects are currently, at least not yet, added to any attribute group. Thank you! Krzysztof
On Thu, Nov 07, 2024 at 05:05:13AM +0900, Krzysztof Wilczyński wrote: > Hello, > > > Several drivers need to dynamically calculate the size of an binary > > attribute. Currently this is done by assigning attr->size from the > > is_bin_visible() callback. > > > > This has drawbacks: > > * It is not documented. > > * A single attribute can be instantiated multiple times, overwriting the > > shared size field. > > * It prevents the structure to be moved to read-only memory. > > > > Introduce a new dedicated callback to calculate the size of the > > attribute. > > Would it be possible to have a helper that when run against a specific > kobject reference, then it would refresh or re-run the size callbacks? > > We have an use case where we resize BARs on demand via sysfs, and currently > the only way to update the size of each resource sysfs object is to remove > and added them again, which is a bit crude, and can also be unsafe. How is it unsafe? > Hence the question. > > There exist the sysfs_update_groups(), but the BAR resource sysfs objects > are currently, at least not yet, added to any attribute group. then maybe they should be added to one :) thanks, greg k-h
Hello, [...] > > There exist the sysfs_update_groups(), but the BAR resource sysfs objects > > are currently, at least not yet, added to any attribute group. > > then maybe they should be added to one :) Yeah. There is work in progress that will take care of some of this. Krzysztof
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c index 45b2e92941da1f49dcc71af3781317c61480c956..8b01a7eda5fb3239e138372417d01967c7a3f122 100644 --- a/fs/sysfs/group.c +++ b/fs/sysfs/group.c @@ -98,6 +98,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj, if (!mode) continue; } + if (grp->bin_size) + size = grp->bin_size(kobj, *bin_attr, i); WARN(mode & ~(SYSFS_PREALLOC | 0664), "Attribute %s: Invalid permissions 0%o\n", diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index c4e64dc112063f7cb89bf66059d0338716089e87..4746cccb95898b24df6f53de9421ea7649b5568f 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -87,6 +87,11 @@ do { \ * SYSFS_GROUP_VISIBLE() when assigning this callback to * specify separate _group_visible() and _attr_visible() * handlers. + * @bin_size: + * Optional: Function to return the size of a binary attribute + * of the group. Will be called repeatedly for each binary + * attribute in the group. Overwrites the size field embedded + * inside the attribute itself. * @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. @@ -97,6 +102,9 @@ struct attribute_group { struct attribute *, int); umode_t (*is_bin_visible)(struct kobject *, struct bin_attribute *, int); + size_t (*bin_size)(struct kobject *, + const struct bin_attribute *, + int); struct attribute **attrs; struct bin_attribute **bin_attrs; };
Several drivers need to dynamically calculate the size of an binary attribute. Currently this is done by assigning attr->size from the is_bin_visible() callback. This has drawbacks: * It is not documented. * A single attribute can be instantiated multiple times, overwriting the shared size field. * It prevents the structure to be moved to read-only memory. Introduce a new dedicated callback to calculate the size of the attribute. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- fs/sysfs/group.c | 2 ++ include/linux/sysfs.h | 8 ++++++++ 2 files changed, 10 insertions(+)