Re: [PATCH 1/4] PCI/sysfs: Add pci_dev_resource_attr_is_visible() helper

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Sep 13, 2021 at 02:47:56PM -0500, Bjorn Helgaas wrote:
> On Sat, Sep 11, 2021 at 12:13:33PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 10, 2021 at 07:21:01PM +0200, Krzysztof Wilczyński wrote:
> > > Hi Greg,
> > > 
> > > [...]
> > > > >   pci_dev_config_attr_is_visible(..., struct bin_attribute *a, ...)
> > > > >   {
> > > > >     a->size = PCI_CFG_SPACE_SIZE;    # <-- set size in global attr
> > > > >     ...
> > > > >   }
> > > > > 
> > > > >   static struct bin_attribute *pci_dev_config_attrs[] = {
> > > > >     &bin_attr_config, NULL,
> > > > >   };
> > > > >   static const struct attribute_group pci_dev_config_attr_group = {
> > > > >     .bin_attrs = pci_dev_config_attrs,
> > > > >     .is_bin_visible = pci_dev_config_attr_is_visible,
> > > > >   };
> > > > > 
> > > > >   pci_device_add
> > > > >     device_add
> > > > >       device_add_attrs
> > > > >         device_add_groups
> > > > >           sysfs_create_groups
> > > > >             internal_create_groups
> > > > >               internal_create_group
> > > > >                 create_files
> > > > >                   grp->is_bin_visible()
> > > > >                   sysfs_add_file_mode_ns
> > > > >                     size = battr->size      # <-- copy size from attr
> > > > >                     __kernfs_create_file(..., size, ...)
> > > > >                       kernfs_new_node
> > > > >                         __kernfs_new_node
> > > > > 
> > > > 
> > > > You can create a dynamic attribute and register that.  I think some
> > > > drivers/busses do that today to handle this type of thing.
> > > 
> > > Some static attributes users don't set size today or simply set it to 0, so
> > > then we report 0 bytes in userspace for each such attribute via the backing
> > > i-node.
> > > 
> > > Would you be open to the idea of adding a .size() callback so that static
> > > attributes users could set size using more proper channels, or do you think
> > > leaving it being set to 0 is fine?
> > 
> > I think leaving it at 0 is fine, are userspace tools really needing to
> > know the size ahead of time for sysfs files?  What would changing this
> > help out with?
> 
> We currently set the inode size for BARs (resource0, resource1, etc)
> to the BAR size.  I don't think lspci uses that inode size; it looks
> at the addresses in "resource" and computes the size from that [1].
> 
> But I doubt we can set the "resourceN" sizes to 0, since somebody else
> might be using that information.
> 
> I'm curious to know what other static attribute users set .size.
> Maybe they're all singleton cases, as opposed to the per-device cases
> we're interested in.

Most are singleton cases from what I have seen.  Or they just leave the
file size at 0.  There are not that many binary sysfs files around,
thankfully.

thanks,

greg k-h