Re: [PATCH 06/20] PCI/sysfs: Convert PCI resource files to static attributes

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 13013 bytes --]

On Fri, 10 Apr 2026, Krzysztof Wilczyński wrote:

> Currently, the PCI resource files (resourceN, resourceN_wc) are
> dynamically created by pci_create_sysfs_dev_files(), called from
> both pci_bus_add_device() and the pci_sysfs_init() late_initcall,
> with only a sysfs_initialized flag for synchronisation.  This has
> caused "duplicate filename" warnings and boot panics when both
> paths race on the same device.
> 
> This is especially likely on Devicetree-based platforms, where the
> PCI host controllers are platform drivers that probe via the driver
> model, which can happen during or after the late_initcall.  As such,
> pci_bus_add_device() and pci_sysfs_init() are more likely to overlap.
> 
> Thus, convert to static const attributes with three attribute groups
> (I/O, UC, WC), each with an .is_bin_visible callback that checks
> resource flags, BAR length, and non_mappable_bars.  A .bin_size
> callback provides pci_resource_len() to the kernfs node for correct
> stat and lseek behaviour.
> 
> As part of this conversion:
> 
>   - Rename pci_read_resource_io() and pci_write_resource_io() to
>     pci_read_resource() and pci_write_resource() since the callbacks
>     are no longer I/O-specific in the static attribute context.
> 
>   - Remove pci_create_resource_files(), pci_remove_resource_files(),
>     and pci_create_attr() which are no longer needed.
> 
>   - Move the __weak stubs outside the #if guard so they remain
>     available for callers converted in subsequent commits.
> 
> Platforms that do not define the HAVE_PCI_MMAP macro or the
> ARCH_GENERIC_PCI_MMAP_RESOURCE macro, such as Alpha architecture,
> continue using their platform-specific resource file creation.
> 
> For reference, the dynamic creation dates back to the pre-Git era:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/drivers/pci/pci-sysfs.c?id=42298be0eeb5ae98453b3374c36161b05a46c5dc
> 
> The write-combine support was added in commit 45aec1ae72fc ("x86: PAT
> export resource_wc in pci sysfs").
> 
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
> ---
>  drivers/pci/pci-sysfs.c | 242 +++++++++++++++++++++-------------------
>  include/linux/pci.h     |   2 -
>  2 files changed, 127 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d29d79be8ee5..e56fddbe7914 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1200,14 +1200,14 @@ static ssize_t pci_resource_io(struct file *filp, struct kobject *kobj,
>  #endif
>  }
>  
> -static ssize_t pci_read_resource_io(struct file *filp, struct kobject *kobj,
> +static ssize_t pci_read_resource(struct file *filp, struct kobject *kobj,
>  				    const struct bin_attribute *attr, char *buf,
>  				    loff_t off, size_t count)
>  {
>  	return pci_resource_io(filp, kobj, attr, buf, off, count, false);
>  }
>  
> -static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
> +static ssize_t pci_write_resource(struct file *filp, struct kobject *kobj,
>  				     const struct bin_attribute *attr, char *buf,
>  				     loff_t off, size_t count)
>  {
> @@ -1261,129 +1261,136 @@ static const struct bin_attribute dev_resource##_bar##_wc_attr = {		\
>  	.mmap = pci_mmap_resource_wc,						\
>  }
>  
> -/**
> - * 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)
> +static inline umode_t
> +__pci_resource_attr_is_visible(struct kobject *kobj,
> +			       const struct bin_attribute *a,
> +			       int bar, bool write_combine,
> +			       unsigned long flags)
>  {
> -	int i;
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
>  
> -	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) {
> -		sprintf(res_attr_name, "resource%d_wc", num);
> -		res_attr->mmap = pci_mmap_resource_wc;
> -	} else {
> -		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;
> -		/*
> -		 * generic_file_llseek() consults f_mapping->host to determine
> -		 * the file size. As iomem_inode knows nothing about the
> -		 * attribute, it's not going to work, so override it as well.
> -		 */
> -		res_attr->llseek = pci_llseek_resource;
> -	}
> -	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;
> -	}
> -
> -	if (write_combine)
> -		pdev->res_attr_wc[num] = res_attr;
> -	else
> -		pdev->res_attr[num] = res_attr;
> -
> -	return 0;
> -}
> -
> -/**
> - * 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;
> -
> -	/* Skip devices with non-mappable BARs */
>  	if (pdev->non_mappable_bars)
>  		return 0;
>  
> -	/* Expose the PCI resources from this device as files */
> -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +	if (!pci_resource_len(pdev, bar))
> +		return 0;

I know it's same as in the previous code but I dislike assuming len != 0 
implies resource has been assigned. While it currently holds, I'd want to 
change that eventually.

The current behavior causes issue e.g. if IOV resource fails to assign, it 
is reset (making its len 0 among other thing) and since IOV resource are 
optional that is fine from kernel's perspective. But resetting the 
resource means we also lose access to that resource because its type gets 
cleared so from kernel perspective the VF BAR stops to exist. Losing it 
means the user cannot solve the issue by e.g. resizing some other BAR 
smaller to make space to allow the VF BARs to assign successfully.

So I think this code would actually want to check resource_assigned() 
which implies also non-zero size.

AFAICT, this change looks fine (despite the diff being very messy).

