| Message ID | 20210825212255.878043-2-kw@linux.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Convert dynamic PCI resources sysfs objects into static | expand |
[+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 >
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
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
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
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
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
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
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
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 --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; }
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(+)