> From: Jason Gunthorpe <jgg@xxxxxxxxxx> > Sent: Tuesday, April 26, 2022 7:01 AM > > The last user of this function is in PCI callbacks that want to convert > their struct pci_dev to a vfio_device. Instead of searching use the > vfio_device available trivially through the drvdata. > > When a callback in the device_driver is called, the caller must hold the > device_lock() on dev. The purpose of the device_lock is to prevent > remove() from being called (see __device_release_driver), and allow the > driver to safely interact with its drvdata without races. > > The PCI core correctly follows this and holds the device_lock() when > calling error_detected (see report_error_detected) and > sriov_configure (see sriov_numvfs_store). Above is clear for the change in this patch. > > Further, since the drvdata holds a positive refcount on the vfio_device > any access of the drvdata, under the driver_lock, from a driver callback > needs no further protection or refcounting. but I'm confused by driver_lock here and below. Which driver_lock is referred to in this context? > > Thus the remark in the vfio_device_get_from_dev() comment does not apply > here, VFIO PCI drivers all call vfio_unregister_group_dev() from their > remove callbacks under the driver lock and cannot race with the remaining > callers. > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx> Nevertheless, this patch sounds the correct thing to do: Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx> but with one nit... [...] > @@ -1960,8 +1942,7 @@ int vfio_pci_core_sriov_configure(struct pci_dev > *pdev, int nr_virtfn) > ret = pci_enable_sriov(pdev, nr_virtfn); > if (ret) > goto out_del; > - ret = nr_virtfn; > - goto out_put; > + return ret; pci_enable_sriov() returns 0 on success while .sriov_configure() is expected to return enabled nr_virtfn on success. Above changes the semantics.
