[PATCH 0/4] PCI: Convert dynamic PCI resources sysfs objects into static

[PATCH 0/4] PCI: Convert dynamic PCI resources sysfs objects into static

[Krzysztof Wilczyński]

Hello,

Currently, a lot of PCI-related sysfs objects are dynamically created
under the "/sys/bus/pci/devices/..." path when either the PCI driver is
initialised or when a new device is hot-added.

When PCI driver loads and the PCI sub-system initialises, the sysfs
attributes are then added when late_initcall() function is called:

    pci_sysfs_init
      sysfs_initialized = 1
      for_each_pci_dev
        pci_create_sysfs_dev_files
          sysfs_create_bin_file

Otherwise, for hot-added devices, the sysfs attributes are then added
when the pci_bus_add_devices() function is called:

  pci_bus_add_devices
    pci_bus_add_device
      pci_create_sysfs_dev_files
        if (!sysfs_initialized)
	  return
        sysfs_create_bin_file

When a device is removed the pci_remove_sysfs_dev_files() function is
called from pci_stop_dev() to remove all the attributes that were
previously added:

  pci_stop_bus_device
    pci_stop_dev
      if (pci_dev_is_added)
        pci_remove_sysfs_dev_files
          sysfs_remove_bin_file

The current implementation is known to cause problems:

  https://lore.kernel.org/linux-pci/1366196798-15929-1-git-send-email-artem.savkov@gmail.com/
  https://lore.kernel.org/linux-pci/20200716110423.xtfyb3n6tn5ixedh@pali/
  https://lore.kernel.org/linux-pci/m3eebg9puj.fsf@t19.piap.pl/
  https://lore.kernel.org/linux-pci/20210507102706.7658-1-danijel.slivka@amd.com/

Most of the PCI-related attributes do not need to be created and removed
dynamically and there is no need to also manage their create and remove
life cycle manually.

The aim is also to first reduce the reliance on using late_initcall() to
eventually remove the need for it completely.

This particular series focusing on PCI resources files on platforms that
provide either the HAVE_PCI_MMAP or ARCH_GENERIC_PCI_MMAP_RESOURCE
definitions (other platforms, such as the Alpha platform, will not be
affected) continues the on-going effort to convert the majority of the
dynamic sysfs objects into static ones so that the PCI driver core can
add and remove sysfs objects and related attributes automatically.

Please note that this series depends on the commits from the following
series to be added prior to applying it:

  https://lore.kernel.org/linux-pci/20210729233235.1508920-1-kw@linux.com/
  https://lore.kernel.org/linux-pci/20210812132144.791268-1-kw@linux.com/

	Krzysztof

Krzysztof Wilczyński (4):
  PCI/sysfs: Add pci_dev_resource_attr_is_visible() helper
  PCI/sysfs: Add pci_dev_resource_attr() macro
  PCI/sysfs: Add pci_dev_resource_group() macro
  PCI/sysfs: Convert PCI resource files to static attributes

 drivers/pci/pci-sysfs.c | 189 ++++++++++++++++++++++------------------
 include/linux/pci.h     |   2 +
 2 files changed, 104 insertions(+), 87 deletions(-)

-- 
2.32.0

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

[Krzysztof Wilczyński]

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;
+		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

[PATCH 2/4] PCI/sysfs: Add pci_dev_resource_attr() macro

[Krzysztof Wilczyński]

The pci_dev_resource_attr() macro will be used to declare and define
each of the PCI resource sysfs objects statically while also reducing
unnecessary code repetition.

Internally this macro relies on the pci_dev_resource_attr_is_visible()
helper which should correctly handle different types of PCI BAR address
space while also providing support for creating either normal and/or
write-combine attributes as required.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c94ab9830932..6eba5c0887df 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1277,6 +1277,53 @@ static umode_t pci_dev_resource_attr_is_visible(struct kobject *kobj,
 
 	return attr->attr.mode;
 }
