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

["Krzysztof Wilczyński" <kw@linux.com>]

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