diff mbox series

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

Message ID 20210825212255.878043-2-kw@linux.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Convert dynamic PCI resources sysfs objects into static | expand

Commit Message

Krzysztof Wilczyński Aug. 25, 2021, 9:22 p.m. UTC
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(+)

Comments

Bjorn Helgaas Aug. 26, 2021, 11:35 p.m. UTC | #1
[+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?

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.

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.

> +		else
> +			return 0;
> +	} else {
> +		if (flags & IORESOURCE_MEM) {
> +			attr->mmap = pci_mmap_resource_uc;
> +		} else if (flags & IORESOURCE_IO) {
> +			attr->read = pci_read_resource_io;
> +			attr->write = pci_write_resource_io;
> +			if (arch_can_pci_mmap_io())
> +				attr->mmap = pci_mmap_resource_uc;
> +		} else {
> +			return 0;
> +		}
> +	}
> +
> +	attr->size = resource_size;
> +	if (attr->mmap)
> +		attr->f_mapping = iomem_get_mapping;
> +
> +	attr->private = (void *)(unsigned long)bar;
> +
> +	return attr->attr.mode;
> +}
>  #else /* !(defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)) */
>  int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
>  void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
> -- 
> 2.32.0
>
Greg KH Aug. 27, 2021, 12:11 p.m. UTC | #2
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
Bjorn Helgaas Aug. 27, 2021, 10:23 p.m. UTC | #3
On Fri, Aug 27, 2021 at 02:11:05PM +0200, Greg Kroah-Hartman wrote:
> 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_visible callback
> if at all possible, as that's not what the callback is for.

Unfortunately I can't find any documentation about what the
.is_visible() callback is for and what the restrictions on it are.

I *did* figure out that bin_attribute.size is updated by some
.is_bin_visible() callbacks, e.g., pci_dev_config_attr_is_visible()
and pci_dev_rom_attr_is_visible().  These are static attributes, so
there's a single copy per system, but that size gets copied to the
inode eventually, so it ends up being per-sysfs file.

This is all done inside device_add(), which means there should be some
mutex so the .is_bin_visible() "size" updates to that single static
attribute inside concurrent device_add() calls don't stomp on each
other.

I could have missed it, but I don't see that mutex, which makes me
suspect we rely on the bus driver to serialize device_add() calls.

Maybe there's nothing to be done here, except that we need to do some
more work to figure out how to fix the "sysfs_initialized" ugliness in
pci_sysfs_init().

Here are the details of the single static attribute and the
device_add() path I mentioned above:

  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
Greg KH Sept. 10, 2021, 2:08 p.m. UTC | #4
On Fri, Aug 27, 2021 at 05:23:31PM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 27, 2021 at 02:11:05PM +0200, Greg Kroah-Hartman wrote:
> > 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_visible callback
> > if at all possible, as that's not what the callback is for.
> 
> Unfortunately I can't find any documentation about what the
> .is_visible() callback is for and what the restrictions on it are.
> 
> I *did* figure out that bin_attribute.size is updated by some
> .is_bin_visible() callbacks, e.g., pci_dev_config_attr_is_visible()
> and pci_dev_rom_attr_is_visible().  These are static attributes, so
> there's a single copy per system, but that size gets copied to the
> inode eventually, so it ends up being per-sysfs file.
> 
> This is all done inside device_add(), which means there should be some
> mutex so the .is_bin_visible() "size" updates to that single static
> attribute inside concurrent device_add() calls don't stomp on each
> other.
> 
> I could have missed it, but I don't see that mutex, which makes me
> suspect we rely on the bus driver to serialize device_add() calls.

Yes, all device_add() calls are relying on the bus mutex, that way we
only probe one driver at a time.

> Maybe there's nothing to be done here, except that we need to do some
> more work to figure out how to fix the "sysfs_initialized" ugliness in
> pci_sysfs_init().
> 
> Here are the details of the single static attribute and the
> device_add() path I mentioned above:
> 
>   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.

thanks,

greg k-h
Krzysztof Wilczyński Sept. 10, 2021, 4:12 p.m. UTC | #5
Hello Bjorn and Greg,

Thank you both for adding more details here!

[...]
> > > > +	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_visible callback
> > if at all possible, as that's not what the callback is for.
> 
> Unfortunately I can't find any documentation about what the
> .is_visible() callback is for and what the restrictions on it are.
> 
> I *did* figure out that bin_attribute.size is updated by some
> .is_bin_visible() callbacks, e.g., pci_dev_config_attr_is_visible()
> and pci_dev_rom_attr_is_visible().  These are static attributes, so
> there's a single copy per system, but that size gets copied to the
> inode eventually, so it ends up being per-sysfs file.
> 
> This is all done inside device_add(), which means there should be some
> mutex so the .is_bin_visible() "size" updates to that single static
> attribute inside concurrent device_add() calls don't stomp on each
> other.
> 
> I could have missed it, but I don't see that mutex, which makes me
> suspect we rely on the bus driver to serialize device_add() calls.
> 
> Maybe there's nothing to be done here, except that we need to do some
> more work to figure out how to fix the "sysfs_initialized" ugliness in
> pci_sysfs_init().
> 
> Here are the details of the single static attribute and the
> device_add() path I mentioned above:
> 
>   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