+
+#define pci_dev_resource_attr(_bar)					\
+static struct bin_attribute						\
+pci_dev_resource##_bar##_attr = __BIN_ATTR(resource##_bar,		\
+					   0600, NULL, NULL, 0);	\
+									\
+static struct bin_attribute *pci_dev_resource##_bar##_attrs[] = {	\
+	&pci_dev_resource##_bar##_attr,					\
+	NULL,								\
+};									\
+									\
+static umode_t								\
+pci_dev_resource##_bar##_attr_is_visible(struct kobject *kobj,		\
+					 struct bin_attribute *a,	\
+					 int n)				\
+{									\
+	return pci_dev_resource_attr_is_visible(kobj, a, _bar, false);	\
+};									\
+									\
+static const struct							\
+attribute_group pci_dev_resource##_bar##_attr_group = {			\
+	.bin_attrs = pci_dev_resource##_bar##_attrs,			\
+	.is_bin_visible = pci_dev_resource##_bar##_attr_is_visible,	\
+};									\
+									\
+static struct bin_attribute						\
+pci_dev_resource##_bar##_wc_attr = __BIN_ATTR(resource##_bar##_wc,	\
+					      0600, NULL, NULL, 0);	\
+									\
+static struct bin_attribute *pci_dev_resource##_bar##_wc_attrs[] = {	\
+	&pci_dev_resource##_bar##_wc_attr,				\
+	NULL,								\
+};									\
+									\
+static umode_t								\
+pci_dev_resource##_bar##_wc_attr_is_visible(struct kobject *kobj,	\
+					    struct bin_attribute *a,	\
+					    int n)			\
+{									\
+	return pci_dev_resource_attr_is_visible(kobj, a, _bar, true);	\
+};									\
+									\
+static const struct							\
+attribute_group pci_dev_resource##_bar##_wc_attr_group = {		\
+	.bin_attrs = pci_dev_resource##_bar##_wc_attrs,			\
+	.is_bin_visible = pci_dev_resource##_bar##_wc_attr_is_visible,	\
+}
 #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

[PATCH 3/4] PCI/sysfs: Add pci_dev_resource_group() macro

[Krzysztof Wilczyński]

The pci_dev_resource_group() macro will be used to reduce unnecessary
code repetition following the use of the pci_dev_resource_attr() macro
when adding each of the many newly created resource groups to the list
of other PCI sysfs objects stored in the pci_dev_groups array.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-sysfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6eba5c0887df..97ab9da47dca 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1324,6 +1324,11 @@ attribute_group pci_dev_resource##_bar##_wc_attr_group = {		\
 	.bin_attrs = pci_dev_resource##_bar##_wc_attrs,			\
 	.is_bin_visible = pci_dev_resource##_bar##_wc_attr_is_visible,	\
 }
+
+#define pci_dev_resource_group(_bar)		\
+	&pci_dev_resource##_bar##_attr_group,	\
+	&pci_dev_resource##_bar##_wc_attr_group
+
 #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

[PATCH 4/4] PCI/sysfs: Convert PCI resource files to static attributes

[Krzysztof Wilczyński]

The PCI resource files are a series of sysfs objects that allow for
access to the PCI BAR address space.

Previously, the PCI resource files for platforms that provide either
the HAVE_PCI_MMAP or ARCH_GENERIC_PCI_MMAP_RESOURCE definitions were
dynamically created by pci_bus_add_device() or the pci_sysfs_init()
late_initcall, but since these objects do not need to be created or
removed dynamically, we can use static attributes so the device model
takes care of addition and removal automatically.

Thus, convert the PCI resources files to static attributes on supported
platforms using the pci_dev_resource_attr() macro.  This macro makes use
of the .is_bin_visible() callback to correctly handle different types of
PCI BAR address spaces while also providing support for creating either
normal or write-combine attributes as required.

