diff mbox series

[v2,13/14] vfio/pci: Replace uses of vfio_device_data() with container_of

Message ID 13-v2-20d933792272+4ff-vfio1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Embed struct vfio_device in all sub-structures | expand

Commit Message

Jason Gunthorpe March 13, 2021, 12:56 a.m. UTC
This tidies a few confused places that think they can have a refcount on
the vfio_device but the device_data could be NULL, that isn't possible by
design.

Most of the change falls out when struct vfio_devices is updated to just
store the struct vfio_pci_device itself. This wasn't possible before
because there was no easy way to get from the 'struct vfio_pci_device' to
the 'struct vfio_device' to put back the refcount.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci.c | 67 +++++++++++++------------------------
 1 file changed, 24 insertions(+), 43 deletions(-)

Comments

Tian, Kevin March 16, 2021, 8:20 a.m. UTC | #1
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, March 13, 2021 8:56 AM
> 
> This tidies a few confused places that think they can have a refcount on
> the vfio_device but the device_data could be NULL, that isn't possible by
> design.
> 
> Most of the change falls out when struct vfio_devices is updated to just
> store the struct vfio_pci_device itself. This wasn't possible before
> because there was no easy way to get from the 'struct vfio_pci_device' to
> the 'struct vfio_device' to put back the refcount.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/pci/vfio_pci.c | 67 +++++++++++++------------------------
>  1 file changed, 24 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 5f1a782d1c65ae..1f70387c8afe37 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -517,30 +517,29 @@ static void vfio_pci_disable(struct vfio_pci_device
> *vdev)
> 
>  static struct pci_driver vfio_pci_driver;
> 
> -static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
> -					   struct vfio_device **pf_dev)
> +static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev)
>  {
>  	struct pci_dev *physfn = pci_physfn(vdev->pdev);
> +	struct vfio_device *pf_dev;
> 
>  	if (!vdev->pdev->is_virtfn)
>  		return NULL;
> 
> -	*pf_dev = vfio_device_get_from_dev(&physfn->dev);
> -	if (!*pf_dev)
> +	pf_dev = vfio_device_get_from_dev(&physfn->dev);
> +	if (!pf_dev)
>  		return NULL;
> 
>  	if (pci_dev_driver(physfn) != &vfio_pci_driver) {
> -		vfio_device_put(*pf_dev);
> +		vfio_device_put(pf_dev);
>  		return NULL;
>  	}
> 
> -	return vfio_device_data(*pf_dev);
> +	return container_of(pf_dev, struct vfio_pci_device, vdev);
>  }
> 
>  static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
>  {
> -	struct vfio_device *pf_dev;
> -	struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev, &pf_dev);
> +	struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev);
> 
>  	if (!pf_vdev)
>  		return;
> @@ -550,7 +549,7 @@ static void vfio_pci_vf_token_user_add(struct
> vfio_pci_device *vdev, int val)
>  	WARN_ON(pf_vdev->vf_token->users < 0);
>  	mutex_unlock(&pf_vdev->vf_token->lock);
> 
> -	vfio_device_put(pf_dev);
> +	vfio_device_put(&pf_vdev->vdev);
>  }
> 
>  static void vfio_pci_release(struct vfio_device *core_vdev)
> @@ -794,7 +793,7 @@ int vfio_pci_register_dev_region(struct
> vfio_pci_device *vdev,
>  }
> 
>  struct vfio_devices {
> -	struct vfio_device **devices;
> +	struct vfio_pci_device **devices;
>  	int cur_index;
>  	int max_index;
>  };
> @@ -1283,9 +1282,7 @@ static long vfio_pci_ioctl(struct vfio_device
> *core_vdev,
>  			goto hot_reset_release;
> 
>  		for (; mem_idx < devs.cur_index; mem_idx++) {
> -			struct vfio_pci_device *tmp;
> -
> -			tmp = vfio_device_data(devs.devices[mem_idx]);
> +			struct vfio_pci_device *tmp = devs.devices[mem_idx];
> 
>  			ret = down_write_trylock(&tmp->memory_lock);
>  			if (!ret) {
> @@ -1300,17 +1297,13 @@ static long vfio_pci_ioctl(struct vfio_device
> *core_vdev,
> 
>  hot_reset_release:
>  		for (i = 0; i < devs.cur_index; i++) {
> -			struct vfio_device *device;
> -			struct vfio_pci_device *tmp;
> -
> -			device = devs.devices[i];
> -			tmp = vfio_device_data(device);
> +			struct vfio_pci_device *tmp = devs.devices[i];
> 
>  			if (i < mem_idx)
>  				up_write(&tmp->memory_lock);
>  			else
>  				mutex_unlock(&tmp->vma_lock);
> -			vfio_device_put(device);
> +			vfio_device_put(&tmp->vdev);
>  		}
>  		kfree(devs.devices);
> 
> @@ -1777,8 +1770,7 @@ static int vfio_pci_validate_vf_token(struct
> vfio_pci_device *vdev,
>  		return 0; /* No VF token provided or required */
> 
>  	if (vdev->pdev->is_virtfn) {
> -		struct vfio_device *pf_dev;
> -		struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev,
> &pf_dev);
> +		struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev);
>  		bool match;
> 
>  		if (!pf_vdev) {
> @@ -1791,7 +1783,7 @@ static int vfio_pci_validate_vf_token(struct
> vfio_pci_device *vdev,
>  		}
> 
>  		if (!vf_token) {
> -			vfio_device_put(pf_dev);
> +			vfio_device_put(&pf_vdev->vdev);
>  			pci_info_ratelimited(vdev->pdev,
>  				"VF token required to access device\n");
>  			return -EACCES;
> @@ -1801,7 +1793,7 @@ static int vfio_pci_validate_vf_token(struct
> vfio_pci_device *vdev,
>  		match = uuid_equal(uuid, &pf_vdev->vf_token->uuid);
>  		mutex_unlock(&pf_vdev->vf_token->lock);
> 
> -		vfio_device_put(pf_dev);
> +		vfio_device_put(&pf_vdev->vdev);
> 
>  		if (!match) {
>  			pci_info_ratelimited(vdev->pdev,
> @@ -2122,11 +2114,7 @@ static pci_ers_result_t
> vfio_pci_aer_err_detected(struct pci_dev *pdev,
>  	if (device == NULL)
>  		return PCI_ERS_RESULT_DISCONNECT;
> 
> -	vdev = vfio_device_data(device);
> -	if (vdev == NULL) {
> -		vfio_device_put(device);
> -		return PCI_ERS_RESULT_DISCONNECT;
> -	}
> +	vdev = container_of(device, struct vfio_pci_device, vdev);
> 
>  	mutex_lock(&vdev->igate);
> 
> @@ -2142,7 +2130,6 @@ static pci_ers_result_t
> vfio_pci_aer_err_detected(struct pci_dev *pdev,
> 
>  static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
>  {
> -	struct vfio_pci_device *vdev;
>  	struct vfio_device *device;
>  	int ret = 0;
> 
> @@ -2155,12 +2142,6 @@ static int vfio_pci_sriov_configure(struct pci_dev
> *pdev, int nr_virtfn)
>  	if (!device)
>  		return -ENODEV;
> 
> -	vdev = vfio_device_data(device);
> -	if (!vdev) {
> -		vfio_device_put(device);
> -		return -ENODEV;
> -	}
> -
>  	if (nr_virtfn == 0)
>  		pci_disable_sriov(pdev);
>  	else
> @@ -2220,7 +2201,7 @@ static int vfio_pci_reflck_find(struct pci_dev *pdev,
> void *data)
>  		return 0;
>  	}
> 
> -	vdev = vfio_device_data(device);
> +	vdev = container_of(device, struct vfio_pci_device, vdev);
> 
>  	if (vdev->reflck) {
>  		vfio_pci_reflck_get(vdev->reflck);
> @@ -2282,7 +2263,7 @@ static int vfio_pci_get_unused_devs(struct pci_dev
> *pdev, void *data)
>  		return -EBUSY;
>  	}
> 
> -	vdev = vfio_device_data(device);
> +	vdev = container_of(device, struct vfio_pci_device, vdev);
> 
>  	/* Fault if the device is not unused */
>  	if (vdev->refcnt) {
> @@ -2290,7 +2271,7 @@ static int vfio_pci_get_unused_devs(struct pci_dev
> *pdev, void *data)
>  		return -EBUSY;
>  	}
> 
> -	devs->devices[devs->cur_index++] = device;
> +	devs->devices[devs->cur_index++] = vdev;
>  	return 0;
>  }
> 
> @@ -2312,7 +2293,7 @@ static int
> vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
>  		return -EBUSY;
>  	}
> 
> -	vdev = vfio_device_data(device);
> +	vdev = container_of(device, struct vfio_pci_device, vdev);
> 
>  	/*
>  	 * Locking multiple devices is prone to deadlock, runaway and
> @@ -2323,7 +2304,7 @@ static int
> vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
>  		return -EBUSY;
>  	}
> 
> -	devs->devices[devs->cur_index++] = device;
> +	devs->devices[devs->cur_index++] = vdev;
>  	return 0;
>  }
> 
> @@ -2371,7 +2352,7 @@ static void vfio_pci_try_bus_reset(struct
> vfio_pci_device *vdev)
> 
>  	/* Does at least one need a reset? */
>  	for (i = 0; i < devs.cur_index; i++) {
> -		tmp = vfio_device_data(devs.devices[i]);
> +		tmp = devs.devices[i];
>  		if (tmp->needs_reset) {
>  			ret = pci_reset_bus(vdev->pdev);
>  			break;
> @@ -2380,7 +2361,7 @@ static void vfio_pci_try_bus_reset(struct
> vfio_pci_device *vdev)
> 
>  put_devs:
>  	for (i = 0; i < devs.cur_index; i++) {
> -		tmp = vfio_device_data(devs.devices[i]);
> +		tmp = devs.devices[i];
> 
>  		/*
>  		 * If reset was successful, affected devices no longer need
> @@ -2396,7 +2377,7 @@ static void vfio_pci_try_bus_reset(struct
> vfio_pci_device *vdev)
>  				vfio_pci_set_power_state(tmp, PCI_D3hot);
>  		}
> 
> -		vfio_device_put(devs.devices[i]);
> +		vfio_device_put(&tmp->vdev);
>  	}
> 
>  	kfree(devs.devices);
> --
> 2.30.2
Cornelia Huck March 17, 2021, 12:06 p.m. UTC | #2
On Fri, 12 Mar 2021 20:56:05 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> This tidies a few confused places that think they can have a refcount on
> the vfio_device but the device_data could be NULL, that isn't possible by
> design.
> 
> Most of the change falls out when struct vfio_devices is updated to just
> store the struct vfio_pci_device itself. This wasn't possible before
> because there was no easy way to get from the 'struct vfio_pci_device' to
> the 'struct vfio_device' to put back the refcount.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 67 +++++++++++++------------------------
>  1 file changed, 24 insertions(+), 43 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 5f1a782d1c65ae..1f70387c8afe37 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -517,30 +517,29 @@  static void vfio_pci_disable(struct vfio_pci_device *vdev)
 
 static struct pci_driver vfio_pci_driver;
 
-static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
-					   struct vfio_device **pf_dev)
+static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev)
 {
 	struct pci_dev *physfn = pci_physfn(vdev->pdev);
+	struct vfio_device *pf_dev;
 
 	if (!vdev->pdev->is_virtfn)
 		return NULL;
 
-	*pf_dev = vfio_device_get_from_dev(&physfn->dev);
-	if (!*pf_dev)
+	pf_dev = vfio_device_get_from_dev(&physfn->dev);
+	if (!pf_dev)
 		return NULL;
 
 	if (pci_dev_driver(physfn) != &vfio_pci_driver) {
-		vfio_device_put(*pf_dev);
+		vfio_device_put(pf_dev);
 		return NULL;
 	}
 
-	return vfio_device_data(*pf_dev);
+	return container_of(pf_dev, struct vfio_pci_device, vdev);
 }
 
 static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
 {
-	struct vfio_device *pf_dev;
-	struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev, &pf_dev);
+	struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev);
 
 	if (!pf_vdev)
 		return;
@@ -550,7 +549,7 @@  static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
 	WARN_ON(pf_vdev->vf_token->users < 0);
 	mutex_unlock(&pf_vdev->vf_token->lock);
 
-	vfio_device_put(pf_dev);
+	vfio_device_put(&pf_vdev->vdev);
 }
 
 static void vfio_pci_release(struct vfio_device *core_vdev)
@@ -794,7 +793,7 @@  int vfio_pci_register_dev_region(struct vfio_pci_device *vdev,
 }
 
 struct vfio_devices {
-	struct vfio_device **devices;
+	struct vfio_pci_device **devices;
 	int cur_index;
 	int max_index;
 };
@@ -1283,9 +1282,7 @@  static long vfio_pci_ioctl(struct vfio_device *core_vdev,
 			goto hot_reset_release;
 
 		for (; mem_idx < devs.cur_index; mem_idx++) {
-			struct vfio_pci_device *tmp;
-
-			tmp = vfio_device_data(devs.devices[mem_idx]);
+			struct vfio_pci_device *tmp = devs.devices[mem_idx];
 
 			ret = down_write_trylock(&tmp->memory_lock);
 			if (!ret) {
@@ -1300,17 +1297,13 @@  static long vfio_pci_ioctl(struct vfio_device *core_vdev,
 
 hot_reset_release:
 		for (i = 0; i < devs.cur_index; i++) {
-			struct vfio_device *device;
-			struct vfio_pci_device *tmp;
-
-			device = devs.devices[i];
-			tmp = vfio_device_data(device);
+			struct vfio_pci_device *tmp = devs.devices[i];
 
 			if (i < mem_idx)
 				up_write(&tmp->memory_lock);
 			else
 				mutex_unlock(&tmp->vma_lock);
-			vfio_device_put(device);
+			vfio_device_put(&tmp->vdev);
 		}
 		kfree(devs.devices);
 
@@ -1777,8 +1770,7 @@  static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
 		return 0; /* No VF token provided or required */
 
 	if (vdev->pdev->is_virtfn) {
-		struct vfio_device *pf_dev;
-		struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev, &pf_dev);
+		struct vfio_pci_device *pf_vdev = get_pf_vdev(vdev);
 		bool match;
 
 		if (!pf_vdev) {
@@ -1791,7 +1783,7 @@  static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
 		}
 
 		if (!vf_token) {
-			vfio_device_put(pf_dev);
+			vfio_device_put(&pf_vdev->vdev);
 			pci_info_ratelimited(vdev->pdev,
 				"VF token required to access device\n");
 			return -EACCES;
@@ -1801,7 +1793,7 @@  static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
 		match = uuid_equal(uuid, &pf_vdev->vf_token->uuid);
 		mutex_unlock(&pf_vdev->vf_token->lock);
 
-		vfio_device_put(pf_dev);
+		vfio_device_put(&pf_vdev->vdev);
 
 		if (!match) {
 			pci_info_ratelimited(vdev->pdev,
@@ -2122,11 +2114,7 @@  static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 	if (device == NULL)
 		return PCI_ERS_RESULT_DISCONNECT;
 
-	vdev = vfio_device_data(device);
-	if (vdev == NULL) {
-		vfio_device_put(device);
-		return PCI_ERS_RESULT_DISCONNECT;
-	}
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	mutex_lock(&vdev->igate);
 
@@ -2142,7 +2130,6 @@  static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 
 static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 {
-	struct vfio_pci_device *vdev;
 	struct vfio_device *device;
 	int ret = 0;
 
@@ -2155,12 +2142,6 @@  static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 	if (!device)
 		return -ENODEV;
 
-	vdev = vfio_device_data(device);
-	if (!vdev) {
-		vfio_device_put(device);
-		return -ENODEV;
-	}
-
 	if (nr_virtfn == 0)
 		pci_disable_sriov(pdev);
 	else
@@ -2220,7 +2201,7 @@  static int vfio_pci_reflck_find(struct pci_dev *pdev, void *data)
 		return 0;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	if (vdev->reflck) {
 		vfio_pci_reflck_get(vdev->reflck);
@@ -2282,7 +2263,7 @@  static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	/* Fault if the device is not unused */
 	if (vdev->refcnt) {
@@ -2290,7 +2271,7 @@  static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	devs->devices[devs->cur_index++] = device;
+	devs->devices[devs->cur_index++] = vdev;
 	return 0;
 }
 
@@ -2312,7 +2293,7 @@  static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	/*
 	 * Locking multiple devices is prone to deadlock, runaway and
@@ -2323,7 +2304,7 @@  static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	devs->devices[devs->cur_index++] = device;
+	devs->devices[devs->cur_index++] = vdev;
 	return 0;
 }
 
@@ -2371,7 +2352,7 @@  static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 
 	/* Does at least one need a reset? */
 	for (i = 0; i < devs.cur_index; i++) {
-		tmp = vfio_device_data(devs.devices[i]);
+		tmp = devs.devices[i];
 		if (tmp->needs_reset) {
 			ret = pci_reset_bus(vdev->pdev);
 			break;
@@ -2380,7 +2361,7 @@  static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 
 put_devs:
 	for (i = 0; i < devs.cur_index; i++) {
-		tmp = vfio_device_data(devs.devices[i]);
+		tmp = devs.devices[i];
 
 		/*
 		 * If reset was successful, affected devices no longer need
@@ -2396,7 +2377,7 @@  static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 				vfio_pci_set_power_state(tmp, PCI_D3hot);
 		}
 
-		vfio_device_put(devs.devices[i]);
+		vfio_device_put(&tmp->vdev);
 	}
 
 	kfree(devs.devices);