Re: [PATCH] vfio/pci: Remove vfio_device_get_from_dev()

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 26, 2022 at 03:51:13AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 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@nvidia.com>
> 
> Nevertheless, this patch sounds the correct thing to do:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> 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