Re: [PATCH v2] PCI: Adjust visibility of boot_display attribute instead of creation

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Sun, 20 Jul 2025 10:15:08 -0500
Mario Limonciello <superm1@kernel.org> wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> There is a desire to avoid creating new sysfs files late, so instead
> of dynamically deciding to create the boot_display attribute, make
> it static and use sysfs_update_group() to adjust visibility on the
> applicable devices.
> 
> This also fixes a compilation warning when compiled without
> CONFIG_VIDEO on sparc.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/linux-next/20250718224118.5b3f22b0@canb.auug.org.au/
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Leaving aside the 'where to call it' discussions, a few bits of feedback on the
code. See inline.


> ---
> v2:
>  * Change to sysfs_update_group() instead
> ---
>  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6b1a0ae254d3a..462a90d13eb87 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1059,37 +1059,6 @@ void pci_remove_legacy_files(struct pci_bus *b)
>  }
>  #endif /* HAVE_PCI_LEGACY */
>  
> -/**
> - * pci_create_boot_display_file - create a file in sysfs for @dev
> - * @pdev: dev in question
> - *
> - * Creates a file `boot_display` in sysfs for the PCI device @pdev
> - * if it is the boot display device.
> - */
> -static int pci_create_boot_display_file(struct pci_dev *pdev)
> -{
> -#ifdef CONFIG_VIDEO
> -	if (video_is_primary_device(&pdev->dev))
> -		return sysfs_create_file(&pdev->dev.kobj, &dev_attr_boot_display.attr);
> -#endif
> -	return 0;
> -}
> -
> -/**
> - * pci_remove_boot_display_file - remove the boot display file for @dev
> - * @pdev: dev in question
> - *
> - * Removes the file `boot_display` in sysfs for the PCI device @pdev
> - * if it is the boot display device.
> - */
> -static void pci_remove_boot_display_file(struct pci_dev *pdev)
> -{
> -#ifdef CONFIG_VIDEO
> -	if (video_is_primary_device(&pdev->dev))
> -		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_boot_display.attr);
> -#endif
> -}
> -
>  #if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
>  /**
>   * pci_mmap_resource - map a PCI resource into user memory space
> @@ -1691,6 +1660,29 @@ static const struct attribute_group pci_dev_resource_resize_group = {
>  	.is_visible = resource_resize_is_visible,
>  };
>  
> +static struct attribute *pci_display_attrs[] = {
> +	&dev_attr_boot_display.attr,
> +	NULL,

Trivial but I'd drop the comma after the NULL. It's at terminating entry and
we don't want it to be easy to add stuff after it!

> +};
> +
> +static umode_t pci_boot_display_visible(struct kobject *kobj,
> +					struct attribute *a, int n)
> +{
> +#ifdef CONFIG_VIDEO
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (a == &dev_attr_boot_display.attr && video_is_primary_device(&pdev->dev))
Is video_is_primary_device() stubbed out on !CONFIG_VIDEO?

There seems to be a stub in include/asm-generic/video.h

If it always is, drop the ifdef guards whilst you are here as the compiler
can see that and remove the code anyway.


> +		return a->mode;
> +#endif
> +	return 0;
> +}
> +
> +static const struct attribute_group pci_display_attr_group = {
> +	.attrs = pci_display_attrs,
> +	.is_visible = pci_boot_display_visible,
> +};
> +
>  int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>  {
>  	int retval;
> @@ -1698,7 +1690,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>  	if (!sysfs_initialized)
>  		return -EACCES;
>  
> -	retval = pci_create_boot_display_file(pdev);
> +	retval = sysfs_update_group(&pdev->dev.kobj, &pci_display_attr_group);
>  	if (retval)
>  		return retval;
>  
> @@ -1716,7 +1708,6 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
>  	if (!sysfs_initialized)
>  		return;
>  
> -	pci_remove_boot_display_file(pdev);
>  	pci_remove_resource_files(pdev);
>  }
>  
> @@ -1755,7 +1746,6 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>  
>  	if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
>  		return a->mode;
> -
Unrelated change.

>  	return 0;
>  }
>  
> @@ -1845,6 +1835,7 @@ static const struct attribute_group pcie_dev_attr_group = {
>  
>  const struct attribute_group *pci_dev_attr_groups[] = {
>  	&pci_dev_attr_group,
> +	&pci_display_attr_group,
>  	&pci_dev_hp_attr_group,
>  #ifdef CONFIG_PCI_IOV
>  	&sriov_pf_dev_attr_group,