On Mon, Sep 18, 2017 at 6:53 PM, Stuart Hayes <stuart.w.hayes@xxxxxxxxx> wrote: > Bjorn et al, > > I'm hoping to enhance network device naming in systemd to parse sysfs "virtfn%u" and "physfn" links, so it understands virtual devices. > > I've noticed that the udev event which triggers the systemd/biosdevname network device naming--the udev "add" event for the network device, which happens within pci_bus_add_device() when the network driver's probe function calls register_netdev()--occurs before the sysfs links "physfn" and "virtfn%u" are created. This creates a race when the network naming code tries to look at those links (something biosdevname already does). > > Is there any reason why we couldn't switch the order of those calls to eliminate the race, as shown in the patch below? I couldn't think of any, since those links apply to the pci subsystem devices in sysfs (which are already created), not the net subsystem devices. I'd be happy to submit a patch if that looks ok. > > Thank you, > Stuart > > > --- linux-4.14-rc1/drivers/pci/iov.c.orig 2017-09-18 15:00:43.168255665 -0500 > +++ linux-4.14-rc1/drivers/pci/iov.c 2017-09-18 15:07:04.792280999 -0500 > @@ -162,7 +162,6 @@ int pci_iov_add_virtfn(struct pci_dev *d > > pci_device_add(virtfn, virtfn->bus); > > - pci_bus_add_device(virtfn); > sprintf(buf, "virtfn%u", id); > rc = sysfs_create_link(&dev->dev.kobj, &virtfn->dev.kobj, buf); > if (rc) > @@ -173,6 +172,8 @@ int pci_iov_add_virtfn(struct pci_dev *d > > kobject_uevent(&virtfn->dev.kobj, KOBJ_CHANGE); > > + pci_bus_add_device(virtfn); > + > return 0; > > failed2: > > So looking over the change the only thing I wasn't sure about was if the exception handling paths all worked out correctly, and from what I can tell it looks like this change didn't introduce any new errors. The one thing I think might be here though before you patch was introduced is a possible memory leak if pci_setup_device() fails as we don't ever free the memory allocated to virtfn. It looks liike checking for the error was added somewhat recently and I suspect nobody was checking for the memory leak. I don't know if you would want to get that as a part of your patchset or not. If not I can probably try to get around to it later this week. - Alex
