[10/15] vfio/fsl-mc: Use the new device life cycle helpers

Commit Message

Tian, Kevin Aug. 27, 2022, 5:10 p.m. UTC

From: Yi Liu <[e-mail]>

Export symbol of vfio_release_device_set() so fsl-mc @init can handle
the error path cleanly instead of assuming certain vfio core API can
help release device_set afterwards.

Signed-off-by: Yi Liu <[e-mail]>
Signed-off-by: Kevin Tian <[e-mail]>
---
 drivers/vfio/fsl-mc/vfio_fsl_mc.c | 87 +++++++++++++++++++------------
 drivers/vfio/vfio_main.c          |  3 +-
 include/linux/vfio.h              |  1 +
 3 files changed, 56 insertions(+), 35 deletions(-)

Comments

Jason Gunthorpe Aug. 31, 2022, 12:22 a.m. UTC | #1

On Sun, Aug 28, 2022 at 01:10:32AM +0800, Kevin Tian wrote:
> From: Yi Liu <[e-mail]>
> 
> Export symbol of vfio_release_device_set() so fsl-mc @init can handle
> the error path cleanly instead of assuming certain vfio core API can
> help release device_set afterwards.

I think you should leave it as is, the "device_set" cleanup is just
something handled completely internally to vfio

If ops->init fails then we expect the core code to clean the
device_set, and it does because it calls vfio_init_device() already.

Having a single weirdly placed release in the driver is pretty
confusing, IMHO.

Jason

Tian, Kevin Aug. 31, 2022, 6:14 a.m. UTC | #2

> From: Jason Gunthorpe <[e-mail]>
> Sent: Wednesday, August 31, 2022 8:23 AM
> 
> On Sun, Aug 28, 2022 at 01:10:32AM +0800, Kevin Tian wrote:
> > From: Yi Liu <[e-mail]>
> >
> > Export symbol of vfio_release_device_set() so fsl-mc @init can handle
> > the error path cleanly instead of assuming certain vfio core API can
> > help release device_set afterwards.
> 
> I think you should leave it as is, the "device_set" cleanup is just
> something handled completely internally to vfio
> 
> If ops->init fails then we expect the core code to clean the
> device_set, and it does because it calls vfio_init_device() already.
> 
> Having a single weirdly placed release in the driver is pretty
> confusing, IMHO.
> 

I considered both. In the end I chose the current way being it's
clearer if just looking at @init. But yes having a release called
by driver is also confusing. Probably having a pair of assign/deassign
helpers is more clearer, but I'll not continue this path for unnecessary
complexity.

So I'll leave it as is and add a comment to clarify it.

Patch

diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 3feff729f3ce..eec3cb914f57 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -418,16 +418,7 @@  static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
 	return vfio_fsl_mc_mmap_mmio(vdev->regions[index], vma);
 }
 
-static const struct vfio_device_ops vfio_fsl_mc_ops = {
-	.name		= "vfio-fsl-mc",
-	.open_device	= vfio_fsl_mc_open_device,
-	.close_device	= vfio_fsl_mc_close_device,
-	.ioctl		= vfio_fsl_mc_ioctl,
-	.read		= vfio_fsl_mc_read,
-	.write		= vfio_fsl_mc_write,
-	.mmap		= vfio_fsl_mc_mmap,
-};
-
+static const struct vfio_device_ops vfio_fsl_mc_ops;
 static int vfio_fsl_mc_bus_notifier(struct notifier_block *nb,
 				    unsigned long action, void *data)
 {
@@ -518,35 +509,49 @@  static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev)
 	bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
 }
 
-static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
+static int vfio_fsl_mc_init_dev(struct vfio_device *core_vdev)
 {
-	struct vfio_fsl_mc_device *vdev;
-	struct device *dev = &mc_dev->dev;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
+	struct fsl_mc_device *mc_dev = to_fsl_mc_device(core_vdev->dev);
 	int ret;
 
-	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
-	if (!vdev)
-		return -ENOMEM;
-
-	vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops);
 	vdev->mc_dev = mc_dev;
 	mutex_init(&vdev->igate);
 
 	if (is_fsl_mc_bus_dprc(mc_dev))
-		ret = vfio_assign_device_set(&vdev->vdev, &mc_dev->dev);
+		ret = vfio_assign_device_set(core_vdev, &mc_dev->dev);
 	else
-		ret = vfio_assign_device_set(&vdev->vdev, mc_dev->dev.parent);
+		ret = vfio_assign_device_set(core_vdev, mc_dev->dev.parent);
+
 	if (ret)
-		goto out_uninit;
+		return ret;
 
 	ret = vfio_fsl_mc_init_device(vdev);
 	if (ret)
-		goto out_uninit;
+		goto err_assign;
+	return 0;
+
+err_assign:
+	vfio_release_device_set(core_vdev);
+	return ret;
+}
+
+static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
+{
+	struct vfio_fsl_mc_device *vdev;
+	struct device *dev = &mc_dev->dev;
+	int ret;
+
+	vdev = vfio_alloc_device(vfio_fsl_mc_device, vdev, dev,
+				 &vfio_fsl_mc_ops);
+	if (IS_ERR(vdev))
+		return PTR_ERR(vdev);
 
 	ret = vfio_register_group_dev(&vdev->vdev);
 	if (ret) {
 		dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
-		goto out_device;
+		goto out_put_vdev;
 	}
 
 	ret = vfio_fsl_mc_scan_container(mc_dev);
@@ -557,30 +562,44 @@  static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 
 out_group_dev:
 	vfio_unregister_group_dev(&vdev->vdev);
-out_device:
-	vfio_fsl_uninit_device(vdev);
-out_uninit:
-	vfio_uninit_group_dev(&vdev->vdev);
-	kfree(vdev);
+out_put_vdev:
+	vfio_put_device(&vdev->vdev);
 	return ret;
 }
 
+void vfio_fsl_mc_release_dev(struct vfio_device *core_vdev)
+{
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
+
+	vfio_fsl_uninit_device(vdev);
+	mutex_destroy(&vdev->igate);
+	vfio_free_device(core_vdev);
+}
+
 static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
 {
 	struct device *dev = &mc_dev->dev;
 	struct vfio_fsl_mc_device *vdev = dev_get_drvdata(dev);
 
 	vfio_unregister_group_dev(&vdev->vdev);
-	mutex_destroy(&vdev->igate);
-
 	dprc_remove_devices(mc_dev, NULL, 0);
-	vfio_fsl_uninit_device(vdev);
-
-	vfio_uninit_group_dev(&vdev->vdev);
-	kfree(vdev);
+	vfio_put_device(&vdev->vdev);
 	return 0;
 }
 
+static const struct vfio_device_ops vfio_fsl_mc_ops = {
+	.name		= "vfio-fsl-mc",
+	.init		= vfio_fsl_mc_init_dev,
+	.release	= vfio_fsl_mc_release_dev,
+	.open_device	= vfio_fsl_mc_open_device,
+	.close_device	= vfio_fsl_mc_close_device,
+	.ioctl		= vfio_fsl_mc_ioctl,
+	.read		= vfio_fsl_mc_read,
+	.write		= vfio_fsl_mc_write,
+	.mmap		= vfio_fsl_mc_mmap,
+};
+
 static struct fsl_mc_driver vfio_fsl_mc_driver = {
 	.probe		= vfio_fsl_mc_probe,
 	.remove		= vfio_fsl_mc_remove,
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index af8aad116f2b..9485da17f2e6 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -141,7 +141,7 @@  int vfio_assign_device_set(struct vfio_device *device, void *set_id)
 }
 EXPORT_SYMBOL_GPL(vfio_assign_device_set);
 
-static void vfio_release_device_set(struct vfio_device *device)
+void vfio_release_device_set(struct vfio_device *device)
 {
 	struct vfio_device_set *dev_set = device->dev_set;
 
@@ -161,6 +161,7 @@  static void vfio_release_device_set(struct vfio_device *device)
 	}
 	xa_unlock(&vfio_device_set_xa);
 }
+EXPORT_SYMBOL_GPL(vfio_release_device_set);
 
 #ifdef CONFIG_VFIO_NOIOMMU
 static void *vfio_noiommu_open(unsigned long arg)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index e1e9e8352903..b0928a81b45a 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -168,6 +168,7 @@  int vfio_register_emulated_iommu_dev(struct vfio_device *device);
 void vfio_unregister_group_dev(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
+void vfio_release_device_set(struct vfio_device *device);
 
 int vfio_mig_get_next_state(struct vfio_device *device,
 			    enum vfio_device_mig_state cur_fsm,