To add more to what Bjorn is talking about here, primarily for posterity as
perhaps someone else might stumble into the same thing we did, a few log
lines illustrating the attribute reuse:

   1 pci 0000:00:00.0: [8086:29c0] type 00 class 0x060000
   2 pci 0000:00:01.0: [8086:10d3] type 00 class 0x020000
   3 pci 0000:00:01.0: reg 0x10: [mem 0xfeb80000-0xfeb9ffff]
   4 pci 0000:00:01.0: reg 0x14: [mem 0xfeba0000-0xfebbffff]
   5 pci 0000:00:01.0: reg 0x18: [io  0xc040-0xc05f]
   6 pci 0000:00:01.0: reg 0x1c: [mem 0xfebc0000-0xfebc3fff]
   7 pci 0000:00:01.0: reg 0x30: [mem 0xfeb00000-0xfeb7ffff pref]
   8 pdev @ ffff8880032fd800, bar 0 131072 @ ffff8880032fdb98 [mem 0xfeb80000-0xfeb9ffff], kobject @ ffff8880032fd8c0, attr resource0 @ ffffffff825b2ee0
   9 pdev @ ffff8880032fd800, bar 1 131072 @ ffff8880032fdbd8 [mem 0xfeba0000-0xfebbffff], kobject @ ffff8880032fd8c0, attr resource1 @ ffffffff825b2e20
  10 pdev @ ffff8880032fd800, bar 2 32 @ ffff8880032fdc18 [io  0xc040-0xc05f], kobject @ ffff8880032fd8c0, attr resource2 @ ffffffff825b2d60
  11 pdev @ ffff8880032fd800, bar 3 16384 @ ffff8880032fdc58 [mem 0xfebc0000-0xfebc3fff], kobject @ ffff8880032fd8c0, attr resource3 @ ffffffff825b2ca0
  12 pci 0000:00:1f.0: [8086:2918] type 00 class 0x060100
  13 pci 0000:00:1f.0: quirk: [io  0x0600-0x067f] claimed by ICH6 ACPI/GPIO/TCO
  14 pci 0000:00:1f.2: [8086:2922] type 00 class 0x010601
  15 pci 0000:00:1f.2: reg 0x20: [io  0xc060-0xc07f]
  16 pci 0000:00:1f.2: reg 0x24: [mem 0xfebc4000-0xfebc4fff]
  17 pdev @ ffff8880032fe800, bar 4 32 @ ffff8880032fec98 [io  0xc060-0xc07f], kobject @ ffff8880032fe8c0, attr resource4 @ ffffffff825b2be0
  18 pdev @ ffff8880032fe800, bar 5 4096 @ ffff8880032fecd8 [mem 0xfebc4000-0xfebc4fff], kobject @ ffff8880032fe8c0, attr resource5 @ ffffffff825b2b20
  19 pci 0000:00:1f.3: [8086:2930] type 00 class 0x0c0500
  20 pci 0000:00:1f.3: reg 0x20: [io  0x0700-0x073f]
  21 pdev @ ffff8880032ff000, bar 4 64 @ ffff8880032ff498 [io  0x0700-0x073f], kobject @ ffff8880032ff0c0, attr resource4 @ ffffffff825b2be0

A close look at lines #17 and #21 tells us that .is_bin_visible() is being
called on the static bin_attribute (those are 00:1f.2 BAR 4 and 00:1f.3 BAR 4)
and it would get a pointer to the same bin_attribute.

	Krzysztof
Krzysztof Wilczyński Sept. 10, 2021, 5:21 p.m. UTC | #6
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?

	Krzysztof
Greg KH Sept. 11, 2021, 10:13 a.m. UTC | #7
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?

thanks,

greg k-h
Bjorn Helgaas Sept. 13, 2021, 7:47 p.m. UTC | #8
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
Greg KH Sept. 14, 2021, 5:06 a.m. UTC | #9
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
diff mbox series

Patch

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;
+		else
+			return 0;
+	} else {
+		if (flags & IORESOURCE_MEM) {
+			attr->mmap = pci_mmap_resource_uc;
+		} else if (flags & IORESOURCE_IO) {
+			attr->read = pci_read_resource_io;
+			attr->write = pci_write_resource_io;
+			if (arch_can_pci_mmap_io())
+				attr->mmap = pci_mmap_resource_uc;
+		} else {
+			return 0;
+		}
+	}
+
+	attr->size = resource_size;
+	if (attr->mmap)
+		attr->f_mapping = iomem_get_mapping;
+
+	attr->private = (void *)(unsigned long)bar;
+
+	return attr->attr.mode;
+}
 #else /* !(defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)) */
 int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
 void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }