On Sun, 20 Jul 2025 10:15:08 -0500 Mario Limonciello <superm1@xxxxxxxxxx> wrote: > From: Mario Limonciello <mario.limonciello@xxxxxxx> > > 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@xxxxxxxxxxxxxxxx> > Closes: https://lore.kernel.org/linux-next/20250718224118.5b3f22b0@xxxxxxxxxxxxxxxx/ > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> 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,
