Re: [PATCH] sysfs: Allow bin_attributes to be added to groups

[Dan Williams <dan.j.williams@intel.com>]

Lukas Wunner wrote:
> On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > > introduced dynamic addition of sysfs attributes to groups.
> > > 
> > > Allow the same for bin_attributes, in support of a forthcoming commit
> > > which adds various bin_attributes every time a PCI device is
> > > authenticated.
> > > 
> > > Addition of bin_attributes to groups differs from regular attributes in
> > > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > > vis-à-vis sysfs_add_file_mode_ns().
> > > 
> > > So call either of those two functions from sysfs_add_file_to_group()
> > > based on an additional boolean parameter and add two wrapper functions,
> > > one for bin_attributes and another for regular attributes.
> > > 
> > > Removal of bin_attributes from groups does not require a differentiation
> > > for bin_attributes and can use the same code path as regular attributes.
> > > 
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > ---
> > > Submitting this ahead of my PCI device authentication v2 patches.
> > > Not sure if the patch is acceptable without an accompanying user,
> > > but even if it's not, perhaps someone has early review feedback
> > > or wants to provide an Acked-by?  Thank you!
> > 
> > On the one hand it makes sense from a symmetry perspective, on the other
> > hand the expectation and the infrastructure for dynamic sysfs visibilty
> > has increased since 2007.
> > 
> > That is why I would like to see the use case to understand why a
> > dynamically added a bin_attribute is needed compared with a statically
> > defined attribute with dynamic visibility.
> 
> I assume "would like to see" means "on the mailing list", which would
> (or will) be as part of the PCI device authentication v2 patches.

That works, yeah.

[..]
> Each signature needs a unique filename and the number of signatures
> that can be generated is essentially unlimited.  (Though older ones
> are likely uninteresting and can be culled.)  If you want to expose,
> say, up to 1000 signatures per device, you'd have to allocate an array
> of 1000 signatures (for each device!), even though the actual number
> of signatures received might be much lower.  It would be a waste of
> memory.  It is much more economical to add signature attributes
> dynamically on demand.

Ah, yeah. If it was a double-digit number of files then a static array
would be ok, but once it gets to 1000 potential files, then dynamic
makes complete sense.

> It has the additional benefit that it allows user space to dynamically
> adjust the maximum number of signatures retained in the log.
> That's more difficult to implement with static attributes, as you'd
> have to reallocate the attributes array and adjust all the pointers
> pointing to it.

To be clear, and for example, dynamic adjusting an array of attributes
based on userspace input is what cxl_region_target_group does, but again
with 1000 potential files the static array would be silly.

Thanks for the details!