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