Platforms that do currently do not define either the HAVE_PCI_MMAP or
ARCH_GENERIC_PCI_MMAP_RESOURCE such as the Alpha platform can keep using
their platform-specific methods for adding PCI resources files.

Also remove pci_create_resource_files(), pci_remove_resource_files() and
pci_create_attr() functions, which are no longer needed.

The PCI resource files were first added in the pre-Git era:
  https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/pci-sysfs.c?id=42298be0eeb5ae98453b3374c36161b05a46c5dc

Support for write-combine resources was added in commit 45aec1ae72fc
("x86: PAT export resource_wc in pci sysfs").

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-sysfs.c | 129 ++++++++--------------------------------
 include/linux/pci.h     |   2 +
 2 files changed, 28 insertions(+), 103 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 97ab9da47dca..9f0062bdb9ab 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1135,109 +1135,6 @@ static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
 	return pci_resource_io(filp, kobj, attr, buf, off, count, true);
 }
 
-/**
- * pci_remove_resource_files - cleanup resource files
- * @pdev: dev to cleanup
- *
- * If we created resource files for @pdev, remove them from sysfs and
- * free their resources.
- */
-static void pci_remove_resource_files(struct pci_dev *pdev)
-{
-	int i;
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		struct bin_attribute *res_attr;
-
-		res_attr = pdev->res_attr[i];
-		if (res_attr) {
-			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
-			kfree(res_attr);
-		}
-
-		res_attr = pdev->res_attr_wc[i];
-		if (res_attr) {
-			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
-			kfree(res_attr);
-		}
-	}
-}
-
-static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
-{
-	/* allocate attribute structure, piggyback attribute name */
-	int name_len = write_combine ? 13 : 10;
-	struct bin_attribute *res_attr;
-	char *res_attr_name;
-	int retval;
-
-	res_attr = kzalloc(sizeof(*res_attr) + name_len, GFP_ATOMIC);
-	if (!res_attr)
-		return -ENOMEM;
-
-	res_attr_name = (char *)(res_attr + 1);
-
-	sysfs_bin_attr_init(res_attr);
-	if (write_combine) {
-		pdev->res_attr_wc[num] = res_attr;
-		sprintf(res_attr_name, "resource%d_wc", num);
-		res_attr->mmap = pci_mmap_resource_wc;
-	} else {
-		pdev->res_attr[num] = res_attr;
-		sprintf(res_attr_name, "resource%d", num);
-		if (pci_resource_flags(pdev, num) & IORESOURCE_IO) {
-			res_attr->read = pci_read_resource_io;
-			res_attr->write = pci_write_resource_io;
-			if (arch_can_pci_mmap_io())
-				res_attr->mmap = pci_mmap_resource_uc;
-		} else {
-			res_attr->mmap = pci_mmap_resource_uc;
-		}
-	}
-	if (res_attr->mmap)
-		res_attr->f_mapping = iomem_get_mapping;
-	res_attr->attr.name = res_attr_name;
-	res_attr->attr.mode = 0600;
-	res_attr->size = pci_resource_len(pdev, num);
-	res_attr->private = (void *)(unsigned long)num;
-	retval = sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
-	if (retval)
-		kfree(res_attr);
-
-	return retval;
-}
-
-/**
- * pci_create_resource_files - create resource files in sysfs for @dev
- * @pdev: dev in question
- *
- * Walk the resources in @pdev creating files for each resource available.
- */
-static int pci_create_resource_files(struct pci_dev *pdev)
-{
-	int i;
-	int retval;
-
-	/* Expose the PCI resources from this device as files */
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-
-		/* skip empty resources */
-		if (!pci_resource_len(pdev, i))
-			continue;
-
-		retval = pci_create_attr(pdev, i, 0);
-		/* for prefetchable resources, create a WC mappable file */
-		if (!retval && arch_can_pci_mmap_wc() &&
-		    pdev->resource[i].flags & IORESOURCE_PREFETCH)
-			retval = pci_create_attr(pdev, i, 1);
-		if (retval) {
-			pci_remove_resource_files(pdev);
-			return retval;
-		}
-	}
-	return 0;
-}
-
 static umode_t pci_dev_resource_attr_is_visible(struct kobject *kobj,
 						struct bin_attribute *attr,
 						int bar, bool write_combine)
