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

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

On Thu, Aug 26, 2021 at 06:35:34PM -0500, Bjorn Helgaas wrote:
> [+cc Greg, sorry for the ill-formed sysfs question below]
> 
> On Wed, Aug 25, 2021 at 09:22:52PM +0000, Krzysztof Wilczyński wrote:
> > This helper aims to replace functions pci_create_resource_files() and
> > pci_create_attr() that are currently involved in the PCI resource sysfs
> > objects dynamic creation and set up once the.
> > 
> > After the conversion to use static sysfs objects when exposing the PCI
> > BAR address space this helper is to be called from the .is_bin_visible()
> > callback for each of the PCI resources attributes.
> > 
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index b70f61fbcd4b..c94ab9830932 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1237,6 +1237,46 @@ static int pci_create_resource_files(struct pci_dev *pdev)
> >  	}
> >  	return 0;
> >  }
> > +
> > +static umode_t pci_dev_resource_attr_is_visible(struct kobject *kobj,
> > +						struct bin_attribute *attr,
> > +						int bar, bool write_combine)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> > +	resource_size_t resource_size = pci_resource_len(pdev, bar);
> > +	unsigned long flags = pci_resource_flags(pdev, bar);
> > +
> > +	if (!resource_size)
> > +		return 0;
> > +
> > +	if (write_combine) {
> > +		if (arch_can_pci_mmap_wc() && (flags &
> > +		    (IORESOURCE_MEM | IORESOURCE_PREFETCH)) ==
> > +			(IORESOURCE_MEM | IORESOURCE_PREFETCH))
> > +			attr->mmap = pci_mmap_resource_wc;
> 
> Is it legal to update attr here in an .is_visible() method?  Is attr
> the single static struct bin_attribute here, or is it a per-device
> copy?

It is whatever was registered with sysfs, that was up to the caller.

> I'm assuming the static bin_attribute is a template and when we add a
> device that uses it, we alloc a new copy so each device has its own
> size, mapping function, etc.

Not that I recall, I think it's just a pointer to the structure that the
driver passed to the sysfs code.

> If that's the case, we only want to update the *copy*, not the
> template.  I don't see an alloc before the call in create_files(),
> so I'm worried that this .is_visible() method might get the template,
> in which case we'd be updating ->mmap for *all* devices.

Yes, I think that is what you are doing here.

Generally, don't mess with attribute values in the is_visable callback
if at all possible, as that's not what the callback is for.

thanks,

greg k-h