Re: [PATCH V3 06/13] vfio/pci: Split the pci_driver code out of vfio_pci_core.c

From: Alex Williamson <alex.williamson@redhat.com>
To: Max Gurtovoy <mgurtovoy@nvidia.com>
Cc: Yishai Hadas <yishaih@nvidia.com>, <bhelgaas@google.com>,
	<corbet@lwn.net>, <diana.craciun@oss.nxp.com>,
	<kwankhede@nvidia.com>, <eric.auger@redhat.com>,
	<masahiroy@kernel.org>, <michal.lkml@markovi.net>,
	<linux-pci@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<kvm@vger.kernel.org>, <linux-s390@vger.kernel.org>,
	<linux-kbuild@vger.kernel.org>, <jgg@nvidia.com>,
	<maorg@nvidia.com>, <leonro@nvidia.com>
Subject: Re: [PATCH V3 06/13] vfio/pci: Split the pci_driver code out of vfio_pci_core.c
Date: Tue, 24 Aug 2021 15:48:39 -0600	[thread overview]
Message-ID: <20210824154839.159a1243.alex.williamson@redhat.com> (raw)
In-Reply-To: <393721ae-2183-2b1b-f670-8006992c4e55@nvidia.com>

On Mon, 23 Aug 2021 18:28:49 +0300
Max Gurtovoy <mgurtovoy@nvidia.com> wrote:

> On 8/23/2021 6:16 PM, Alex Williamson wrote:
> > On Sun, 22 Aug 2021 17:35:55 +0300
> > Yishai Hadas <yishaih@nvidia.com> wrote:  
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> new file mode 100644
> >> index 000000000000..15474ebadd98
> >> --- /dev/null
> >> +++ b/drivers/vfio/pci/vfio_pci.c  
> > ...  
> >> +static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
> >> +{
> >> +	might_sleep();
> >> +
> >> +	if (!enable_sriov)
> >> +		return -ENOENT;
> >> +
> >> +	return vfio_pci_core_sriov_configure(pdev, nr_virtfn);
> >> +}  
> > As noted in previous version, why do we need the might_sleep() above
> > when the core code below includes it and there's nothing above that
> > might sleep before that?  Thanks,  
> 
> This is used to mention vfio_pci_core_sriov_configure might sleep.
> 
> If this is redundant, can you please remove this one line upon merge ?

I guess I'm not sure how far up we need to, or should, percolate
might_sleep() annotations.  vfio_pci_core_sriov_configure() calls
vfio_device_get_from_dev() which makes use of mutexes, which I think is
the original reason for the annotation there ahead of those in the PCI
iov code.  But is the annotation through mutex_lock() enough on its own,
ie. should we remove all of our gratuitous annotations in the vfio part
of the code path?  Thanks,

Alex

> >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> >> index 94f062818e0c..87d1960d0d61 100644
> >> --- a/drivers/vfio/pci/vfio_pci_core.c
> >> +++ b/drivers/vfio/pci/vfio_pci_core.c  
> > ...  
> >> -static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
> >> +int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
> >>   {
> >>   	struct vfio_device *device;
> >>   	int ret = 0;
> >>   
> >>   	might_sleep();
> >>   
> >> -	if (!enable_sriov)
> >> -		return -ENOENT;
> >> -
> >>   	device = vfio_device_get_from_dev(&pdev->dev);
> >>   	if (!device)
> >>   		return -ENODEV;  
> 