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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

[1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/tree/lib/sysfs.c?id=v3.7.0#n152