Hi Mani,
>Separate question: Do we really need to call pci_create_sysfs_dev_files() from
>pci_sysfs_init()? It is already getting called from pci_bus_add_device() and it
>looks redundant to me. More importantly, it can also lead to a race (though
>won't happen practically) [1].
>Same goes for pci_proc_attach_device(). I was tempted to submit a patch removing
>both these calls from pci_sysfs_init() and pci_proc_init(), but wanted to check
>first.
Thanks for pointing this out.
Yes, please do this! I was also considering deleting those unprotected calls,
pci_proc_attach_device and pci_create_sysfs_dev_files respectively.
Just wondering what needs to be checked before removing them.
Plus, now I am testing the following changes on my developing environment and
it works well so far.
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1722,18 +1722,9 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
static int __init pci_sysfs_init(void)
{
- struct pci_dev *pdev = NULL;
struct pci_bus *pbus = NULL;
- int retval;
sysfs_initialized = 1;
- for_each_pci_dev(pdev) {
- retval = pci_create_sysfs_dev_files(pdev);
- if (retval) {
- pci_dev_put(pdev);
- return retval;
- }
- }
while ((pbus = pci_find_next_bus(pbus)))
pci_create_legacy_files(pbus);
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 9348a0fb8084..b78286afe18e 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -463,13 +463,10 @@ int pci_proc_detach_bus(struct pci_bus *bus)
static int __init pci_proc_init(void)
{
- struct pci_dev *dev = NULL;
proc_bus_pci_dir = proc_mkdir("bus/pci", NULL);
proc_create_seq("devices", 0, proc_bus_pci_dir,
&proc_bus_pci_devices_op);
proc_initialized = 1;
- for_each_pci_dev(dev)
- pci_proc_attach_device(dev);
return 0;
}
Bests,
Shuan
On Mon, Jul 21, 2025 at 3:16 PM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
>
> On Sun, Jul 20, 2025 at 05:34:44PM GMT, Lukas Wunner wrote:
> > On Sun, Jul 20, 2025 at 10:15:08AM -0500, Mario Limonciello wrote:
> > > 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.
> > [...]
> > > @@ -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;
> > >
> >
> > pci_create_sysfs_dev_files() is called from two places, pci_bus_add_device()
> > and the late_initcall pci_sysfs_init().
> >
>
> Separate question: Do we really need to call pci_create_sysfs_dev_files() from
> pci_sysfs_init()? It is already getting called from pci_bus_add_device() and it
> looks redundant to me. More importantly, it can also lead to a race (though
> won't happen practically) [1].
>
> Same goes for pci_proc_attach_device(). I was tempted to submit a patch removing
> both these calls from pci_sysfs_init() and pci_proc_init(), but wanted to check
> first.
>
> [1] https://lore.kernel.org/linux-pci/r7fgb5xrn6ocstq6ctq4q7r4o2esgh4rqr44c3u234kcep6thk@bge2vzl33ptb/
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
>