@@ -1329,6 +1226,13 @@ attribute_group pci_dev_resource##_bar##_wc_attr_group = {		\
 	&pci_dev_resource##_bar##_attr_group,	\
 	&pci_dev_resource##_bar##_wc_attr_group
 
+pci_dev_resource_attr(0);
+pci_dev_resource_attr(1);
+pci_dev_resource_attr(2);
+pci_dev_resource_attr(3);
+pci_dev_resource_attr(4);
+pci_dev_resource_attr(5);
+
 #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; }
@@ -1470,6 +1374,10 @@ static const struct attribute_group pci_dev_reset_attr_group = {
 	.is_visible = pci_dev_reset_attr_is_visible,
 };
 
+#if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
+int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev) { return 0; }
+void pci_remove_sysfs_dev_files(struct pci_dev *pdev) { }
+#else /* !(defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)) */
 int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 {
 	if (!sysfs_initialized)
@@ -1491,9 +1399,15 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
 
 	pci_remove_resource_files(pdev);
 }
+#endif
 
 static int __init pci_sysfs_init(void)
 {
+#if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
+	struct pci_bus *pbus = NULL;
+
+	sysfs_initialized = 1;
+#else /* !(defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)) */
 	struct pci_dev *pdev = NULL;
 	struct pci_bus *pbus = NULL;
 	int retval;
@@ -1507,6 +1421,7 @@ static int __init pci_sysfs_init(void)
 		}
 	}
 
+#endif
 	while ((pbus = pci_find_next_bus(pbus)))
 		pci_create_legacy_files(pbus);
 