> -		/* skip empty resources */
> -		if (!pci_resource_len(pdev, i))
> -			continue;
> +	if ((pci_resource_flags(pdev, bar) & flags) != flags)
> +		return 0;
>  
> -		retval = pci_create_attr(pdev, i, 0);
> -		/* for prefetchable resources, create a WC mappable file */
> -		if (!retval && arch_can_pci_mmap_wc() &&
> -		    pci_resource_flags(pdev, i) & IORESOURCE_PREFETCH)
> -			retval = pci_create_attr(pdev, i, 1);
> -		if (retval) {
> -			pci_remove_resource_files(pdev);
> -			return retval;
> -		}
> -	}
> -	return 0;
> +	if (write_combine && !arch_can_pci_mmap_wc())
> +		return 0;
> +
> +	return a->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; }
> +
> +static umode_t pci_dev_resource_io_is_visible(struct kobject *kobj,
> +					      const struct bin_attribute *a,
> +					      int n)
> +{
> +	return __pci_resource_attr_is_visible(kobj, a, n, false,
> +					      IORESOURCE_IO);
> +}
> +
> +static umode_t pci_dev_resource_uc_is_visible(struct kobject *kobj,
> +					      const struct bin_attribute *a,
> +					      int n)
> +{
> +	return __pci_resource_attr_is_visible(kobj, a, n, false,
> +					      IORESOURCE_MEM);
> +}
> +
> +static umode_t pci_dev_resource_wc_is_visible(struct kobject *kobj,
> +					      const struct bin_attribute *a,
> +					      int n)
> +{
> +	return __pci_resource_attr_is_visible(kobj, a, n, true,
> +					      IORESOURCE_MEM | IORESOURCE_PREFETCH);
> +}
> +
> +static size_t pci_dev_resource_bin_size(struct kobject *kobj,
> +					const struct bin_attribute *a,
> +					int n)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +
> +	return pci_resource_len(pdev, n);
> +}
> +
> +pci_dev_resource_io_attr(0);
> +pci_dev_resource_io_attr(1);
> +pci_dev_resource_io_attr(2);
> +pci_dev_resource_io_attr(3);
> +pci_dev_resource_io_attr(4);
> +pci_dev_resource_io_attr(5);
> +
> +pci_dev_resource_uc_attr(0);
> +pci_dev_resource_uc_attr(1);
> +pci_dev_resource_uc_attr(2);
> +pci_dev_resource_uc_attr(3);
> +pci_dev_resource_uc_attr(4);
> +pci_dev_resource_uc_attr(5);
> +
> +pci_dev_resource_wc_attr(0);
> +pci_dev_resource_wc_attr(1);
> +pci_dev_resource_wc_attr(2);
> +pci_dev_resource_wc_attr(3);
> +pci_dev_resource_wc_attr(4);
> +pci_dev_resource_wc_attr(5);
> +
> +static const struct bin_attribute *const pci_dev_resource_io_attrs[] = {
> +	&dev_resource0_io_attr,
> +	&dev_resource1_io_attr,
> +	&dev_resource2_io_attr,
> +	&dev_resource3_io_attr,
> +	&dev_resource4_io_attr,
> +	&dev_resource5_io_attr,
> +	NULL,
> +};
> +
> +static const struct bin_attribute *const pci_dev_resource_uc_attrs[] = {
> +	&dev_resource0_uc_attr,
> +	&dev_resource1_uc_attr,
> +	&dev_resource2_uc_attr,
> +	&dev_resource3_uc_attr,
> +	&dev_resource4_uc_attr,
> +	&dev_resource5_uc_attr,
> +	NULL,
> +};
> +
> +static const struct bin_attribute *const pci_dev_resource_wc_attrs[] = {
> +	&dev_resource0_wc_attr,
> +	&dev_resource1_wc_attr,
> +	&dev_resource2_wc_attr,
> +	&dev_resource3_wc_attr,
> +	&dev_resource4_wc_attr,
> +	&dev_resource5_wc_attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pci_dev_resource_io_attr_group = {
> +	.bin_attrs = pci_dev_resource_io_attrs,
> +	.is_bin_visible = pci_dev_resource_io_is_visible,
> +	.bin_size = pci_dev_resource_bin_size,
> +};
> +
> +static const struct attribute_group pci_dev_resource_uc_attr_group = {
> +	.bin_attrs = pci_dev_resource_uc_attrs,
> +	.is_bin_visible = pci_dev_resource_uc_is_visible,
> +	.bin_size = pci_dev_resource_bin_size,
> +};
> +
> +static const struct attribute_group pci_dev_resource_wc_attr_group = {
> +	.bin_attrs = pci_dev_resource_wc_attrs,
> +	.is_bin_visible = pci_dev_resource_wc_is_visible,
> +	.bin_size = pci_dev_resource_bin_size,
> +};
> +
>  #endif
>  
> +int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
> +void __weak pci_remove_resource_files(struct pci_dev *dev) { }
> +
>  /**
>   * pci_write_rom - used to enable access to the PCI ROM display
>   * @filp: sysfs file
> @@ -1861,6 +1868,11 @@ 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_io_attr_group,
> +	&pci_dev_resource_uc_attr_group,
> +	&pci_dev_resource_wc_attr_group,
> +#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 1c270f1d5123..a7a104427b07 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2501,10 +2501,8 @@ int pcibios_alloc_irq(struct pci_dev *dev);
>  void pcibios_free_irq(struct pci_dev *dev);
>  resource_size_t pcibios_default_alignment(void);
>  
> -#if !defined(HAVE_PCI_MMAP) && !defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
>  extern int pci_create_resource_files(struct pci_dev *dev);
>  extern void pci_remove_resource_files(struct pci_dev *dev);
> -#endif
>  
>  #if defined(CONFIG_PCI_MMCONFIG) || defined(CONFIG_ACPI_MCFG)
>  void __init pci_mmcfg_early_init(void);
> 

-- 
 i.