Re: Usage of sriov_drivers_autoprobe

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, May 30, 2023 at 01:58:00PM -0700, Ben Gardon wrote:
> Hi Bodong, PCI folk generally,
> 
> I've found an issue with sriov_drivers_autoprobe not working as I
> would expect it to and I'd like to check if my expectations are
> incorrect or if it's not working as intended.
> 
> Please consider the sequence below
> 
> /sys/bus/pci/devices/0000:12:34.1# echo 0 > sriov_numvfs
> /sys/bus/pci/devices/0000:12:34.1# echo 0 > sriov_drivers_autoprobe
> /sys/bus/pci/devices/0000:12:34.1# echo 1 > sriov_numvfs
> (Let's say 0000:13:ab.0 is a VF of 0000:12:34.1)
> /sys/bus/pci/devices/0000:12:34.1# echo 0000:13:ab.0 >
> /sys/bus/pci/drivers/vfio-pci/bind
> -bash: echo: write error: No such device
> /sys/bus/pci/devices/0000:12:34.1# echo 1 > sriov_drivers_autoprobe
> /sys/bus/pci/devices/0000:12:34.1# echo 0000:13:ab.0 >
> /sys/bus/pci/drivers/vfio-pci/bind
> /sys/bus/pci/devices/0000:12:34.1# echo 0000:13:ab.0 >
> /sys/bus/pci/drivers/vfio-pci/unbind
> /sys/bus/pci/devices/0000:12:34.1#
> 
> From the above, we can see that having sriov_drivers_autoprobe unset
> prevents even manually binding a driver, after VF initialization. This
> seems unintentional, but it can be worked around by unsetting
> sriov_drivers_autoprobe.
> 
> If this is how it was intended to work please let me know. If it is,
> then the documentation should be updated. It says: "Note that changing
> this file does not affect already-enabled VFs." But that does not
> appear to be true.

I think the intent of the doc was something like this:

  Note that changing this file only affects future attempts to bind
  VFs to a driver, e.g., when VFs are enabled or a new driver is
  loaded.  Setting it to 0 does not unbind VFs from drivers, and
  setting it to 1 does not cause existing VFs to be bound to drivers.

I'm happy to update the doc if that seems right.

This behavior IS a little different from /sys/bus/*/drivers_autoprobe.
In that case, I think manual binding when you write to /sys/*/bind
*does* work even after writing 0 to /sys/bus/*/drivers_autoprobe.

This is because the drivers_autoprobe check happens earlier, in
bus_probe_device() or bus_add_driver(), before we get down to
driver_probe_device():

  pci_device_add(dev)                         # add new device
    device_add
      bus_probe_device
        if (sp->drivers_autoprobe)            # set by /sys/*/drivers_autoprobe
          device_initial_probe
            bus_for_each_drv(__device_attach_driver)
              __device_attach_driver
                driver_probe_device(drv, dev) # bind device to driver
		  pci_device_probe

  pci_register_driver(drv)                    # add new driver
    driver_register
      bus_add_driver
        if (sp->drivers_autoprobe)            # set by /sys/*/drivers_autoprobe
          driver_attach
            bus_for_each_dev(__driver_attach)
              __driver_attach
                driver_probe_device(drv, dev) # bind device to driver
		  pci_device_probe

When we write a device ID to /sys/*/bind path, we look up the device
and bypass the drivers_autoprobe check:

  bind_store(drv, buf)                        # /sys/bus/*/bind
    dev = bus_find_device_by_name(buf)
    if (driver_match_device(drv, dev))
      device_driver_attach(drv, dev)
        driver_probe_device                   # bind device to driver
	  pci_device_probe

It would be nice if sriov_drivers_autoprobe worked the same way.  The
problem is the sriov->drivers_autoprobe check happens inside
pci_device_probe(), so we don't know whether we came from bind_store()
or the other paths.

The current generic drivers_autoprobe is a per-bus thing (set by
bus_register() and the sysfs file), and sriov_drivers_autoprobe is a
per-PF thing.  I could imagine a new struct bus_type callback where we
could do something like this:

  bus_probe_device(dev)
  {
    struct bus_type bus = dev->bus;

    if (sp->drivers_autoprobe) {
      if (!bus->autoprobe || bus->autoprobe(dev))
        device_initial_probe(dev)
    }
  }

That would give us the flexibility to make this work as you expected
by moving the check from pci_device_probe() to the callback.

Bjorn