@@ -1580,6 +1495,14 @@ static const struct attribute_group pci_dev_group = {
 
 const struct attribute_group *pci_dev_groups[] = {
 	&pci_dev_group,
+#if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
+	pci_dev_resource_group(0),
+	pci_dev_resource_group(1),
+	pci_dev_resource_group(2),
+	pci_dev_resource_group(3),
+	pci_dev_resource_group(4),
+	pci_dev_resource_group(5),
+#endif
 	&pci_dev_config_attr_group,
 	&pci_dev_rom_attr_group,
 	&pci_dev_reset_attr_group,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 540b377ca8f6..3f460ea73f8f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -459,8 +459,10 @@ struct pci_dev {
 	u32		saved_config_space[16]; /* Config space saved at suspend time */
 	struct hlist_head saved_cap_space;
 	int		rom_attr_enabled;	/* Display of ROM attribute enabled? */
+ #if !(defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE))
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
+ #endif
 
 #ifdef CONFIG_HOTPLUG_PCI_PCIE
 	unsigned int	broken_cmd_compl:1;	/* No compl for some cmds */
-- 
2.32.0

Re: [PATCH 0/4] PCI: Convert dynamic PCI resources sysfs objects into static

[Krzysztof Wilczyński]

[+cc Krzysztof Hałasa, Sasha and Pali for visibility]

Hello Bjorn,

I wanted to add that Krzysztof Hałasa was able to find a bit of spare
time to test this series on his i.MX6 (Gateworks Ventana SBC), and
completed a number of tests (such as power-cycling the SoC, etc.)
without running into previous Oops and panic during boot.

The follow-up to this series will be changes specific to the Alpha
platform and legacy PCI attributes.

But since a majority of hardware platforms aren't really using legacy
PCI attributes these days (unless I am mistaken) and Alpha is not widely
used, fixing race condition in the PCI resource files converting them to
static sysfs objects would be a priority.

This will also make it easier to back-port this and other patches that
paved the way prior to this one the stable and LTS releases, especially
since these are widely used in popular distributions that tend to
back-port a lot too into their stable releases.

	Krzysztof

Re: [PATCH 2/4] PCI/sysfs: Add pci_dev_resource_attr() macro

[Bjorn Helgaas]

On Wed, Aug 25, 2021 at 09:22:53PM +0000, Krzysztof Wilczyński wrote:
> The pci_dev_resource_attr() macro will be used to declare and define
> each of the PCI resource sysfs objects statically while also reducing
> unnecessary code repetition.
> 
> Internally this macro relies on the pci_dev_resource_attr_is_visible()
> helper which should correctly handle different types of PCI BAR address
> space while also providing support for creating either normal and/or
> write-combine attributes as required.

This makes a lot of sense.  To make it more approachable, I think we
might extend the commit log to mention that each BAR is independently
visible/hidden based on its existence, and each .is_visible() function
applies to the entire attribute group, which means we need a separate
attribute group for each BAR.  Actually I guess we need *two* for each
BAR -- the normal one and the WC one.  So 12 attribute groups per
device.

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/pci/pci-sysfs.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c94ab9830932..6eba5c0887df 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1277,6 +1277,53 @@ static umode_t pci_dev_resource_attr_is_visible(struct kobject *kobj,
>  
>  	return attr->attr.mode;
>  }
> +
> +#define pci_dev_resource_attr(_bar)					\
> +static struct bin_attribute						\
> +pci_dev_resource##_bar##_attr = __BIN_ATTR(resource##_bar,		\
> +					   0600, NULL, NULL, 0);	\
> +									\
> +static struct bin_attribute *pci_dev_resource##_bar##_attrs[] = {	\
> +	&pci_dev_resource##_bar##_attr,					\
> +	NULL,								\
> +};									\
> +									\
> +static umode_t								\
> +pci_dev_resource##_bar##_attr_is_visible(struct kobject *kobj,		\
> +					 struct bin_attribute *a,	\
> +					 int n)				\
> +{									\
> +	return pci_dev_resource_attr_is_visible(kobj, a, _bar, false);	\
> +};									\
> +									\
> +static const struct							\
> +attribute_group pci_dev_resource##_bar##_attr_group = {			\
> +	.bin_attrs = pci_dev_resource##_bar##_attrs,			\
> +	.is_bin_visible = pci_dev_resource##_bar##_attr_is_visible,	\
> +};									\
> +									\
> +static struct bin_attribute						\
> +pci_dev_resource##_bar##_wc_attr = __BIN_ATTR(resource##_bar##_wc,	\
> +					      0600, NULL, NULL, 0);	\
> +									\
> +static struct bin_attribute *pci_dev_resource##_bar##_wc_attrs[] = {	\
> +	&pci_dev_resource##_bar##_wc_attr,				\
> +	NULL,								\
> +};									\
> +									\
> +static umode_t								\
> +pci_dev_resource##_bar##_wc_attr_is_visible(struct kobject *kobj,	\
> +					    struct bin_attribute *a,	\
> +					    int n)			\
> +{									\
> +	return pci_dev_resource_attr_is_visible(kobj, a, _bar, true);	\
> +};									\
> +									\
> +static const struct							\
> +attribute_group pci_dev_resource##_bar##_wc_attr_group = {		\
> +	.bin_attrs = pci_dev_resource##_bar##_wc_attrs,			\
> +	.is_bin_visible = pci_dev_resource##_bar##_wc_attr_is_visible,	\
> +}
>  #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
> 

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

[Bjorn Helgaas]

[+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
> 

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

[Greg Kroah-Hartman]

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

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

[Bjorn Helgaas]

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

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

[Greg Kroah-Hartman]

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

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

[Krzysztof Wilczyński]

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

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

[Krzysztof Wilczyński]

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

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

[Greg Kroah-Hartman]

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

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

[Bjorn Helgaas]

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

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

[Greg Kroah-Hartman]

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