Re: [PATCH 19/20] PCI/sysfs: Convert legacy I/O and memory attributes to static definitions

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

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

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

> Currently, legacy_io and legacy_mem are dynamically allocated and
> created by pci_create_legacy_files(), with pci_adjust_legacy_attr()
> updating the attributes at runtime on Alpha to rename them and shift
> the size for sparse addressing.
> 
> Convert to four static const attributes (legacy_io, legacy_io_sparse,
> legacy_mem, legacy_mem_sparse) with is_bin_visible() callbacks that
> use pci_legacy_has_sparse() to select the appropriate variant per bus.
> The sizes are compile-time constants and .size is set directly on
> each attribute.
> 
> Register the groups in pcibus_groups[] under a HAVE_PCI_LEGACY guard
> so the driver model handles creation and removal automatically.
> 
> Stub out pci_create_legacy_files() and pci_remove_legacy_files() as
> the dynamic creation is no longer needed.  Remove the __weak
> pci_adjust_legacy_attr(), Alpha's override, and its declaration from
> both Alpha and PowerPC asm/pci.h headers.
> 
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
> ---
>  arch/alpha/include/asm/pci.h   |   4 +-
>  arch/alpha/kernel/pci-sysfs.c  |  24 ----
>  arch/powerpc/include/asm/pci.h |   2 -
>  drivers/pci/pci-sysfs.c        | 200 ++++++++++++++++++++-------------
>  4 files changed, 122 insertions(+), 108 deletions(-)
> 
> diff --git a/arch/alpha/include/asm/pci.h b/arch/alpha/include/asm/pci.h
> index ef19295f2e33..ad5d1391e1fa 100644
> --- a/arch/alpha/include/asm/pci.h
> +++ b/arch/alpha/include/asm/pci.h
> @@ -84,8 +84,8 @@ extern int pci_legacy_write(struct pci_bus *bus, loff_t port, u32 val,
>  extern int pci_mmap_legacy_page_range(struct pci_bus *bus,
>  				      struct vm_area_struct *vma,
>  				      enum pci_mmap_state mmap_state);
> -extern void pci_adjust_legacy_attr(struct pci_bus *bus,
> -				   enum pci_mmap_state mmap_type);
> +extern bool pci_legacy_has_sparse(struct pci_bus *bus,
> +				  enum pci_mmap_state type);
>  #define HAVE_PCI_LEGACY	1
>  
>  extern const struct attribute_group pci_dev_resource_attr_group;
> diff --git a/arch/alpha/kernel/pci-sysfs.c b/arch/alpha/kernel/pci-sysfs.c
> index 2031a3b6972c..2db91169bf5a 100644
> --- a/arch/alpha/kernel/pci-sysfs.c
> +++ b/arch/alpha/kernel/pci-sysfs.c
> @@ -197,30 +197,6 @@ bool pci_legacy_has_sparse(struct pci_bus *bus, enum pci_mmap_state type)
>  	return has_sparse(hose, type);
>  }
>  
> -/**
> - * pci_adjust_legacy_attr - adjustment of legacy file attributes
> - * @bus: bus to create files under
> - * @mmap_type: I/O port or memory
> - *
> - * Adjust file name and size for sparse mappings.
> - */
> -void pci_adjust_legacy_attr(struct pci_bus *bus, enum pci_mmap_state mmap_type)
> -{
> -	struct pci_controller *hose = bus->sysdata;
> -
> -	if (!has_sparse(hose, mmap_type))
> -		return;
> -
> -	if (mmap_type == pci_mmap_mem) {
> -		bus->legacy_mem->attr.name = "legacy_mem_sparse";
> -		bus->legacy_mem->size <<= 5;
> -	} else {
> -		bus->legacy_io->attr.name = "legacy_io_sparse";
> -		bus->legacy_io->size <<= 5;
> -	}
> -	return;
> -}
> -
>  /* Legacy I/O bus read/write functions */
>  int pci_legacy_read(struct pci_bus *bus, loff_t port, u32 *val, size_t size)
>  {
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 46a9c4491ed0..72f286e74786 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -82,8 +82,6 @@ extern int pci_legacy_write(struct pci_bus *bus, loff_t port, u32 val,
>  extern int pci_mmap_legacy_page_range(struct pci_bus *bus,
>  				      struct vm_area_struct *vma,
>  				      enum pci_mmap_state mmap_state);
> -extern void pci_adjust_legacy_attr(struct pci_bus *bus,
> -				   enum pci_mmap_state mmap_type);
>  #define HAVE_PCI_LEGACY	1
>  
>  extern void pcibios_claim_one_bus(struct pci_bus *b);
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index fe079fb31dce..dc0bb0488317 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -702,11 +702,6 @@ static const struct attribute_group pcibus_group = {
>  	.attrs = pcibus_attrs,
>  };
>  
> -const struct attribute_group *pcibus_groups[] = {
> -	&pcibus_group,
> -	NULL,
> -};
> -
>  static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
> @@ -1025,91 +1020,136 @@ bool __weak pci_legacy_has_sparse(struct pci_bus *bus,
>  	return false;
>  }
>  
> -/**
> - * pci_adjust_legacy_attr - adjustment of legacy file attributes
> - * @b: bus to create files under
> - * @mmap_type: I/O port or memory
> - *
> - * Stub implementation. Can be overridden by arch if necessary.
> - */
> -void __weak pci_adjust_legacy_attr(struct pci_bus *b,
> -				   enum pci_mmap_state mmap_type)
> +static inline umode_t __pci_legacy_is_visible(struct kobject *kobj,
> +					      const struct bin_attribute *a,
> +					      enum pci_mmap_state type,
> +					      bool sparse)
>  {
> +	struct pci_bus *bus = to_pci_bus(kobj_to_dev(kobj));
> +
> +	if (pci_legacy_has_sparse(bus, type) != sparse)
> +		return 0;
> +
> +	return a->attr.mode;
>  }
>  
> -/**
> - * pci_create_legacy_files - create legacy I/O port and memory files
> - * @b: bus to create files under
> - *
> - * Some platforms allow access to legacy I/O port and ISA memory space on
> - * a per-bus basis.  This routine creates the files and ties them into
> - * their associated read, write and mmap files from pci-sysfs.c
> - *
> - * On error unwind, but don't propagate the error to the caller
> - * as it is ok to set up the PCI bus without these files.
> - */
> -void pci_create_legacy_files(struct pci_bus *b)
> +static umode_t pci_legacy_io_is_visible(struct kobject *kobj,
> +					const struct bin_attribute *a, int n)
>  {
> -	int error;
> -
> -	if (!sysfs_initialized)
> -		return;
> -
> -	b->legacy_io = kzalloc_objs(struct bin_attribute, 2, GFP_ATOMIC);
> -	if (!b->legacy_io)
> -		goto kzalloc_err;
> -
> -	sysfs_bin_attr_init(b->legacy_io);
> -	b->legacy_io->attr.name = "legacy_io";
> -	b->legacy_io->size = 0xffff;
> -	b->legacy_io->attr.mode = 0600;
> -	b->legacy_io->read = pci_read_legacy_io;
> -	b->legacy_io->write = pci_write_legacy_io;
> -	/* See pci_create_attr() for motivation */
> -	b->legacy_io->llseek = pci_llseek_resource;
> -	b->legacy_io->mmap = pci_mmap_legacy_io;
> -	b->legacy_io->f_mapping = iomem_get_mapping;
> -	pci_adjust_legacy_attr(b, pci_mmap_io);
> -	error = device_create_bin_file(&b->dev, b->legacy_io);
> -	if (error)
> -		goto legacy_io_err;
> -
> -	/* Allocated above after the legacy_io struct */
> -	b->legacy_mem = b->legacy_io + 1;
> -	sysfs_bin_attr_init(b->legacy_mem);
> -	b->legacy_mem->attr.name = "legacy_mem";
> -	b->legacy_mem->size = 1024*1024;
> -	b->legacy_mem->attr.mode = 0600;
> -	b->legacy_mem->mmap = pci_mmap_legacy_mem;
> -	/* See pci_create_attr() for motivation */
> -	b->legacy_mem->llseek = pci_llseek_resource;
> -	b->legacy_mem->f_mapping = iomem_get_mapping;
> -	pci_adjust_legacy_attr(b, pci_mmap_mem);
> -	error = device_create_bin_file(&b->dev, b->legacy_mem);
> -	if (error)
> -		goto legacy_mem_err;
> -
> -	return;
> -
> -legacy_mem_err:
> -	device_remove_bin_file(&b->dev, b->legacy_io);
> -legacy_io_err:
> -	kfree(b->legacy_io);
> -	b->legacy_io = NULL;
> -kzalloc_err:
> -	dev_warn(&b->dev, "could not create legacy I/O port and ISA memory resources in sysfs\n");
> +	return __pci_legacy_is_visible(kobj, a, pci_mmap_io, false);
>  }
>  
> -void pci_remove_legacy_files(struct pci_bus *b)
> +static umode_t pci_legacy_io_sparse_is_visible(struct kobject *kobj,
> +					       const struct bin_attribute *a,
> +					       int n)
>  {
> -	if (b->legacy_io) {
> -		device_remove_bin_file(&b->dev, b->legacy_io);
> -		device_remove_bin_file(&b->dev, b->legacy_mem);
> -		kfree(b->legacy_io); /* both are allocated here */
> -	}
> +	return __pci_legacy_is_visible(kobj, a, pci_mmap_io, true);
>  }
> +
> +static umode_t pci_legacy_mem_is_visible(struct kobject *kobj,
> +					 const struct bin_attribute *a, int n)
> +{
> +	return __pci_legacy_is_visible(kobj, a, pci_mmap_mem, false);
> +}
> +
> +static umode_t pci_legacy_mem_sparse_is_visible(struct kobject *kobj,
> +						const struct bin_attribute *a,
> +						int n)
> +{
> +	return __pci_legacy_is_visible(kobj, a, pci_mmap_mem, true);
> +}
> +
> +static const struct bin_attribute pci_legacy_io_attr = {
> +	.attr = { .name = "legacy_io", .mode = 0600 },
> +	.size = 0xffff,
> +	.read = pci_read_legacy_io,
> +	.write = pci_write_legacy_io,
> +	.mmap = pci_mmap_legacy_io,
> +	.llseek = pci_llseek_resource,
> +	.f_mapping = iomem_get_mapping,
> +};
> +
> +static const struct bin_attribute pci_legacy_io_sparse_attr = {
> +	.attr = { .name = "legacy_io_sparse", .mode = 0600 },
> +	.size = 0xffff << 5,
> +	.read = pci_read_legacy_io,
> +	.write = pci_write_legacy_io,
> +	.mmap = pci_mmap_legacy_io,
> +	.llseek = pci_llseek_resource,
> +	.f_mapping = iomem_get_mapping,
> +};
> +
> +static const struct bin_attribute pci_legacy_mem_attr = {
> +	.attr = { .name = "legacy_mem", .mode = 0600 },
> +	.size = 0x100000,
> +	.mmap = pci_mmap_legacy_mem,
> +	.llseek = pci_llseek_resource,
> +	.f_mapping = iomem_get_mapping,
> +};
> +
> +static const struct bin_attribute pci_legacy_mem_sparse_attr = {
> +	.attr = { .name = "legacy_mem_sparse", .mode = 0600 },
> +	.size = 0x100000 << 5,

I suggest naming these legacy literals with defines. At least the mem one 
could also use SZ_1M:

#define PCI_LEGACY_MEM_SIZE	SZ_1M

> +	.mmap = pci_mmap_legacy_mem,
> +	.llseek = pci_llseek_resource,
> +	.f_mapping = iomem_get_mapping,
> +};
> +
> +static const struct bin_attribute *const pci_legacy_io_attrs[] = {
> +	&pci_legacy_io_attr,
> +	NULL,
> +};
> +
> +static const struct bin_attribute *const pci_legacy_io_sparse_attrs[] = {
> +	&pci_legacy_io_sparse_attr,
> +	NULL,
> +};
> +
> +static const struct bin_attribute *const pci_legacy_mem_attrs[] = {
> +	&pci_legacy_mem_attr,
> +	NULL,
> +};
> +
> +static const struct bin_attribute *const pci_legacy_mem_sparse_attrs[] = {
> +	&pci_legacy_mem_sparse_attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pci_legacy_io_group = {
> +	.bin_attrs = pci_legacy_io_attrs,
> +	.is_bin_visible = pci_legacy_io_is_visible,
> +};
> +
> +static const struct attribute_group pci_legacy_io_sparse_group = {
> +	.bin_attrs = pci_legacy_io_sparse_attrs,
> +	.is_bin_visible = pci_legacy_io_sparse_is_visible,
> +};
> +
> +static const struct attribute_group pci_legacy_mem_group = {
> +	.bin_attrs = pci_legacy_mem_attrs,
> +	.is_bin_visible = pci_legacy_mem_is_visible,
> +};
> +
> +static const struct attribute_group pci_legacy_mem_sparse_group = {
> +	.bin_attrs = pci_legacy_mem_sparse_attrs,
> +	.is_bin_visible = pci_legacy_mem_sparse_is_visible,
> +};
> +
> +void pci_create_legacy_files(struct pci_bus *b) { }
> +void pci_remove_legacy_files(struct pci_bus *b) { }
>  #endif /* HAVE_PCI_LEGACY */
>  
> +const struct attribute_group *pcibus_groups[] = {
> +	&pcibus_group,
> +#ifdef HAVE_PCI_LEGACY
> +	&pci_legacy_io_group,
> +	&pci_legacy_io_sparse_group,
> +	&pci_legacy_mem_group,
> +	&pci_legacy_mem_sparse_group,
> +#endif
> +	NULL,
> +};
> +
>  #if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
>  /**
>   * pci_mmap_resource - map a PCI resource into user memory space
> 

-- 
 i.