On Tue, Apr 26, 2022 at 03:51:13AM +0000, Tian, Kevin wrote: > > 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? That is a typo, it should be device_lock > > 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. Arg, I botched it when rebasing over the final version of the rc patch.. Thanks! Jason
