On Thu, 30 May 2019 08:47:27 -0500
Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Wed, May 01, 2019 at 11:00:16AM -0600, Alex Williamson wrote:
> > Commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
> > VF driver binding") allows the user to specify that drivers for VFs of
> > a PF should not be probed, but it actually causes pci_device_probe() to
> > return success back to the driver core in this case. Therefore by all
> > sysfs appearances the device is bound to a driver, the driver link from
> > the device exists as does the device link back from the driver, yet the
> > driver's probe function is never called on the device. We also fail to
> > do any sort of cleanup when we're prohibited from probing the device,
> > the irq setup remains in place and we even hold a device reference.
> >
> > Instead, abort with errno before any setup or references are taken when
> > pci_device_can_probe() prevents us from trying to probe the device.
> >
> > Fixes: 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>
> Applied to pci/enumeration for v5.3, thanks!
>
> The scenario you describe, Laine, indeed sounds cumbersome. If you
> want to propose an alternate or additional patch to address that, or
> if you think Alex's patch will make it harder to clean up that
> scenario, I'm all ears. But it seems like Alex's patch is an
> improvement even if it leaves some problems unsolved.
Hi Bjorn,
It's probably deeper in your queue, but I've posted:
https://patchwork.kernel.org/patch/10937577/
which allows devices with a driver_override to always probe. I think
it gives us the more desirable usage model. Thanks,
Alex
> > ---
> >
> > This issue is easily tested by disabling sriov_drivers_autoprobe and
> > creating VFs:
> >
> > # echo 0 > sriov_drivers_autoprobe
> > # echo 3 > sriov_numvfs
> > # readlink -f virtfn*/driver
> > /sys/bus/pci/drivers/iavf
> > /sys/bus/pci/drivers/iavf
> > /sys/bus/pci/drivers/iavf
> > (yet no netdevs exist for these VFs)
> >
> > The semantics of this autoprobe disabling are a bit strange for the
> > user as well, I suppose it works if we force a bind through a driver's
> > bind attribute, but tools like libvirt and driverctl expect to bind
> > devices by setting the driver_override and then pushing the device
> > through driver_probe on the bus. Is the intention of disabling
> > "autoprobe" that a driver_override should still work? Otherwise the
> > user needs to set the driver_override for each VF, re-enable
> > sriov_drivers_autoprobe on the PF, and then probe the VFs. Thus maybe
> > pci_device_can_probe() should allow probes of the driver_override
> > driver? Thanks,
> >
> > Alex
> >
> > drivers/pci/pci-driver.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 71853befd435..da7b82e56c83 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -414,6 +414,9 @@ static int pci_device_probe(struct device *dev)
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > struct pci_driver *drv = to_pci_driver(dev->driver);
> >
> > + if (!pci_device_can_probe(pci_dev))
> > + return -ENODEV;
> > +
> > pci_assign_irq(pci_dev);
> >
> > error = pcibios_alloc_irq(pci_dev);
> > @@ -421,12 +424,10 @@ static int pci_device_probe(struct device *dev)
> > return error;
> >
> > pci_dev_get(pci_dev);
> > - if (pci_device_can_probe(pci_dev)) {
> > - error = __pci_device_probe(drv, pci_dev);
> > - if (error) {
> > - pcibios_free_irq(pci_dev);
> > - pci_dev_put(pci_dev);
> > - }
> > + error = __pci_device_probe(drv, pci_dev);
> > + if (error) {
> > + pcibios_free_irq(pci_dev);
> > + pci_dev_put(pci_dev);
> > }
> >
> > return error;
> >
