simplify the mdev interface

simplify the mdev interface

[Christoph Hellwig]

Hi all,

this series signigicantly simplies the mdev driver interface by following
the patterns for device model interaction used elsewhere in the kernel.

Diffstat:
 Documentation/driver-api/vfio-mediated-device.rst |   16 -
 Documentation/s390/vfio-ap.rst                    |    2 
 Documentation/s390/vfio-ccw.rst                   |    2 
 drivers/gpu/drm/i915/gvt/gvt.h                    |    1 
 drivers/gpu/drm/i915/gvt/kvmgt.c                  |  116 ++++----------
 drivers/gpu/drm/i915/i915_drv.h                   |    2 
 drivers/s390/cio/cio.h                            |    3 
 drivers/s390/cio/vfio_ccw_ops.c                   |   32 +--
 drivers/s390/crypto/vfio_ap_ops.c                 |   25 +--
 drivers/s390/crypto/vfio_ap_private.h             |    2 
 drivers/vfio/mdev/mdev_core.c                     |  181 ++++------------------
 drivers/vfio/mdev/mdev_driver.c                   |    1 
 drivers/vfio/mdev/mdev_private.h                  |   38 ----
 drivers/vfio/mdev/mdev_sysfs.c                    |  145 ++---------------
 include/linux/mdev.h                              |   40 +++-
 samples/vfio-mdev/mbochs.c                        |   69 ++++----
 samples/vfio-mdev/mdpy.c                          |   61 +++----
 samples/vfio-mdev/mtty.c                          |   77 +++++----
 18 files changed, 258 insertions(+), 555 deletions(-)

[PATCH 1/8] vfio/mdev: make mdev.h standalone includable

[Christoph Hellwig]

Include <linux/device.h> and <linux/uuid.h> so that users of this headers
don't need to do that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/mdev.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index bb539794f54a8..555c1d015b5f0 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -10,6 +10,9 @@
 #ifndef MDEV_H
 #define MDEV_H
 
+#include <linux/device.h>
+#include <linux/uuid.h>
+
 struct mdev_type;
 
 struct mdev_device {
-- 
2.30.2

Re: [PATCH 1/8] vfio/mdev: make mdev.h standalone includable

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:21AM +0200, Christoph Hellwig wrote:
> Include <linux/device.h> and <linux/uuid.h> so that users of this headers
> don't need to do that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/mdev.h | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 1/8] vfio/mdev: make mdev.h standalone includable

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> Include <linux/device.h> and <linux/uuid.h> so that users of this headers
> don't need to do that.

looks the existing inclusions in those users are not removed...

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/mdev.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index bb539794f54a8..555c1d015b5f0 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -10,6 +10,9 @@
>  #ifndef MDEV_H
>  #define MDEV_H
> 
> +#include <linux/device.h>
> +#include <linux/uuid.h>
> +
>  struct mdev_type;
> 
>  struct mdev_device {
> --
> 2.30.2

[PATCH 2/8] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Christoph Hellwig]

Simplify mdev_{un}register_device by requiring the caller to pass in
a structure allocate as part of the parent device structure.  This
removes the need for a list of parents and the separate mdev_parent
refcount as we can simplify rely on the reference to the parent device.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 .../driver-api/vfio-mediated-device.rst       |  12 +-
 Documentation/s390/vfio-ap.rst                |   2 +-
 Documentation/s390/vfio-ccw.rst               |   2 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c              |   5 +-
 drivers/gpu/drm/i915/i915_drv.h               |   2 +
 drivers/s390/cio/cio.h                        |   2 +
 drivers/s390/cio/vfio_ccw_ops.c               |   6 +-
 drivers/s390/crypto/vfio_ap_ops.c             |   5 +-
 drivers/s390/crypto/vfio_ap_private.h         |   1 +
 drivers/vfio/mdev/mdev_core.c                 | 116 +++---------------
 drivers/vfio/mdev/mdev_private.h              |  23 ----
 drivers/vfio/mdev/mdev_sysfs.c                |   4 +-
 include/linux/mdev.h                          |  15 ++-
 samples/vfio-mdev/mbochs.c                    |   5 +-
 samples/vfio-mdev/mdpy.c                      |   5 +-
 samples/vfio-mdev/mtty.c                      |   6 +-
 16 files changed, 65 insertions(+), 146 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index eded8719180fb..3749f59c855fa 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -60,19 +60,19 @@ devices as examples, as these devices are the first devices to use this module::
      |  MDEV CORE    |
      |   MODULE      |
      |   mdev.ko     |
-     | +-----------+ |  mdev_register_device() +--------------+
+     | +-----------+ |  mdev_register_parent() +--------------+
      | |           | +<------------------------+              |
      | |           | |                         |  nvidia.ko   |<-> physical
      | |           | +------------------------>+              |    device
      | |           | |        callbacks        +--------------+
      | | Physical  | |
-     | |  device   | |  mdev_register_device() +--------------+
+     | |  device   | |  mdev_register_parent() +--------------+
      | | interface | |<------------------------+              |
      | |           | |                         |  i915.ko     |<-> physical
      | |           | +------------------------>+              |    device
      | |           | |        callbacks        +--------------+
      | |           | |
-     | |           | |  mdev_register_device() +--------------+
+     | |           | |  mdev_register_parent() +--------------+
      | |           | +<------------------------+              |
      | |           | |                         | ccw_device.ko|<-> physical
      | |           | +------------------------>+              |    device
@@ -127,8 +127,8 @@ vfio_device_ops.
 When a driver wants to add the GUID creation sysfs to an existing device it has
 probe'd to then it should call::
 
-	extern int  mdev_register_device(struct device *dev,
-	                                 struct mdev_driver *mdev_driver);
+	int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
+			struct mdev_driver *mdev_driver);
 
 This will provide the 'mdev_supported_types/XX/create' files which can then be
 used to trigger the creation of a mdev_device. The created mdev_device will be
@@ -136,7 +136,7 @@ attached to the specified driver.
 
 When the driver needs to remove itself it calls::
 
-	extern void mdev_unregister_device(struct device *dev);
+	void mdev_unregister_parent(struct mdev_parent *parent);
 
 Which will unbind and destroy all the created mdevs and remove the sysfs files.
 
diff --git a/Documentation/s390/vfio-ap.rst b/Documentation/s390/vfio-ap.rst
index f57ae621f33e8..37e16158c7fbf 100644
--- a/Documentation/s390/vfio-ap.rst
+++ b/Documentation/s390/vfio-ap.rst
@@ -299,7 +299,7 @@ of the VFIO AP mediated matrix device driver::
    |  MDEV CORE  |
    |   MODULE    |
    |   mdev.ko   |
-   | +---------+ | mdev_register_device() +--------------+
+   | +---------+ | mdev_register_parent() +--------------+
    | |Physical | +<-----------------------+              |
    | | device  | |                        |  vfio_ap.ko  |<-> matrix
    | |interface| +----------------------->+              |    device
diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index 8aad08a8b8a50..ea928a3806f43 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -156,7 +156,7 @@ Below is a high Level block diagram::
  |  MDEV CORE  |
  |   MODULE    |
  |   mdev.ko   |
- | +---------+ | mdev_register_device() +--------------+
+ | +---------+ | mdev_register_parent() +--------------+
  | |Physical | +<-----------------------+              |
  | | device  | |                        |  vfio_ccw.ko |<-> subchannel
  | |interface| +----------------------->+              |     device
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index e2f6c56ab3420..37bda01c1bb97 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1960,7 +1960,7 @@ static void intel_gvt_clean_device(struct drm_i915_private *i915)
 	if (drm_WARN_ON(&i915->drm, !gvt))
 		return;
 
-	mdev_unregister_device(i915->drm.dev);
+	mdev_unregister_parent(&i915->vgpu.parent);
 	intel_gvt_cleanup_vgpu_type_groups(gvt);
 	intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu);
 	intel_gvt_clean_vgpu_types(gvt);
@@ -2065,7 +2065,8 @@ static int intel_gvt_init_device(struct drm_i915_private *i915)
 	if (ret)
 		goto out_destroy_idle_vgpu;
 
-	ret = mdev_register_device(i915->drm.dev, &intel_vgpu_mdev_driver);
+	ret = mdev_register_parent(&i915->vgpu.parent, i915->drm.dev,
+				   &intel_vgpu_mdev_driver);
 	if (ret)
 		goto out_cleanup_vgpu_type_groups;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 00d7eeae33bd3..7d62cc9a57e4e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -32,6 +32,7 @@
 
 #include <uapi/drm/i915_drm.h>
 
+#include <linux/mdev.h>
 #include <linux/pm_qos.h>
 
 #include <drm/drm_connector.h>
@@ -400,6 +401,7 @@ struct i915_frontbuffer_tracking {
 
 struct i915_virtual_gpu {
 	struct mutex lock; /* serialises sending of g2v_notify command pkts */
+	struct mdev_parent parent;
 	bool active;
 	u32 caps;
 	u32 *initial_mmio;
diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
index 1cb9daf9c6450..4ae462da7b45b 100644
--- a/drivers/s390/cio/cio.h
+++ b/drivers/s390/cio/cio.h
@@ -5,6 +5,7 @@
 #include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
+#include <linux/mdev.h>
 #include <asm/chpid.h>
 #include <asm/cio.h>
 #include <asm/fcx.h>
@@ -104,6 +105,7 @@ struct subchannel {
 	struct schib_config config;
 	u64 dma_mask;
 	char *driver_override; /* Driver name to force a match */
+	struct mdev_parent parent;
 } __attribute__ ((aligned(8)));
 
 DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index b49e2e9db2dc6..9192a21085ce4 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -11,7 +11,6 @@
  */
 
 #include <linux/vfio.h>
-#include <linux/mdev.h>
 #include <linux/nospec.h>
 #include <linux/slab.h>
 
@@ -660,10 +659,11 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 
 int vfio_ccw_mdev_reg(struct subchannel *sch)
 {
-	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
+	return mdev_register_parent(&sch->parent, &sch->dev,
+				    &vfio_ccw_mdev_driver);
 }
 
 void vfio_ccw_mdev_unreg(struct subchannel *sch)
 {
-	mdev_unregister_device(&sch->dev);
+	mdev_unregister_parent(&sch->parent);
 }
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a7d2a95796d36..834945150dc9f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1485,7 +1485,8 @@ int vfio_ap_mdev_register(void)
 	if (ret)
 		return ret;
 
-	ret = mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_driver);
+	ret = mdev_register_parent(&matrix_dev->parent, &matrix_dev->device,
+				   &vfio_ap_matrix_driver);
 	if (ret)
 		goto err_driver;
 	return 0;
@@ -1497,6 +1498,6 @@ int vfio_ap_mdev_register(void)
 
 void vfio_ap_mdev_unregister(void)
 {
-	mdev_unregister_device(&matrix_dev->device);
+	mdev_unregister_parent(&matrix_dev->parent);
 	mdev_unregister_driver(&vfio_ap_matrix_driver);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index a26efd804d0df..b97dc548a7c95 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -46,6 +46,7 @@ struct ap_matrix_dev {
 	struct list_head mdev_list;
 	struct mutex lock;
 	struct ap_driver  *vfio_ap_drv;
+	struct mdev_parent parent;
 };
 
 extern struct ap_matrix_dev *matrix_dev;
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b8b9e7911e559..8b5a2b3f0c882 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -20,8 +20,6 @@
 #define DRIVER_AUTHOR		"NVIDIA Corporation"
 #define DRIVER_DESC		"Mediated device Core Driver"
 
-static LIST_HEAD(parent_list);
-static DEFINE_MUTEX(parent_list_lock);
 static struct class_compat *mdev_bus_compat_class;
 
 static LIST_HEAD(mdev_list);
@@ -63,28 +61,6 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype)
 }
 EXPORT_SYMBOL(mtype_get_parent_dev);
 
-/* Should be called holding parent_list_lock */
-static struct mdev_parent *__find_parent_device(struct device *dev)
-{
-	struct mdev_parent *parent;
-
-	list_for_each_entry(parent, &parent_list, next) {
-		if (parent->dev == dev)
-			return parent;
-	}
-	return NULL;
-}
-
-void mdev_release_parent(struct kref *kref)
-{
-	struct mdev_parent *parent = container_of(kref, struct mdev_parent,
-						  ref);
-	struct device *dev = parent->dev;
-
-	kfree(parent);
-	put_device(dev);
-}
-
 /* Caller must hold parent unreg_sem read or write lock */
 static void mdev_device_remove_common(struct mdev_device *mdev)
 {
@@ -107,125 +83,69 @@ static int mdev_device_remove_cb(struct device *dev, void *data)
 }
 
 /*
- * mdev_register_device : Register a device
+ * mdev_register_parent: Register a device as parent for mdevs
+ * @parent: parent structure registered
  * @dev: device structure representing parent device.
  * @mdev_driver: Device driver to bind to the newly created mdev
  *
- * Add device to list of registered parent devices.
  * Returns a negative value on error, otherwise 0.
  */
-int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver)
+int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
+		struct mdev_driver *mdev_driver)
 {
-	int ret;
-	struct mdev_parent *parent;
 	char *env_string = "MDEV_STATE=registered";
 	char *envp[] = { env_string, NULL };
+	int ret;
 
 	/* check for mandatory ops */
 	if (!mdev_driver->supported_type_groups)
 		return -EINVAL;
 
-	dev = get_device(dev);
-	if (!dev)
-		return -EINVAL;
-
-	mutex_lock(&parent_list_lock);
-
-	/* Check for duplicate */
-	parent = __find_parent_device(dev);
-	if (parent) {
-		parent = NULL;
-		ret = -EEXIST;
-		goto add_dev_err;
-	}
-
-	parent = kzalloc(sizeof(*parent), GFP_KERNEL);
-	if (!parent) {
-		ret = -ENOMEM;
-		goto add_dev_err;
-	}
-
-	kref_init(&parent->ref);
+	memset(parent, 0, sizeof(*parent));
 	init_rwsem(&parent->unreg_sem);
-
 	parent->dev = dev;
 	parent->mdev_driver = mdev_driver;
 
 	if (!mdev_bus_compat_class) {
 		mdev_bus_compat_class = class_compat_register("mdev_bus");
-		if (!mdev_bus_compat_class) {
-			ret = -ENOMEM;
-			goto add_dev_err;
-		}
+		if (!mdev_bus_compat_class)
+			return -ENOMEM;
 	}
 
 	ret = parent_create_sysfs_files(parent);
 	if (ret)
-		goto add_dev_err;
+		return ret;
 
 	ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
 	if (ret)
 		dev_warn(dev, "Failed to create compatibility class link\n");
 
-	list_add(&parent->next, &parent_list);
-	mutex_unlock(&parent_list_lock);
-
 	dev_info(dev, "MDEV: Registered\n");
 	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
-
 	return 0;
-
-add_dev_err:
-	mutex_unlock(&parent_list_lock);
-	if (parent)
-		mdev_put_parent(parent);
-	else
-		put_device(dev);
-	return ret;
 }
-EXPORT_SYMBOL(mdev_register_device);
+EXPORT_SYMBOL(mdev_register_parent);
 
 /*
- * mdev_unregister_device : Unregister a parent device
- * @dev: device structure representing parent device.
- *
- * Remove device from list of registered parent devices. Give a chance to free
- * existing mediated devices for given device.
+ * mdev_unregister_parent : Unregister a parent device
+ * @parent: parent structure to unregister
  */
-
-void mdev_unregister_device(struct device *dev)
+void mdev_unregister_parent(struct mdev_parent *parent)
 {
-	struct mdev_parent *parent;
 	char *env_string = "MDEV_STATE=unregistered";
 	char *envp[] = { env_string, NULL };
 
-	mutex_lock(&parent_list_lock);
-	parent = __find_parent_device(dev);
-
-	if (!parent) {
-		mutex_unlock(&parent_list_lock);
-		return;
-	}
-	dev_info(dev, "MDEV: Unregistering\n");
-
-	list_del(&parent->next);
-	mutex_unlock(&parent_list_lock);
+	dev_info(parent->dev, "MDEV: Unregistering\n");
 
 	down_write(&parent->unreg_sem);
-
-	class_compat_remove_link(mdev_bus_compat_class, dev, NULL);
-
-	device_for_each_child(dev, NULL, mdev_device_remove_cb);
-
+	class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL);
+	device_for_each_child(parent->dev, NULL, mdev_device_remove_cb);
 	parent_remove_sysfs_files(parent);
 	up_write(&parent->unreg_sem);
 
-	mdev_put_parent(parent);
-
-	/* We still have the caller's reference to use for the uevent */
-	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+	kobject_uevent_env(&parent->dev->kobj, KOBJ_CHANGE, envp);
 }
-EXPORT_SYMBOL(mdev_unregister_device);
+EXPORT_SYMBOL(mdev_unregister_parent);
 
 static void mdev_device_release(struct device *dev)
 {
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 7c9fc79f3d838..297f911fdc890 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -13,17 +13,6 @@
 int  mdev_bus_register(void);
 void mdev_bus_unregister(void);
 
-struct mdev_parent {
-	struct device *dev;
-	struct mdev_driver *mdev_driver;
-	struct kref ref;
-	struct list_head next;
-	struct kset *mdev_types_kset;
-	struct list_head type_list;
-	/* Synchronize device creation/removal with parent unregistration */
-	struct rw_semaphore unreg_sem;
-};
-
 struct mdev_type {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -48,16 +37,4 @@ void mdev_remove_sysfs_files(struct mdev_device *mdev);
 int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid);
 int  mdev_device_remove(struct mdev_device *dev);
 
-void mdev_release_parent(struct kref *kref);
-
-static inline void mdev_get_parent(struct mdev_parent *parent)
-{
-	kref_get(&parent->ref);
-}
-
-static inline void mdev_put_parent(struct mdev_parent *parent)
-{
-	kref_put(&parent->ref, mdev_release_parent);
-}
-
 #endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 0ccfeb3dda245..becce814aabfb 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -83,7 +83,7 @@ static void mdev_type_release(struct kobject *kobj)
 
 	pr_debug("Releasing group %s\n", kobj->name);
 	/* Pairs with the get in add_mdev_supported_type() */
-	mdev_put_parent(type->parent);
+	put_device(type->parent->dev);
 	kfree(type);
 }
 
@@ -112,7 +112,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 	type->kobj.kset = parent->mdev_types_kset;
 	type->parent = parent;
 	/* Pairs with the put in mdev_type_release() */
-	mdev_get_parent(parent);
+	get_device(parent->dev);
 	type->type_group_id = type_group_id;
 
 	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 555c1d015b5f0..327ce3e5c6b5f 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -23,6 +23,16 @@ struct mdev_device {
 	bool active;
 };
 
+/* embedded into the struct device that the mdev devices hang off */
+struct mdev_parent {
+	struct device *dev;
+	struct mdev_driver *mdev_driver;
+	struct kset *mdev_types_kset;
+	struct list_head type_list;
+	/* Synchronize device creation/removal with parent unregistration */
+	struct rw_semaphore unreg_sem;
+};
+
 static inline struct mdev_device *to_mdev_device(struct device *dev)
 {
 	return container_of(dev, struct mdev_device, dev);
@@ -75,8 +85,9 @@ static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
 
 extern struct bus_type mdev_bus_type;
 
-int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver);
-void mdev_unregister_device(struct device *dev);
+int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
+		struct mdev_driver *mdev_driver);
+void mdev_unregister_parent(struct mdev_parent *parent);
 
 int mdev_register_driver(struct mdev_driver *drv);
 void mdev_unregister_driver(struct mdev_driver *drv);
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 344c2901a82bf..f0967a81eabe7 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -129,6 +129,7 @@ static dev_t		mbochs_devt;
 static struct class	*mbochs_class;
 static struct cdev	mbochs_cdev;
 static struct device	mbochs_dev;
+static struct mdev_parent mbochs_parent;
 static atomic_t mbochs_avail_mbytes;
 static const struct vfio_device_ops mbochs_dev_ops;
 
@@ -1457,7 +1458,7 @@ static int __init mbochs_dev_init(void)
 	if (ret)
 		goto err_class;
 
-	ret = mdev_register_device(&mbochs_dev, &mbochs_driver);
+	ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver);
 	if (ret)
 		goto err_device;
 
@@ -1478,7 +1479,7 @@ static int __init mbochs_dev_init(void)
 static void __exit mbochs_dev_exit(void)
 {
 	mbochs_dev.bus = NULL;
-	mdev_unregister_device(&mbochs_dev);
+	mdev_unregister_parent(&mbochs_parent);
 
 	device_unregister(&mbochs_dev);
 	mdev_unregister_driver(&mbochs_driver);
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index e8c46eb2e2468..8ab88a1d149cb 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -84,6 +84,7 @@ static dev_t		mdpy_devt;
 static struct class	*mdpy_class;
 static struct cdev	mdpy_cdev;
 static struct device	mdpy_dev;
+static struct mdev_parent mdpy_parent;
 static u32		mdpy_count;
 static const struct vfio_device_ops mdpy_dev_ops;
 
@@ -766,7 +767,7 @@ static int __init mdpy_dev_init(void)
 	if (ret)
 		goto err_class;
 
-	ret = mdev_register_device(&mdpy_dev, &mdpy_driver);
+	ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver);
 	if (ret)
 		goto err_device;
 
@@ -787,7 +788,7 @@ static int __init mdpy_dev_init(void)
 static void __exit mdpy_dev_exit(void)
 {
 	mdpy_dev.bus = NULL;
-	mdev_unregister_device(&mdpy_dev);
+	mdev_unregister_parent(&mdpy_parent);
 
 	device_unregister(&mdpy_dev);
 	mdev_unregister_driver(&mdpy_driver);
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index f42a59ed2e3fe..20136def93fdb 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -74,6 +74,7 @@ static struct mtty_dev {
 	struct cdev	vd_cdev;
 	struct idr	vd_idr;
 	struct device	dev;
+	struct mdev_parent parent;
 } mtty_dev;
 
 struct mdev_region_info {
@@ -1352,7 +1353,8 @@ static int __init mtty_dev_init(void)
 	if (ret)
 		goto err_class;
 
-	ret = mdev_register_device(&mtty_dev.dev, &mtty_driver);
+	ret = mdev_register_parent(&mtty_dev.parent, &mtty_dev.dev,
+				   &mtty_driver);
 	if (ret)
 		goto err_device;
 	return 0;
@@ -1372,7 +1374,7 @@ static int __init mtty_dev_init(void)
 static void __exit mtty_dev_exit(void)
 {
 	mtty_dev.dev.bus = NULL;
-	mdev_unregister_device(&mtty_dev.dev);
+	mdev_unregister_parent(&mtty_dev.parent);
 
 	device_unregister(&mtty_dev.dev);
 	idr_destroy(&mtty_dev.vd_idr);
-- 
2.30.2

Re: [PATCH 2/8] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Kirti Wankhede]

On 6/3/2022 12:03 PM, Christoph Hellwig wrote:
> Simplify mdev_{un}register_device by requiring the caller to pass in
> a structure allocate as part of the parent device structure.  This
> removes the need for a list of parents and the separate mdev_parent
> refcount as we can simplify rely on the reference to the parent device.
> 

By removing this list, there is no way to know if parent device is 
registered repeatedly? What will happen if same parent device is 
registered twice? will it fail somewhere else?

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   .../driver-api/vfio-mediated-device.rst       |  12 +-
>   Documentation/s390/vfio-ap.rst                |   2 +-
>   Documentation/s390/vfio-ccw.rst               |   2 +-
>   drivers/gpu/drm/i915/gvt/kvmgt.c              |   5 +-
>   drivers/gpu/drm/i915/i915_drv.h               |   2 +
>   drivers/s390/cio/cio.h                        |   2 +
>   drivers/s390/cio/vfio_ccw_ops.c               |   6 +-
>   drivers/s390/crypto/vfio_ap_ops.c             |   5 +-
>   drivers/s390/crypto/vfio_ap_private.h         |   1 +
>   drivers/vfio/mdev/mdev_core.c                 | 116 +++---------------
>   drivers/vfio/mdev/mdev_private.h              |  23 ----
>   drivers/vfio/mdev/mdev_sysfs.c                |   4 +-
>   include/linux/mdev.h                          |  15 ++-
>   samples/vfio-mdev/mbochs.c                    |   5 +-
>   samples/vfio-mdev/mdpy.c                      |   5 +-
>   samples/vfio-mdev/mtty.c                      |   6 +-
>   16 files changed, 65 insertions(+), 146 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index eded8719180fb..3749f59c855fa 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -60,19 +60,19 @@ devices as examples, as these devices are the first devices to use this module::
>        |  MDEV CORE    |
>        |   MODULE      |
>        |   mdev.ko     |
> -     | +-----------+ |  mdev_register_device() +--------------+
> +     | +-----------+ |  mdev_register_parent() +--------------+
>        | |           | +<------------------------+              |
>        | |           | |                         |  nvidia.ko   |<-> physical
>        | |           | +------------------------>+              |    device
>        | |           | |        callbacks        +--------------+
>        | | Physical  | |
> -     | |  device   | |  mdev_register_device() +--------------+
> +     | |  device   | |  mdev_register_parent() +--------------+
>        | | interface | |<------------------------+              |
>        | |           | |                         |  i915.ko     |<-> physical
>        | |           | +------------------------>+              |    device
>        | |           | |        callbacks        +--------------+
>        | |           | |
> -     | |           | |  mdev_register_device() +--------------+
> +     | |           | |  mdev_register_parent() +--------------+
>        | |           | +<------------------------+              |
>        | |           | |                         | ccw_device.ko|<-> physical
>        | |           | +------------------------>+              |    device
> @@ -127,8 +127,8 @@ vfio_device_ops.
>   When a driver wants to add the GUID creation sysfs to an existing device it has
>   probe'd to then it should call::
>   
> -	extern int  mdev_register_device(struct device *dev,
> -	                                 struct mdev_driver *mdev_driver);
> +	int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
> +			struct mdev_driver *mdev_driver);
>

No need to change API name as it still registers 'struct device *dev', 
it could be 'mdev_register_device' but with new argument list.

>   This will provide the 'mdev_supported_types/XX/create' files which can then be
>   used to trigger the creation of a mdev_device. The created mdev_device will be
> @@ -136,7 +136,7 @@ attached to the specified driver.
>   
>   When the driver needs to remove itself it calls::
>   
> -	extern void mdev_unregister_device(struct device *dev);
> +	void mdev_unregister_parent(struct mdev_parent *parent);
>   

Same as above.

>   Which will unbind and destroy all the created mdevs and remove the sysfs files.
>   
> diff --git a/Documentation/s390/vfio-ap.rst b/Documentation/s390/vfio-ap.rst
> index f57ae621f33e8..37e16158c7fbf 100644
> --- a/Documentation/s390/vfio-ap.rst
> +++ b/Documentation/s390/vfio-ap.rst
> @@ -299,7 +299,7 @@ of the VFIO AP mediated matrix device driver::
>      |  MDEV CORE  |
>      |   MODULE    |
>      |   mdev.ko   |
> -   | +---------+ | mdev_register_device() +--------------+
> +   | +---------+ | mdev_register_parent() +--------------+
>      | |Physical | +<-----------------------+              |
>      | | device  | |                        |  vfio_ap.ko  |<-> matrix
>      | |interface| +----------------------->+              |    device
> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> index 8aad08a8b8a50..ea928a3806f43 100644
> --- a/Documentation/s390/vfio-ccw.rst
> +++ b/Documentation/s390/vfio-ccw.rst
> @@ -156,7 +156,7 @@ Below is a high Level block diagram::
>    |  MDEV CORE  |
>    |   MODULE    |
>    |   mdev.ko   |
> - | +---------+ | mdev_register_device() +--------------+
> + | +---------+ | mdev_register_parent() +--------------+
>    | |Physical | +<-----------------------+              |
>    | | device  | |                        |  vfio_ccw.ko |<-> subchannel
>    | |interface| +----------------------->+              |     device
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index e2f6c56ab3420..37bda01c1bb97 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1960,7 +1960,7 @@ static void intel_gvt_clean_device(struct drm_i915_private *i915)
>   	if (drm_WARN_ON(&i915->drm, !gvt))
>   		return;
>   
> -	mdev_unregister_device(i915->drm.dev);
> +	mdev_unregister_parent(&i915->vgpu.parent);

Ideally, parent should be member of gvt. There could be multiple vgpus 
created on one physical device. Intel folks would be better reviewer though.

>   	intel_gvt_cleanup_vgpu_type_groups(gvt);
>   	intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu);
>   	intel_gvt_clean_vgpu_types(gvt);
> @@ -2065,7 +2065,8 @@ static int intel_gvt_init_device(struct drm_i915_private *i915)
>   	if (ret)
>   		goto out_destroy_idle_vgpu;
>   
> -	ret = mdev_register_device(i915->drm.dev, &intel_vgpu_mdev_driver);
> +	ret = mdev_register_parent(&i915->vgpu.parent, i915->drm.dev,
> +				   &intel_vgpu_mdev_driver);
>   	if (ret)
>   		goto out_cleanup_vgpu_type_groups;
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 00d7eeae33bd3..7d62cc9a57e4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -32,6 +32,7 @@
>   
>   #include <uapi/drm/i915_drm.h>
>   
> +#include <linux/mdev.h>
>   #include <linux/pm_qos.h>
>   
>   #include <drm/drm_connector.h>
> @@ -400,6 +401,7 @@ struct i915_frontbuffer_tracking {
>   
>   struct i915_virtual_gpu {
>   	struct mutex lock; /* serialises sending of g2v_notify command pkts */
> +	struct mdev_parent parent;
>   	bool active;
>   	u32 caps;
>   	u32 *initial_mmio;
> diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
> index 1cb9daf9c6450..4ae462da7b45b 100644
> --- a/drivers/s390/cio/cio.h
> +++ b/drivers/s390/cio/cio.h
> @@ -5,6 +5,7 @@
>   #include <linux/mutex.h>
>   #include <linux/device.h>
>   #include <linux/mod_devicetable.h>
> +#include <linux/mdev.h>
>   #include <asm/chpid.h>
>   #include <asm/cio.h>
>   #include <asm/fcx.h>
> @@ -104,6 +105,7 @@ struct subchannel {
>   	struct schib_config config;
>   	u64 dma_mask;
>   	char *driver_override; /* Driver name to force a match */
> +	struct mdev_parent parent;
>   } __attribute__ ((aligned(8)));
>   
>   DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index b49e2e9db2dc6..9192a21085ce4 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -11,7 +11,6 @@
>    */
>   
>   #include <linux/vfio.h>
> -#include <linux/mdev.h>
>   #include <linux/nospec.h>
>   #include <linux/slab.h>
>   
> @@ -660,10 +659,11 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>   
>   int vfio_ccw_mdev_reg(struct subchannel *sch)
>   {
> -	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver);
> +	return mdev_register_parent(&sch->parent, &sch->dev,
> +				    &vfio_ccw_mdev_driver);
>   }
>   
>   void vfio_ccw_mdev_unreg(struct subchannel *sch)
>   {
> -	mdev_unregister_device(&sch->dev);
> +	mdev_unregister_parent(&sch->parent);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a7d2a95796d36..834945150dc9f 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1485,7 +1485,8 @@ int vfio_ap_mdev_register(void)
>   	if (ret)
>   		return ret;
>   
> -	ret = mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_driver);
> +	ret = mdev_register_parent(&matrix_dev->parent, &matrix_dev->device,
> +				   &vfio_ap_matrix_driver);
>   	if (ret)
>   		goto err_driver;
>   	return 0;
> @@ -1497,6 +1498,6 @@ int vfio_ap_mdev_register(void)
>   
>   void vfio_ap_mdev_unregister(void)
>   {
> -	mdev_unregister_device(&matrix_dev->device);
> +	mdev_unregister_parent(&matrix_dev->parent);
>   	mdev_unregister_driver(&vfio_ap_matrix_driver);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index a26efd804d0df..b97dc548a7c95 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -46,6 +46,7 @@ struct ap_matrix_dev {
>   	struct list_head mdev_list;
>   	struct mutex lock;
>   	struct ap_driver  *vfio_ap_drv;
> +	struct mdev_parent parent;
>   };
>   
>   extern struct ap_matrix_dev *matrix_dev;
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b8b9e7911e559..8b5a2b3f0c882 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -20,8 +20,6 @@
>   #define DRIVER_AUTHOR		"NVIDIA Corporation"
>   #define DRIVER_DESC		"Mediated device Core Driver"
>   
> -static LIST_HEAD(parent_list);
> -static DEFINE_MUTEX(parent_list_lock);
>   static struct class_compat *mdev_bus_compat_class;
>   
>   static LIST_HEAD(mdev_list);
> @@ -63,28 +61,6 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype)
>   }
>   EXPORT_SYMBOL(mtype_get_parent_dev);
>   
> -/* Should be called holding parent_list_lock */
> -static struct mdev_parent *__find_parent_device(struct device *dev)
> -{
> -	struct mdev_parent *parent;
> -
> -	list_for_each_entry(parent, &parent_list, next) {
> -		if (parent->dev == dev)
> -			return parent;
> -	}
> -	return NULL;
> -}
> -
> -void mdev_release_parent(struct kref *kref)
> -{
> -	struct mdev_parent *parent = container_of(kref, struct mdev_parent,
> -						  ref);
> -	struct device *dev = parent->dev;
> -
> -	kfree(parent);
> -	put_device(dev);
> -}
> -
>   /* Caller must hold parent unreg_sem read or write lock */
>   static void mdev_device_remove_common(struct mdev_device *mdev)
>   {
> @@ -107,125 +83,69 @@ static int mdev_device_remove_cb(struct device *dev, void *data)
>   }
>   
>   /*
> - * mdev_register_device : Register a device
> + * mdev_register_parent: Register a device as parent for mdevs
> + * @parent: parent structure registered
>    * @dev: device structure representing parent device.
>    * @mdev_driver: Device driver to bind to the newly created mdev
>    *
> - * Add device to list of registered parent devices.
>    * Returns a negative value on error, otherwise 0.
>    */
> -int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver)
> +int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
> +		struct mdev_driver *mdev_driver)
>   {
> -	int ret;
> -	struct mdev_parent *parent;
>   	char *env_string = "MDEV_STATE=registered";
>   	char *envp[] = { env_string, NULL };
> +	int ret;
>   
>   	/* check for mandatory ops */
>   	if (!mdev_driver->supported_type_groups)
>   		return -EINVAL;
>   
> -	dev = get_device(dev);
> -	if (!dev)
> -		return -EINVAL;
> -

Why not held device here? What if some driver miss behave where it 
registers device to mdev, but unloads without unregistering from mdev?

Thanks,
Kirti

> -	mutex_lock(&parent_list_lock);
> -
> -	/* Check for duplicate */
> -	parent = __find_parent_device(dev);
> -	if (parent) {
> -		parent = NULL;
> -		ret = -EEXIST;
> -		goto add_dev_err;
> -	}
> -
> -	parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> -	if (!parent) {
> -		ret = -ENOMEM;
> -		goto add_dev_err;
> -	}
> -
> -	kref_init(&parent->ref);
> +	memset(parent, 0, sizeof(*parent));
>   	init_rwsem(&parent->unreg_sem);
> -
>   	parent->dev = dev;
>   	parent->mdev_driver = mdev_driver;
>   
>   	if (!mdev_bus_compat_class) {
>   		mdev_bus_compat_class = class_compat_register("mdev_bus");
> -		if (!mdev_bus_compat_class) {
> -			ret = -ENOMEM;
> -			goto add_dev_err;
> -		}
> +		if (!mdev_bus_compat_class)
> +			return -ENOMEM;
>   	}
>   
>   	ret = parent_create_sysfs_files(parent);
>   	if (ret)
> -		goto add_dev_err;
> +		return ret;
>   
>   	ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
>   	if (ret)
>   		dev_warn(dev, "Failed to create compatibility class link\n");
>   
> -	list_add(&parent->next, &parent_list);
> -	mutex_unlock(&parent_list_lock);
> -
>   	dev_info(dev, "MDEV: Registered\n");
>   	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> -
>   	return 0;
> -
> -add_dev_err:
> -	mutex_unlock(&parent_list_lock);
> -	if (parent)
> -		mdev_put_parent(parent);
> -	else
> -		put_device(dev);
> -	return ret;
>   }
> -EXPORT_SYMBOL(mdev_register_device);
> +EXPORT_SYMBOL(mdev_register_parent);
>   
>   /*
> - * mdev_unregister_device : Unregister a parent device
> - * @dev: device structure representing parent device.
> - *
> - * Remove device from list of registered parent devices. Give a chance to free
> - * existing mediated devices for given device.
> + * mdev_unregister_parent : Unregister a parent device
> + * @parent: parent structure to unregister
>    */
> -
> -void mdev_unregister_device(struct device *dev)
> +void mdev_unregister_parent(struct mdev_parent *parent)
>   {
> -	struct mdev_parent *parent;
>   	char *env_string = "MDEV_STATE=unregistered";
>   	char *envp[] = { env_string, NULL };
>   
> -	mutex_lock(&parent_list_lock);
> -	parent = __find_parent_device(dev);
> -
> -	if (!parent) {
> -		mutex_unlock(&parent_list_lock);
> -		return;
> -	}
> -	dev_info(dev, "MDEV: Unregistering\n");
> -
> -	list_del(&parent->next);
> -	mutex_unlock(&parent_list_lock);
> +	dev_info(parent->dev, "MDEV: Unregistering\n");
>   
>   	down_write(&parent->unreg_sem);
> -
> -	class_compat_remove_link(mdev_bus_compat_class, dev, NULL);
> -
> -	device_for_each_child(dev, NULL, mdev_device_remove_cb);
> -
> +	class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL);
> +	device_for_each_child(parent->dev, NULL, mdev_device_remove_cb);
>   	parent_remove_sysfs_files(parent);
>   	up_write(&parent->unreg_sem);
>   
> -	mdev_put_parent(parent);
> -
> -	/* We still have the caller's reference to use for the uevent */
> -	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +	kobject_uevent_env(&parent->dev->kobj, KOBJ_CHANGE, envp);
>   }
> -EXPORT_SYMBOL(mdev_unregister_device);
> +EXPORT_SYMBOL(mdev_unregister_parent);
>   
>   static void mdev_device_release(struct device *dev)
>   {
> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
> index 7c9fc79f3d838..297f911fdc890 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -13,17 +13,6 @@
>   int  mdev_bus_register(void);
>   void mdev_bus_unregister(void);
>   
> -struct mdev_parent {
> -	struct device *dev;
> -	struct mdev_driver *mdev_driver;
> -	struct kref ref;
> -	struct list_head next;
> -	struct kset *mdev_types_kset;
> -	struct list_head type_list;
> -	/* Synchronize device creation/removal with parent unregistration */
> -	struct rw_semaphore unreg_sem;
> -};
> -
>   struct mdev_type {
>   	struct kobject kobj;
>   	struct kobject *devices_kobj;
> @@ -48,16 +37,4 @@ void mdev_remove_sysfs_files(struct mdev_device *mdev);
>   int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid);
>   int  mdev_device_remove(struct mdev_device *dev);
>   
> -void mdev_release_parent(struct kref *kref);
> -
> -static inline void mdev_get_parent(struct mdev_parent *parent)
> -{
> -	kref_get(&parent->ref);
> -}
> -
> -static inline void mdev_put_parent(struct mdev_parent *parent)
> -{
> -	kref_put(&parent->ref, mdev_release_parent);
> -}
> -
>   #endif /* MDEV_PRIVATE_H */
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 0ccfeb3dda245..becce814aabfb 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -83,7 +83,7 @@ static void mdev_type_release(struct kobject *kobj)
>   
>   	pr_debug("Releasing group %s\n", kobj->name);
>   	/* Pairs with the get in add_mdev_supported_type() */
> -	mdev_put_parent(type->parent);
> +	put_device(type->parent->dev);
>   	kfree(type);
>   }
>   
> @@ -112,7 +112,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   	type->kobj.kset = parent->mdev_types_kset;
>   	type->parent = parent;
>   	/* Pairs with the put in mdev_type_release() */
> -	mdev_get_parent(parent);
> +	get_device(parent->dev);
>   	type->type_group_id = type_group_id;
>   
>   	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 555c1d015b5f0..327ce3e5c6b5f 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -23,6 +23,16 @@ struct mdev_device {
>   	bool active;
>   };
>   
> +/* embedded into the struct device that the mdev devices hang off */
> +struct mdev_parent {
> +	struct device *dev;
> +	struct mdev_driver *mdev_driver;
> +	struct kset *mdev_types_kset;
> +	struct list_head type_list;
> +	/* Synchronize device creation/removal with parent unregistration */
> +	struct rw_semaphore unreg_sem;
> +};
> +
>   static inline struct mdev_device *to_mdev_device(struct device *dev)
>   {
>   	return container_of(dev, struct mdev_device, dev);
> @@ -75,8 +85,9 @@ static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
>   
>   extern struct bus_type mdev_bus_type;
>   
> -int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver);
> -void mdev_unregister_device(struct device *dev);
> +int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
> +		struct mdev_driver *mdev_driver);
> +void mdev_unregister_parent(struct mdev_parent *parent);
>   
>   int mdev_register_driver(struct mdev_driver *drv);
>   void mdev_unregister_driver(struct mdev_driver *drv);
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index 344c2901a82bf..f0967a81eabe7 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -129,6 +129,7 @@ static dev_t		mbochs_devt;
>   static struct class	*mbochs_class;
>   static struct cdev	mbochs_cdev;
>   static struct device	mbochs_dev;
> +static struct mdev_parent mbochs_parent;
>   static atomic_t mbochs_avail_mbytes;
>   static const struct vfio_device_ops mbochs_dev_ops;
>   
> @@ -1457,7 +1458,7 @@ static int __init mbochs_dev_init(void)
>   	if (ret)
>   		goto err_class;
>   
> -	ret = mdev_register_device(&mbochs_dev, &mbochs_driver);
> +	ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver);
>   	if (ret)
>   		goto err_device;
>   
> @@ -1478,7 +1479,7 @@ static int __init mbochs_dev_init(void)
>   static void __exit mbochs_dev_exit(void)
>   {
>   	mbochs_dev.bus = NULL;
> -	mdev_unregister_device(&mbochs_dev);
> +	mdev_unregister_parent(&mbochs_parent);
>   
>   	device_unregister(&mbochs_dev);
>   	mdev_unregister_driver(&mbochs_driver);
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index e8c46eb2e2468..8ab88a1d149cb 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -84,6 +84,7 @@ static dev_t		mdpy_devt;
>   static struct class	*mdpy_class;
>   static struct cdev	mdpy_cdev;
>   static struct device	mdpy_dev;
> +static struct mdev_parent mdpy_parent;
>   static u32		mdpy_count;
>   static const struct vfio_device_ops mdpy_dev_ops;
>   
> @@ -766,7 +767,7 @@ static int __init mdpy_dev_init(void)
>   	if (ret)
>   		goto err_class;
>   
> -	ret = mdev_register_device(&mdpy_dev, &mdpy_driver);
> +	ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver);
>   	if (ret)
>   		goto err_device;
>   
> @@ -787,7 +788,7 @@ static int __init mdpy_dev_init(void)
>   static void __exit mdpy_dev_exit(void)
>   {
>   	mdpy_dev.bus = NULL;
> -	mdev_unregister_device(&mdpy_dev);
> +	mdev_unregister_parent(&mdpy_parent);
>   
>   	device_unregister(&mdpy_dev);
>   	mdev_unregister_driver(&mdpy_driver);
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index f42a59ed2e3fe..20136def93fdb 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -74,6 +74,7 @@ static struct mtty_dev {
>   	struct cdev	vd_cdev;
>   	struct idr	vd_idr;
>   	struct device	dev;
> +	struct mdev_parent parent;
>   } mtty_dev;
>   
>   struct mdev_region_info {
> @@ -1352,7 +1353,8 @@ static int __init mtty_dev_init(void)
>   	if (ret)
>   		goto err_class;
>   
> -	ret = mdev_register_device(&mtty_dev.dev, &mtty_driver);
> +	ret = mdev_register_parent(&mtty_dev.parent, &mtty_dev.dev,
> +				   &mtty_driver);
>   	if (ret)
>   		goto err_device;
>   	return 0;
> @@ -1372,7 +1374,7 @@ static int __init mtty_dev_init(void)
>   static void __exit mtty_dev_exit(void)
>   {
>   	mtty_dev.dev.bus = NULL;
> -	mdev_unregister_device(&mtty_dev.dev);
> +	mdev_unregister_parent(&mtty_dev.parent);
>   
>   	device_unregister(&mtty_dev.dev);
>   	idr_destroy(&mtty_dev.vd_idr);

Re: [PATCH 2/8] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Christoph Hellwig]

On Tue, Jun 07, 2022 at 12:52:30AM +0530, Kirti Wankhede wrote:
> By removing this list, there is no way to know if parent device is 
> registered repeatedly? What will happen if same parent device is registered 
> twice? will it fail somewhere else?

The driver core will warn if you double register a device.

>>   probe'd to then it should call::
>>   -	extern int  mdev_register_device(struct device *dev,
>> -	                                 struct mdev_driver *mdev_driver);
>> +	int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
>> +			struct mdev_driver *mdev_driver);
>>
>
> No need to change API name as it still registers 'struct device *dev', it 
> could be 'mdev_register_device' but with new argument list.

I think the name name is a lot more clear, as device is really overused.
Especially as this is not a mdev_device, which are registered
elsewhere..

>>   -	mdev_unregister_device(i915->drm.dev);
>> +	mdev_unregister_parent(&i915->vgpu.parent);
>
> Ideally, parent should be member of gvt. There could be multiple vgpus 
> created on one physical device. Intel folks would be better reviewer 
> though.

i915->vgpu is not for a single VGPU, but all VGPU related core
support.

>> -	struct mdev_parent *parent;
>>   	char *env_string = "MDEV_STATE=registered";
>>   	char *envp[] = { env_string, NULL };
>> +	int ret;
>>     	/* check for mandatory ops */
>>   	if (!mdev_driver->supported_type_groups)
>>   		return -EINVAL;
>>   -	dev = get_device(dev);
>> -	if (!dev)
>> -		return -EINVAL;
>> -
>
> Why not held device here? What if some driver miss behave where it 
> registers device to mdev, but unloads without unregistering from mdev?

Then that driver is buggy.  We don't add extra reference to slightly
paper over buggy code elsewhere in the kernel either.

RE: [PATCH 2/8] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Tian, Kevin]

> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, June 7, 2022 1:49 PM
> 
> >>   -	mdev_unregister_device(i915->drm.dev);
> >> +	mdev_unregister_parent(&i915->vgpu.parent);
> >
> > Ideally, parent should be member of gvt. There could be multiple vgpus
> > created on one physical device. Intel folks would be better reviewer
> > though.
> 
> i915->vgpu is not for a single VGPU, but all VGPU related core
> support.
> 

Yes i915->gpu is per parent, but i915->gvt is a better fit. The former
is solely for mmio snapshot while the latter contains everything
actually related to vgpu core support.

apart from that,

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

[PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Christoph Hellwig]

Allocate the mdev_type structures in the caller and add them one-by one.
This allows the caller to use container_of to get at the containing
struture and thus significantly simplify the code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 .../driver-api/vfio-mediated-device.rst       |   1 -
 drivers/gpu/drm/i915/gvt/gvt.h                |   1 +
 drivers/gpu/drm/i915/gvt/kvmgt.c              | 113 +++++------------
 drivers/s390/cio/cio.h                        |   1 +
 drivers/s390/cio/vfio_ccw_ops.c               |  27 ++--
 drivers/s390/crypto/vfio_ap_ops.c             |  20 ++-
 drivers/s390/crypto/vfio_ap_private.h         |   1 +
 drivers/vfio/mdev/mdev_core.c                 |  33 +----
 drivers/vfio/mdev/mdev_private.h              |  11 --
 drivers/vfio/mdev/mdev_sysfs.c                | 117 +++---------------
 include/linux/mdev.h                          |  17 ++-
 samples/vfio-mdev/mbochs.c                    |  64 +++++-----
 samples/vfio-mdev/mdpy.c                      |  56 ++++-----
 samples/vfio-mdev/mtty.c                      |  71 ++++++-----
 14 files changed, 184 insertions(+), 349 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 3749f59c855fa..8ff463aa9e2be 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -105,7 +105,6 @@ structure to represent a mediated device's driver::
      struct mdev_driver {
 	     int  (*probe)  (struct mdev_device *dev);
 	     void (*remove) (struct mdev_device *dev);
-	     struct attribute_group **supported_type_groups;
 	     struct device_driver    driver;
      };
 
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index aee1a45da74bc..dc5183ab0175e 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -297,6 +297,7 @@ struct intel_gvt_firmware {
 
 #define NR_MAX_INTEL_VGPU_TYPES 20
 struct intel_vgpu_type {
+	struct mdev_type type;
 	char name[16];
 	unsigned int avail_instance;
 	unsigned int low_gm_size;
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 37bda01c1bb97..06b8acc6a995d 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -119,17 +119,10 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
 {
-	struct intel_vgpu_type *type;
-	unsigned int num = 0;
-	struct intel_gvt *gvt = kdev_to_i915(mtype_get_parent_dev(mtype))->gvt;
+	struct intel_vgpu_type *type =
+		container_of(mtype, struct intel_vgpu_type, type);
 
-	type = &gvt->types[mtype_get_type_group_id(mtype)];
-	if (!type)
-		num = 0;
-	else
-		num = type->avail_instance;
-
-	return sprintf(buf, "%u\n", num);
+	return sprintf(buf, "%u\n", type->avail_instance);
 }
 
 static ssize_t device_api_show(struct mdev_type *mtype,
@@ -141,12 +134,8 @@ static ssize_t device_api_show(struct mdev_type *mtype,
 static ssize_t description_show(struct mdev_type *mtype,
 				struct mdev_type_attribute *attr, char *buf)
 {
-	struct intel_vgpu_type *type;
-	struct intel_gvt *gvt = kdev_to_i915(mtype_get_parent_dev(mtype))->gvt;
-
-	type = &gvt->types[mtype_get_type_group_id(mtype)];
-	if (!type)
-		return 0;
+	struct intel_vgpu_type *type =
+		container_of(mtype, struct intel_vgpu_type, type);
 
 	return sprintf(buf, "low_gm_size: %dMB\nhigh_gm_size: %dMB\n"
 		       "fence: %d\nresolution: %s\n"
@@ -160,12 +149,8 @@ static ssize_t description_show(struct mdev_type *mtype,
 static ssize_t name_show(struct mdev_type *mtype,
 			 struct mdev_type_attribute *attr, char *buf)
 {
-	struct intel_vgpu_type *type;
-	struct intel_gvt *gvt = kdev_to_i915(mtype_get_parent_dev(mtype))->gvt;
-
-	type = &gvt->types[mtype_get_type_group_id(mtype)];
-	if (!type)
-		return 0;
+	struct intel_vgpu_type *type =
+		container_of(mtype, struct intel_vgpu_type, type);
 
 	return sprintf(buf, "%s\n", type->name);
 }
@@ -175,7 +160,7 @@ static MDEV_TYPE_ATTR_RO(device_api);
 static MDEV_TYPE_ATTR_RO(description);
 static MDEV_TYPE_ATTR_RO(name);
 
-static struct attribute *gvt_type_attrs[] = {
+static const struct attribute *gvt_type_attrs[] = {
 	&mdev_type_attr_available_instances.attr,
 	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_description.attr,
@@ -183,51 +168,6 @@ static struct attribute *gvt_type_attrs[] = {
 	NULL,
 };
 
-static struct attribute_group *gvt_vgpu_type_groups[] = {
-	[0 ... NR_MAX_INTEL_VGPU_TYPES - 1] = NULL,
-};
-
-static int intel_gvt_init_vgpu_type_groups(struct intel_gvt *gvt)
-{
-	int i, j;
-	struct intel_vgpu_type *type;
-	struct attribute_group *group;
-
-	for (i = 0; i < gvt->num_types; i++) {
-		type = &gvt->types[i];
-
-		group = kzalloc(sizeof(struct attribute_group), GFP_KERNEL);
-		if (!group)
-			goto unwind;
-
-		group->name = type->name;
-		group->attrs = gvt_type_attrs;
-		gvt_vgpu_type_groups[i] = group;
-	}
-
-	return 0;
-
-unwind:
-	for (j = 0; j < i; j++) {
-		group = gvt_vgpu_type_groups[j];
-		kfree(group);
-	}
-
-	return -ENOMEM;
-}
-
-static void intel_gvt_cleanup_vgpu_type_groups(struct intel_gvt *gvt)
-{
-	int i;
-	struct attribute_group *group;
-
-	for (i = 0; i < gvt->num_types; i++) {
-		group = gvt_vgpu_type_groups[i];
-		gvt_vgpu_type_groups[i] = NULL;
-		kfree(group);
-	}
-}
-
 static void gvt_unpin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn,
 		unsigned long size)
 {
@@ -1616,14 +1556,11 @@ static int intel_vgpu_probe(struct mdev_device *mdev)
 {
 	struct device *pdev = mdev_parent_dev(mdev);
 	struct intel_gvt *gvt = kdev_to_i915(pdev)->gvt;
-	struct intel_vgpu_type *type;
+	struct intel_vgpu_type *type =
+		container_of(mdev->type, struct intel_vgpu_type, type);
 	struct intel_vgpu *vgpu;
 	int ret;
 
-	type = &gvt->types[mdev_get_type_group_id(mdev)];
-	if (!type)
-		return -EINVAL;
-
 	vgpu = intel_gvt_create_vgpu(gvt, type);
 	if (IS_ERR(vgpu)) {
 		gvt_err("failed to create intel vgpu: %ld\n", PTR_ERR(vgpu));
@@ -1662,7 +1599,6 @@ static struct mdev_driver intel_vgpu_mdev_driver = {
 	},
 	.probe		= intel_vgpu_probe,
 	.remove		= intel_vgpu_remove,
-	.supported_type_groups	= gvt_vgpu_type_groups,
 };
 
 int intel_gvt_page_track_add(struct intel_vgpu *info, u64 gfn)
@@ -1956,12 +1892,14 @@ static int init_service_thread(struct intel_gvt *gvt)
 static void intel_gvt_clean_device(struct drm_i915_private *i915)
 {
 	struct intel_gvt *gvt = fetch_and_zero(&i915->gvt);
+	int i;
 
 	if (drm_WARN_ON(&i915->drm, !gvt))
 		return;
 
+	for (i = 0; i < gvt->num_types; i++)
+		mdev_type_remove(&gvt->types[i].type, gvt_type_attrs);
 	mdev_unregister_parent(&i915->vgpu.parent);
-	intel_gvt_cleanup_vgpu_type_groups(gvt);
 	intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu);
 	intel_gvt_clean_vgpu_types(gvt);
 
@@ -1994,6 +1932,7 @@ static int intel_gvt_init_device(struct drm_i915_private *i915)
 	struct intel_gvt *gvt;
 	struct intel_vgpu *vgpu;
 	int ret;
+	int i;
 
 	if (drm_WARN_ON(&i915->drm, i915->gvt))
 		return -EEXIST;
@@ -2061,20 +2000,30 @@ static int intel_gvt_init_device(struct drm_i915_private *i915)
 
 	intel_gvt_debugfs_init(gvt);
 
-	ret = intel_gvt_init_vgpu_type_groups(gvt);
-	if (ret)
-		goto out_destroy_idle_vgpu;
-
 	ret = mdev_register_parent(&i915->vgpu.parent, i915->drm.dev,
 				   &intel_vgpu_mdev_driver);
 	if (ret)
-		goto out_cleanup_vgpu_type_groups;
+		goto out_destroy_idle_vgpu;
+
+	for (i = 0; i < gvt->num_types; i++) {
+		struct intel_vgpu_type *type = &gvt->types[i];
+
+		ret = mdev_type_add(&i915->vgpu.parent, &type->type, type->name,
+				    gvt_type_attrs);
+		if (ret)
+			goto out_remove_type_groups;
+	}
 
 	gvt_dbg_core("gvt device initialization is done\n");
 	return 0;
 
-out_cleanup_vgpu_type_groups:
-	intel_gvt_cleanup_vgpu_type_groups(gvt);
+out_remove_type_groups:
+	while (--i >= 0) {
+		struct intel_vgpu_type *type = &gvt->types[i];
+
+		mdev_type_remove(&type->type, gvt_type_attrs);
+	}
+	mdev_unregister_parent(&i915->vgpu.parent);
 out_destroy_idle_vgpu:
 	intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu);
 	intel_gvt_debugfs_clean(gvt);
diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
index 4ae462da7b45b..fb0c283a975a7 100644
--- a/drivers/s390/cio/cio.h
+++ b/drivers/s390/cio/cio.h
@@ -106,6 +106,7 @@ struct subchannel {
 	u64 dma_mask;
 	char *driver_override; /* Driver name to force a match */
 	struct mdev_parent parent;
+	struct mdev_type mdev_type;
 } __attribute__ ((aligned(8)));
 
 DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9192a21085ce4..e219ea3c62fc6 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -95,23 +95,13 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(available_instances);
 
-static struct attribute *mdev_types_attrs[] = {
+static const struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
-static struct attribute_group mdev_type_group = {
-	.name  = "io",
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group *mdev_type_groups[] = {
-	&mdev_type_group,
-	NULL,
-};
-
 static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
@@ -654,16 +644,25 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 	},
 	.probe = vfio_ccw_mdev_probe,
 	.remove = vfio_ccw_mdev_remove,
-	.supported_type_groups  = mdev_type_groups,
 };
 
 int vfio_ccw_mdev_reg(struct subchannel *sch)
 {
-	return mdev_register_parent(&sch->parent, &sch->dev,
-				    &vfio_ccw_mdev_driver);
+	int ret;
+
+	ret = mdev_register_parent(&sch->parent, &sch->dev,
+				   &vfio_ccw_mdev_driver);
+	if (ret)
+		return ret;
+	ret = mdev_type_add(&sch->parent, &sch->mdev_type, "io",
+			    mdev_types_attrs);
+	if (ret)
+		mdev_unregister_parent(&sch->parent);
+	return ret;
 }
 
 void vfio_ccw_mdev_unreg(struct subchannel *sch)
 {
+	mdev_type_remove(&sch->mdev_type, mdev_types_attrs);
 	mdev_unregister_parent(&sch->parent);
 }
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 834945150dc9f..caaa4fcb00460 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -537,23 +537,13 @@ static ssize_t device_api_show(struct mdev_type *mtype,
 
 static MDEV_TYPE_ATTR_RO(device_api);
 
-static struct attribute *vfio_ap_mdev_type_attrs[] = {
+static const struct attribute *vfio_ap_mdev_type_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
-static struct attribute_group vfio_ap_mdev_hwvirt_type_group = {
-	.name = VFIO_AP_MDEV_TYPE_HWVIRT,
-	.attrs = vfio_ap_mdev_type_attrs,
-};
-
-static struct attribute_group *vfio_ap_mdev_type_groups[] = {
-	&vfio_ap_mdev_hwvirt_type_group,
-	NULL,
-};
-
 struct vfio_ap_queue_reserved {
 	unsigned long *apid;
 	unsigned long *apqi;
@@ -1472,7 +1462,6 @@ static struct mdev_driver vfio_ap_matrix_driver = {
 	},
 	.probe = vfio_ap_mdev_probe,
 	.remove = vfio_ap_mdev_remove,
-	.supported_type_groups = vfio_ap_mdev_type_groups,
 };
 
 int vfio_ap_mdev_register(void)
@@ -1489,8 +1478,14 @@ int vfio_ap_mdev_register(void)
 				   &vfio_ap_matrix_driver);
 	if (ret)
 		goto err_driver;
+	ret = mdev_type_add(&matrix_dev->parent, &matrix_dev->mdev_type,
+			    VFIO_AP_MDEV_TYPE_HWVIRT, vfio_ap_mdev_type_attrs);
+	if (ret)
+		goto err_parent;
 	return 0;
 
+err_parent:
+	mdev_unregister_parent(&matrix_dev->parent);
 err_driver:
 	mdev_unregister_driver(&vfio_ap_matrix_driver);
 	return ret;
@@ -1498,6 +1493,7 @@ int vfio_ap_mdev_register(void)
 
 void vfio_ap_mdev_unregister(void)
 {
+	mdev_type_remove(&matrix_dev->mdev_type, vfio_ap_mdev_type_attrs);
 	mdev_unregister_parent(&matrix_dev->parent);
 	mdev_unregister_driver(&vfio_ap_matrix_driver);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index b97dc548a7c95..0e9b576e63db5 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -47,6 +47,7 @@ struct ap_matrix_dev {
 	struct mutex lock;
 	struct ap_driver  *vfio_ap_drv;
 	struct mdev_parent parent;
+	struct mdev_type mdev_type;
 };
 
 extern struct ap_matrix_dev *matrix_dev;
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 8b5a2b3f0c882..ff38c9549a55e 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -31,26 +31,6 @@ struct device *mdev_parent_dev(struct mdev_device *mdev)
 }
 EXPORT_SYMBOL(mdev_parent_dev);
 
-/*
- * Return the index in supported_type_groups that this mdev_device was created
- * from.
- */
-unsigned int mdev_get_type_group_id(struct mdev_device *mdev)
-{
-	return mdev->type->type_group_id;
-}
-EXPORT_SYMBOL(mdev_get_type_group_id);
-
-/*
- * Used in mdev_type_attribute sysfs functions to return the index in the
- * supported_type_groups that the sysfs is called from.
- */
-unsigned int mtype_get_type_group_id(struct mdev_type *mtype)
-{
-	return mtype->type_group_id;
-}
-EXPORT_SYMBOL(mtype_get_type_group_id);
-
 /*
  * Used in mdev_type_attribute sysfs functions to return the parent struct
  * device
@@ -97,10 +77,6 @@ int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
 	char *envp[] = { env_string, NULL };
 	int ret;
 
-	/* check for mandatory ops */
-	if (!mdev_driver->supported_type_groups)
-		return -EINVAL;
-
 	memset(parent, 0, sizeof(*parent));
 	init_rwsem(&parent->unreg_sem);
 	parent->dev = dev;
@@ -112,9 +88,10 @@ int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
 			return -ENOMEM;
 	}
 
-	ret = parent_create_sysfs_files(parent);
-	if (ret)
-		return ret;
+	parent->mdev_types_kset = kset_create_and_add("mdev_supported_types",
+					       NULL, &parent->dev->kobj);
+	if (!parent->mdev_types_kset)
+		return -ENOMEM;
 
 	ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
 	if (ret)
@@ -140,7 +117,7 @@ void mdev_unregister_parent(struct mdev_parent *parent)
 	down_write(&parent->unreg_sem);
 	class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL);
 	device_for_each_child(parent->dev, NULL, mdev_device_remove_cb);
-	parent_remove_sysfs_files(parent);
+	kset_unregister(parent->mdev_types_kset);
 	up_write(&parent->unreg_sem);
 
 	kobject_uevent_env(&parent->dev->kobj, KOBJ_CHANGE, envp);
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 297f911fdc890..476cc0379ede0 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -13,14 +13,6 @@
 int  mdev_bus_register(void);
 void mdev_bus_unregister(void);
 
-struct mdev_type {
-	struct kobject kobj;
-	struct kobject *devices_kobj;
-	struct mdev_parent *parent;
-	struct list_head next;
-	unsigned int type_group_id;
-};
-
 extern const struct attribute_group *mdev_device_groups[];
 
 #define to_mdev_type_attr(_attr)	\
@@ -28,9 +20,6 @@ extern const struct attribute_group *mdev_device_groups[];
 #define to_mdev_type(_kobj)		\
 	container_of(_kobj, struct mdev_type, kobj)
 
-int  parent_create_sysfs_files(struct mdev_parent *parent);
-void parent_remove_sysfs_files(struct mdev_parent *parent);
-
 int  mdev_create_sysfs_files(struct mdev_device *mdev);
 void mdev_remove_sysfs_files(struct mdev_device *mdev);
 
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index becce814aabfb..fb058755d85b8 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -82,8 +82,6 @@ static void mdev_type_release(struct kobject *kobj)
 	struct mdev_type *type = to_mdev_type(kobj);
 
 	pr_debug("Releasing group %s\n", kobj->name);
-	/* Pairs with the get in add_mdev_supported_type() */
-	put_device(type->parent->dev);
 	kfree(type);
 }
 
@@ -92,36 +90,19 @@ static struct kobj_type mdev_type_ktype = {
 	.release = mdev_type_release,
 };
 
-static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
-						 unsigned int type_group_id)
+
+int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type,
+		const char *name, const struct attribute * const *attrs)
 {
-	struct mdev_type *type;
-	struct attribute_group *group =
-		parent->mdev_driver->supported_type_groups[type_group_id];
 	int ret;
 
-	if (!group->name) {
-		pr_err("%s: Type name empty!\n", __func__);
-		return ERR_PTR(-EINVAL);
-	}
-
-	type = kzalloc(sizeof(*type), GFP_KERNEL);
-	if (!type)
-		return ERR_PTR(-ENOMEM);
-
-	type->kobj.kset = parent->mdev_types_kset;
+	memset(type, 0, sizeof(*type));
 	type->parent = parent;
-	/* Pairs with the put in mdev_type_release() */
-	get_device(parent->dev);
-	type->type_group_id = type_group_id;
-
-	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
-				   "%s-%s", dev_driver_string(parent->dev),
-				   group->name);
-	if (ret) {
-		kobject_put(&type->kobj);
-		return ERR_PTR(ret);
-	}
+	type->kobj.kset = parent->mdev_types_kset;
+	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, "%s-%s",
+				   dev_driver_string(parent->dev), name);
+	if (ret)
+		return ret;
 
 	ret = sysfs_create_file(&type->kobj, &mdev_type_attr_create.attr);
 	if (ret)
@@ -133,13 +114,10 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 		goto attr_devices_failed;
 	}
 
-	ret = sysfs_create_files(&type->kobj,
-				 (const struct attribute **)group->attrs);
-	if (ret) {
-		ret = -ENOMEM;
+	ret = sysfs_create_files(&type->kobj, attrs);
+	if (ret)
 		goto attrs_failed;
-	}
-	return type;
+	return 0;
 
 attrs_failed:
 	kobject_put(type->devices_kobj);
@@ -148,79 +126,20 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 attr_create_failed:
 	kobject_del(&type->kobj);
 	kobject_put(&type->kobj);
-	return ERR_PTR(ret);
+	return ret;
 }
+EXPORT_SYMBOL_GPL(mdev_type_add);
 
-static void remove_mdev_supported_type(struct mdev_type *type)
+void mdev_type_remove(struct mdev_type *type,
+		const struct attribute * const *attrs)
 {
-	struct attribute_group *group =
-		type->parent->mdev_driver->supported_type_groups[type->type_group_id];
-
-	sysfs_remove_files(&type->kobj,
-			   (const struct attribute **)group->attrs);
+	sysfs_remove_files(&type->kobj, attrs);
 	kobject_put(type->devices_kobj);
 	sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
 	kobject_del(&type->kobj);
 	kobject_put(&type->kobj);
 }
-
-static int add_mdev_supported_type_groups(struct mdev_parent *parent)
-{
-	int i;
-
-	for (i = 0; parent->mdev_driver->supported_type_groups[i]; i++) {
-		struct mdev_type *type;
-
-		type = add_mdev_supported_type(parent, i);
-		if (IS_ERR(type)) {
-			struct mdev_type *ltype, *tmp;
-
-			list_for_each_entry_safe(ltype, tmp, &parent->type_list,
-						  next) {
-				list_del(&ltype->next);
-				remove_mdev_supported_type(ltype);
-			}
-			return PTR_ERR(type);
-		}
-		list_add(&type->next, &parent->type_list);
-	}
-	return 0;
-}
-
-/* mdev sysfs functions */
-void parent_remove_sysfs_files(struct mdev_parent *parent)
-{
-	struct mdev_type *type, *tmp;
-
-	list_for_each_entry_safe(type, tmp, &parent->type_list, next) {
-		list_del(&type->next);
-		remove_mdev_supported_type(type);
-	}
-
-	kset_unregister(parent->mdev_types_kset);
-}
-
-int parent_create_sysfs_files(struct mdev_parent *parent)
-{
-	int ret;
-
-	parent->mdev_types_kset = kset_create_and_add("mdev_supported_types",
-					       NULL, &parent->dev->kobj);
-
-	if (!parent->mdev_types_kset)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(&parent->type_list);
-
-	ret = add_mdev_supported_type_groups(parent);
-	if (ret)
-		goto create_err;
-	return 0;
-
-create_err:
-	kset_unregister(parent->mdev_types_kset);
-	return ret;
-}
+EXPORT_SYMBOL_GPL(mdev_type_remove);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 327ce3e5c6b5f..cbb53dcd20d9d 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -23,12 +23,17 @@ struct mdev_device {
 	bool active;
 };
 
+struct mdev_type {
+	struct kobject kobj;
+	struct kobject *devices_kobj;
+	struct mdev_parent *parent;
+};
+
 /* embedded into the struct device that the mdev devices hang off */
 struct mdev_parent {
 	struct device *dev;
 	struct mdev_driver *mdev_driver;
 	struct kset *mdev_types_kset;
-	struct list_head type_list;
 	/* Synchronize device creation/removal with parent unregistration */
 	struct rw_semaphore unreg_sem;
 };
@@ -38,8 +43,11 @@ static inline struct mdev_device *to_mdev_device(struct device *dev)
 	return container_of(dev, struct mdev_device, dev);
 }
 
-unsigned int mdev_get_type_group_id(struct mdev_device *mdev);
-unsigned int mtype_get_type_group_id(struct mdev_type *mtype);
+int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type,
+		const char *name, const struct attribute * const *attrs);
+void mdev_type_remove(struct mdev_type *type,
+		const struct attribute * const *attrs);
+
 struct device *mtype_get_parent_dev(struct mdev_type *mtype);
 
 /* interface for exporting mdev supported type attributes */
@@ -66,15 +74,12 @@ struct mdev_type_attribute mdev_type_attr_##_name =		\
  * struct mdev_driver - Mediated device driver
  * @probe: called when new device created
  * @remove: called when device removed
- * @supported_type_groups: Attributes to define supported types. It is mandatory
- *			to provide supported types.
  * @driver: device driver structure
  *
  **/
 struct mdev_driver {
 	int (*probe)(struct mdev_device *dev);
 	void (*remove)(struct mdev_device *dev);
-	struct attribute_group **supported_type_groups;
 	struct device_driver driver;
 };
 
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index f0967a81eabe7..32fd6f385fafd 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -100,23 +100,28 @@ MODULE_PARM_DESC(mem, "megabytes available to " MBOCHS_NAME " devices");
 #define MBOCHS_TYPE_2 "medium"
 #define MBOCHS_TYPE_3 "large"
 
-static const struct mbochs_type {
+static struct mbochs_type {
+	struct mdev_type type;
+	const char *fname;
 	const char *name;
 	u32 mbytes;
 	u32 max_x;
 	u32 max_y;
 } mbochs_types[] = {
 	{
+		.fname	= MBOCHS_TYPE_1,
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_1,
 		.mbytes = 4,
 		.max_x  = 800,
 		.max_y  = 600,
 	}, {
+		.fname	= MBOCHS_TYPE_2,
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_2,
 		.mbytes = 16,
 		.max_x  = 1920,
 		.max_y  = 1440,
 	}, {
+		.fname	= MBOCHS_TYPE_3,
 		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_3,
 		.mbytes = 64,
 		.max_x  = 0,
@@ -509,8 +514,8 @@ static int mbochs_reset(struct mdev_state *mdev_state)
 static int mbochs_probe(struct mdev_device *mdev)
 {
 	int avail_mbytes = atomic_read(&mbochs_avail_mbytes);
-	const struct mbochs_type *type =
-		&mbochs_types[mdev_get_type_group_id(mdev)];
+	struct mbochs_type *type =
+		container_of(mdev->type, struct mbochs_type, type);
 	struct device *dev = mdev_dev(mdev);
 	struct mdev_state *mdev_state;
 	int ret = -ENOMEM;
@@ -1329,8 +1334,8 @@ static const struct attribute_group *mdev_dev_groups[] = {
 static ssize_t name_show(struct mdev_type *mtype,
 			 struct mdev_type_attribute *attr, char *buf)
 {
-	const struct mbochs_type *type =
-		&mbochs_types[mtype_get_type_group_id(mtype)];
+	struct mbochs_type *type =
+		container_of(mtype, struct mbochs_type, type);
 
 	return sprintf(buf, "%s\n", type->name);
 }
@@ -1339,8 +1344,8 @@ static MDEV_TYPE_ATTR_RO(name);
 static ssize_t description_show(struct mdev_type *mtype,
 				struct mdev_type_attribute *attr, char *buf)
 {
-	const struct mbochs_type *type =
-		&mbochs_types[mtype_get_type_group_id(mtype)];
+	struct mbochs_type *type =
+		container_of(mtype, struct mbochs_type, type);
 
 	return sprintf(buf, "virtual display, %d MB video memory\n",
 		       type ? type->mbytes  : 0);
@@ -1351,8 +1356,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
 {
-	const struct mbochs_type *type =
-		&mbochs_types[mtype_get_type_group_id(mtype)];
+	struct mbochs_type *type =
+		container_of(mtype, struct mbochs_type, type);
 	int count = atomic_read(&mbochs_avail_mbytes) / type->mbytes;
 
 	return sprintf(buf, "%d\n", count);
@@ -1366,7 +1371,7 @@ static ssize_t device_api_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(device_api);
 
-static struct attribute *mdev_types_attrs[] = {
+static const struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_description.attr,
 	&mdev_type_attr_device_api.attr,
@@ -1374,28 +1379,6 @@ static struct attribute *mdev_types_attrs[] = {
 	NULL,
 };
 
-static struct attribute_group mdev_type_group1 = {
-	.name  = MBOCHS_TYPE_1,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group mdev_type_group2 = {
-	.name  = MBOCHS_TYPE_2,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group mdev_type_group3 = {
-	.name  = MBOCHS_TYPE_3,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group *mdev_type_groups[] = {
-	&mdev_type_group1,
-	&mdev_type_group2,
-	&mdev_type_group3,
-	NULL,
-};
-
 static const struct vfio_device_ops mbochs_dev_ops = {
 	.close_device = mbochs_close_device,
 	.read = mbochs_read,
@@ -1413,7 +1396,6 @@ static struct mdev_driver mbochs_driver = {
 	},
 	.probe = mbochs_probe,
 	.remove	= mbochs_remove,
-	.supported_type_groups = mdev_type_groups,
 };
 
 static const struct file_operations vd_fops = {
@@ -1427,7 +1409,7 @@ static void mbochs_device_release(struct device *dev)
 
 static int __init mbochs_dev_init(void)
 {
-	int ret = 0;
+	int i, ret = 0;
 
 	atomic_set(&mbochs_avail_mbytes, max_mbytes);
 
@@ -1461,9 +1443,19 @@ static int __init mbochs_dev_init(void)
 	ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver);
 	if (ret)
 		goto err_device;
+	for (i = 0; i < ARRAY_SIZE(mbochs_types); i++) {
+		ret = mdev_type_add(&mbochs_parent, &mbochs_types[i].type,
+				    mbochs_types[i].fname, mdev_types_attrs);
+		if (ret)
+			goto err_types;
+	}
 
 	return 0;
 
+err_types:
+	while (--i >= 0)
+		mdev_type_remove(&mbochs_types[i].type, mdev_types_attrs);
+	mdev_unregister_parent(&mbochs_parent);
 err_device:
 	device_unregister(&mbochs_dev);
 err_class:
@@ -1478,7 +1470,11 @@ static int __init mbochs_dev_init(void)
 
 static void __exit mbochs_dev_exit(void)
 {
+	int i;
+
 	mbochs_dev.bus = NULL;
+	for (i = 0; i < ARRAY_SIZE(mbochs_types); i++)
+		mdev_type_remove(&mbochs_types[i].type, mdev_types_attrs);
 	mdev_unregister_parent(&mbochs_parent);
 
 	device_unregister(&mbochs_dev);
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index 8ab88a1d149cb..a6aa7f6f97095 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -52,7 +52,9 @@ MODULE_PARM_DESC(count, "number of " MDPY_NAME " devices");
 #define MDPY_TYPE_2 "xga"
 #define MDPY_TYPE_3 "hd"
 
-static const struct mdpy_type {
+static struct mdpy_type {
+	struct mdev_type type;
+	const char *fname;
 	const char *name;
 	u32 format;
 	u32 bytepp;
@@ -60,18 +62,21 @@ static const struct mdpy_type {
 	u32 height;
 } mdpy_types[] = {
 	{
+		.fname 	= MDPY_TYPE_1,
 		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_1,
 		.format = DRM_FORMAT_XRGB8888,
 		.bytepp = 4,
 		.width	= 640,
 		.height = 480,
 	}, {
+		.fname 	= MDPY_TYPE_2,
 		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_2,
 		.format = DRM_FORMAT_XRGB8888,
 		.bytepp = 4,
 		.width	= 1024,
 		.height = 768,
 	}, {
+		.fname 	= MDPY_TYPE_3,
 		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_3,
 		.format = DRM_FORMAT_XRGB8888,
 		.bytepp = 4,
@@ -220,7 +225,7 @@ static int mdpy_reset(struct mdev_state *mdev_state)
 static int mdpy_probe(struct mdev_device *mdev)
 {
 	const struct mdpy_type *type =
-		&mdpy_types[mdev_get_type_group_id(mdev)];
+		container_of(mdev->type, struct mdpy_type, type);
 	struct device *dev = mdev_dev(mdev);
 	struct mdev_state *mdev_state;
 	u32 fbsize;
@@ -645,8 +650,7 @@ static const struct attribute_group *mdev_dev_groups[] = {
 static ssize_t name_show(struct mdev_type *mtype,
 			 struct mdev_type_attribute *attr, char *buf)
 {
-	const struct mdpy_type *type =
-		&mdpy_types[mtype_get_type_group_id(mtype)];
+	struct mdpy_type *type = container_of(mtype, struct mdpy_type, type);
 
 	return sprintf(buf, "%s\n", type->name);
 }
@@ -655,8 +659,7 @@ static MDEV_TYPE_ATTR_RO(name);
 static ssize_t description_show(struct mdev_type *mtype,
 				struct mdev_type_attribute *attr, char *buf)
 {
-	const struct mdpy_type *type =
-		&mdpy_types[mtype_get_type_group_id(mtype)];
+	struct mdpy_type *type = container_of(mtype, struct mdpy_type, type);
 
 	return sprintf(buf, "virtual display, %dx%d framebuffer\n",
 		       type->width, type->height);
@@ -678,7 +681,7 @@ static ssize_t device_api_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(device_api);
 
-static struct attribute *mdev_types_attrs[] = {
+static const struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_description.attr,
 	&mdev_type_attr_device_api.attr,
@@ -686,28 +689,6 @@ static struct attribute *mdev_types_attrs[] = {
 	NULL,
 };
 
-static struct attribute_group mdev_type_group1 = {
-	.name  = MDPY_TYPE_1,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group mdev_type_group2 = {
-	.name  = MDPY_TYPE_2,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group mdev_type_group3 = {
-	.name  = MDPY_TYPE_3,
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group *mdev_type_groups[] = {
-	&mdev_type_group1,
-	&mdev_type_group2,
-	&mdev_type_group3,
-	NULL,
-};
-
 static const struct vfio_device_ops mdpy_dev_ops = {
 	.read = mdpy_read,
 	.write = mdpy_write,
@@ -724,7 +705,6 @@ static struct mdev_driver mdpy_driver = {
 	},
 	.probe = mdpy_probe,
 	.remove	= mdpy_remove,
-	.supported_type_groups = mdev_type_groups,
 };
 
 static const struct file_operations vd_fops = {
@@ -738,7 +718,7 @@ static void mdpy_device_release(struct device *dev)
 
 static int __init mdpy_dev_init(void)
 {
-	int ret = 0;
+	int i, ret = 0;
 
 	ret = alloc_chrdev_region(&mdpy_devt, 0, MINORMASK + 1, MDPY_NAME);
 	if (ret < 0) {
@@ -770,9 +750,19 @@ static int __init mdpy_dev_init(void)
 	ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver);
 	if (ret)
 		goto err_device;
+	for (i = 0; i < ARRAY_SIZE(mdpy_types); i++) {
+		ret = mdev_type_add(&mdpy_parent, &mdpy_types[i].type,
+				    mdpy_types[i].fname, mdev_types_attrs);
+		if (ret)
+			goto err_types;
+	}
 
 	return 0;
 
+err_types:
+	while (--i >= 0)
+		mdev_type_remove(&mdpy_types[i].type, mdev_types_attrs);
+	mdev_unregister_parent(&mdpy_parent);
 err_device:
 	device_unregister(&mdpy_dev);
 err_class:
@@ -787,7 +777,11 @@ static int __init mdpy_dev_init(void)
 
 static void __exit mdpy_dev_exit(void)
 {
+	int i;
+
 	mdpy_dev.bus = NULL;
+	for (i = 0; i < ARRAY_SIZE(mdpy_types); i++)
+		mdev_type_remove(&mdpy_types[i].type, mdev_types_attrs);
 	mdev_unregister_parent(&mdpy_parent);
 
 	device_unregister(&mdpy_dev);
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 20136def93fdb..28c0811cf2312 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -145,6 +145,16 @@ struct mdev_state {
 	int nr_ports;
 };
 
+static struct mtty_type {
+	struct mdev_type type;
+	int nr_ports;
+	const char *fname;
+	const char *name;
+} mtty_types[2] = {
+	{ .nr_ports = 1, .fname = "1", .name = "Single port serial" },
+	{ .nr_ports = 2, .fname = "2", .name = "Dual port serial" },
+};
+
 static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
 
 static const struct file_operations vd_fops = {
@@ -706,16 +716,18 @@ static ssize_t mdev_access(struct mdev_state *mdev_state, u8 *buf, size_t count,
 
 static int mtty_probe(struct mdev_device *mdev)
 {
+	struct mtty_type *type =
+		container_of(mdev->type, struct mtty_type, type);
 	struct mdev_state *mdev_state;
-	int nr_ports = mdev_get_type_group_id(mdev) + 1;
 	int avail_ports = atomic_read(&mdev_avail_ports);
 	int ret;
 
 	do {
-		if (avail_ports < nr_ports)
+		if (avail_ports < type->nr_ports)
 			return -ENOSPC;
 	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
-				     &avail_ports, avail_ports - nr_ports));
+				     &avail_ports,
+				     avail_ports - type->nr_ports));
 
 	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
 	if (mdev_state == NULL) {
@@ -725,13 +737,13 @@ static int mtty_probe(struct mdev_device *mdev)
 
 	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
 
-	mdev_state->nr_ports = nr_ports;
+	mdev_state->nr_ports = type->nr_ports;
 	mdev_state->irq_index = -1;
 	mdev_state->s[0].max_fifo_size = MAX_FIFO_SIZE;
 	mdev_state->s[1].max_fifo_size = MAX_FIFO_SIZE;
 	mutex_init(&mdev_state->rxtx_lock);
-	mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL);
 
+	mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL);
 	if (mdev_state->vconfig == NULL) {
 		ret = -ENOMEM;
 		goto err_state;
@@ -754,7 +766,7 @@ static int mtty_probe(struct mdev_device *mdev)
 	vfio_uninit_group_dev(&mdev_state->vdev);
 	kfree(mdev_state);
 err_nr_ports:
-	atomic_add(nr_ports, &mdev_avail_ports);
+	atomic_add(type->nr_ports, &mdev_avail_ports);
 	return ret;
 }
 
@@ -1235,11 +1247,9 @@ static const struct attribute_group *mdev_dev_groups[] = {
 static ssize_t name_show(struct mdev_type *mtype,
 			 struct mdev_type_attribute *attr, char *buf)
 {
-	static const char *name_str[2] = { "Single port serial",
-					   "Dual port serial" };
+	struct mtty_type *type = container_of(mtype, struct mtty_type, type);
 
-	return sysfs_emit(buf, "%s\n",
-			  name_str[mtype_get_type_group_id(mtype)]);
+	return sysfs_emit(buf, "%s\n", type->name);
 }
 
 static MDEV_TYPE_ATTR_RO(name);
@@ -1248,9 +1258,10 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
 {
-	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
+	struct mtty_type *type = container_of(mtype, struct mtty_type, type);
 
-	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
+	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) /
+			type->nr_ports);
 }
 
 static MDEV_TYPE_ATTR_RO(available_instances);
@@ -1263,29 +1274,13 @@ static ssize_t device_api_show(struct mdev_type *mtype,
 
 static MDEV_TYPE_ATTR_RO(device_api);
 
-static struct attribute *mdev_types_attrs[] = {
+static const struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
-static struct attribute_group mdev_type_group1 = {
-	.name  = "1",
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group mdev_type_group2 = {
-	.name  = "2",
-	.attrs = mdev_types_attrs,
-};
-
-static struct attribute_group *mdev_type_groups[] = {
-	&mdev_type_group1,
-	&mdev_type_group2,
-	NULL,
-};
-
 static const struct vfio_device_ops mtty_dev_ops = {
 	.name = "vfio-mtty",
 	.read = mtty_read,
@@ -1302,7 +1297,6 @@ static struct mdev_driver mtty_driver = {
 	},
 	.probe = mtty_probe,
 	.remove	= mtty_remove,
-	.supported_type_groups = mdev_type_groups,
 };
 
 static void mtty_device_release(struct device *dev)
@@ -1312,7 +1306,7 @@ static void mtty_device_release(struct device *dev)
 
 static int __init mtty_dev_init(void)
 {
-	int ret = 0;
+	int i, ret = 0;
 
 	pr_info("mtty_dev: %s\n", __func__);
 
@@ -1357,8 +1351,19 @@ static int __init mtty_dev_init(void)
 				   &mtty_driver);
 	if (ret)
 		goto err_device;
+	for (i = 0; i < ARRAY_SIZE(mtty_types); i++) {
+		ret = mdev_type_add(&mtty_dev.parent, &mtty_types[i].type,
+				    mtty_types[i].fname, mdev_types_attrs);
+		if (ret)
+			goto err_types;
+	}
+
 	return 0;
 
+err_types:
+	while (--i >= 0)
+		mdev_type_remove(&mtty_types[i].type, mdev_types_attrs);
+	mdev_unregister_parent(&mtty_dev.parent);
 err_device:
 	device_unregister(&mtty_dev.dev);
 err_class:
@@ -1373,7 +1378,11 @@ static int __init mtty_dev_init(void)
 
 static void __exit mtty_dev_exit(void)
 {
+	int i;
+
 	mtty_dev.dev.bus = NULL;
+	for (i = 0; i < ARRAY_SIZE(mtty_types); i++)
+		mdev_type_remove(&mtty_types[i].type, mdev_types_attrs);
 	mdev_unregister_parent(&mtty_dev.parent);
 
 	device_unregister(&mtty_dev.dev);
-- 
2.30.2

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Kirti Wankhede]

<snip...>

>   static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>   			    const char *buf, size_t count)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 327ce3e5c6b5f..cbb53dcd20d9d 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -23,12 +23,17 @@ struct mdev_device {
>   	bool active;
>   };
>   
> +struct mdev_type {
> +	struct kobject kobj;
> +	struct kobject *devices_kobj;
> +	struct mdev_parent *parent;
> +};
> +
>   /* embedded into the struct device that the mdev devices hang off */
>   struct mdev_parent {
>   	struct device *dev;
>   	struct mdev_driver *mdev_driver;
>   	struct kset *mdev_types_kset;
> -	struct list_head type_list;
>   	/* Synchronize device creation/removal with parent unregistration */
>   	struct rw_semaphore unreg_sem;
>   };
> @@ -38,8 +43,11 @@ static inline struct mdev_device *to_mdev_device(struct device *dev)
>   	return container_of(dev, struct mdev_device, dev);
>   }
>   
> -unsigned int mdev_get_type_group_id(struct mdev_device *mdev);
> -unsigned int mtype_get_type_group_id(struct mdev_type *mtype);
> +int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type,
> +		const char *name, const struct attribute * const *attrs);
> +void mdev_type_remove(struct mdev_type *type,
> +		const struct attribute * const *attrs);
> +
>   struct device *mtype_get_parent_dev(struct mdev_type *mtype);
>   
>   /* interface for exporting mdev supported type attributes */
> @@ -66,15 +74,12 @@ struct mdev_type_attribute mdev_type_attr_##_name =		\
>    * struct mdev_driver - Mediated device driver
>    * @probe: called when new device created
>    * @remove: called when device removed
> - * @supported_type_groups: Attributes to define supported types. It is mandatory
> - *			to provide supported types.
>    * @driver: device driver structure
>    *
>    **/
>   struct mdev_driver {
>   	int (*probe)(struct mdev_device *dev);
>   	void (*remove)(struct mdev_device *dev);
> -	struct attribute_group **supported_type_groups;
>   	struct device_driver driver;
>   };

mdev_type should be part of mdev_parent, separating it from mdev_parent 
could result in more errors while using mdev framework. Similarly it 
should be added as part of mdev_register_device. Below adding types is 
separated from mdev_register_device which is more error prone. What if 
driver registering to mdev doesn't add mdev_types? - mdev framework is 
un-usable in that case. We had kept it together with mdev registration 
so that mdev_types should be mandatory to be defined by driver during 
registration. How would you mandate mdev_type by such separation?

Thanks,
Kirti


>   
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index f0967a81eabe7..32fd6f385fafd 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -100,23 +100,28 @@ MODULE_PARM_DESC(mem, "megabytes available to " MBOCHS_NAME " devices");
>   #define MBOCHS_TYPE_2 "medium"
>   #define MBOCHS_TYPE_3 "large"
>   
> -static const struct mbochs_type {
> +static struct mbochs_type {
> +	struct mdev_type type;
> +	const char *fname;
>   	const char *name;
>   	u32 mbytes;
>   	u32 max_x;
>   	u32 max_y;
>   } mbochs_types[] = {
>   	{
> +		.fname	= MBOCHS_TYPE_1,
>   		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_1,
>   		.mbytes = 4,
>   		.max_x  = 800,
>   		.max_y  = 600,
>   	}, {
> +		.fname	= MBOCHS_TYPE_2,
>   		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_2,
>   		.mbytes = 16,
>   		.max_x  = 1920,
>   		.max_y  = 1440,
>   	}, {
> +		.fname	= MBOCHS_TYPE_3,
>   		.name	= MBOCHS_CLASS_NAME "-" MBOCHS_TYPE_3,
>   		.mbytes = 64,
>   		.max_x  = 0,
> @@ -509,8 +514,8 @@ static int mbochs_reset(struct mdev_state *mdev_state)
>   static int mbochs_probe(struct mdev_device *mdev)
>   {
>   	int avail_mbytes = atomic_read(&mbochs_avail_mbytes);
> -	const struct mbochs_type *type =
> -		&mbochs_types[mdev_get_type_group_id(mdev)];
> +	struct mbochs_type *type =
> +		container_of(mdev->type, struct mbochs_type, type);
>   	struct device *dev = mdev_dev(mdev);
>   	struct mdev_state *mdev_state;
>   	int ret = -ENOMEM;
> @@ -1329,8 +1334,8 @@ static const struct attribute_group *mdev_dev_groups[] = {
>   static ssize_t name_show(struct mdev_type *mtype,
>   			 struct mdev_type_attribute *attr, char *buf)
>   {
> -	const struct mbochs_type *type =
> -		&mbochs_types[mtype_get_type_group_id(mtype)];
> +	struct mbochs_type *type =
> +		container_of(mtype, struct mbochs_type, type);
>   
>   	return sprintf(buf, "%s\n", type->name);
>   }
> @@ -1339,8 +1344,8 @@ static MDEV_TYPE_ATTR_RO(name);
>   static ssize_t description_show(struct mdev_type *mtype,
>   				struct mdev_type_attribute *attr, char *buf)
>   {
> -	const struct mbochs_type *type =
> -		&mbochs_types[mtype_get_type_group_id(mtype)];
> +	struct mbochs_type *type =
> +		container_of(mtype, struct mbochs_type, type);
>   
>   	return sprintf(buf, "virtual display, %d MB video memory\n",
>   		       type ? type->mbytes  : 0);
> @@ -1351,8 +1356,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   					struct mdev_type_attribute *attr,
>   					char *buf)
>   {
> -	const struct mbochs_type *type =
> -		&mbochs_types[mtype_get_type_group_id(mtype)];
> +	struct mbochs_type *type =
> +		container_of(mtype, struct mbochs_type, type);
>   	int count = atomic_read(&mbochs_avail_mbytes) / type->mbytes;
>   
>   	return sprintf(buf, "%d\n", count);
> @@ -1366,7 +1371,7 @@ static ssize_t device_api_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(device_api);
>   
> -static struct attribute *mdev_types_attrs[] = {
> +static const struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
>   	&mdev_type_attr_device_api.attr,
> @@ -1374,28 +1379,6 @@ static struct attribute *mdev_types_attrs[] = {
>   	NULL,
>   };
>   
> -static struct attribute_group mdev_type_group1 = {
> -	.name  = MBOCHS_TYPE_1,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group mdev_type_group2 = {
> -	.name  = MBOCHS_TYPE_2,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group mdev_type_group3 = {
> -	.name  = MBOCHS_TYPE_3,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group *mdev_type_groups[] = {
> -	&mdev_type_group1,
> -	&mdev_type_group2,
> -	&mdev_type_group3,
> -	NULL,
> -};
> -
>   static const struct vfio_device_ops mbochs_dev_ops = {
>   	.close_device = mbochs_close_device,
>   	.read = mbochs_read,
> @@ -1413,7 +1396,6 @@ static struct mdev_driver mbochs_driver = {
>   	},
>   	.probe = mbochs_probe,
>   	.remove	= mbochs_remove,
> -	.supported_type_groups = mdev_type_groups,
>   };
>   
>   static const struct file_operations vd_fops = {
> @@ -1427,7 +1409,7 @@ static void mbochs_device_release(struct device *dev)
>   
>   static int __init mbochs_dev_init(void)
>   {
> -	int ret = 0;
> +	int i, ret = 0;
>   
>   	atomic_set(&mbochs_avail_mbytes, max_mbytes);
>   
> @@ -1461,9 +1443,19 @@ static int __init mbochs_dev_init(void)
>   	ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver);
>   	if (ret)
>   		goto err_device;
> +	for (i = 0; i < ARRAY_SIZE(mbochs_types); i++) {
> +		ret = mdev_type_add(&mbochs_parent, &mbochs_types[i].type,
> +				    mbochs_types[i].fname, mdev_types_attrs);
> +		if (ret)
> +			goto err_types;
> +	}
>   
>   	return 0;
>   
> +err_types:
> +	while (--i >= 0)
> +		mdev_type_remove(&mbochs_types[i].type, mdev_types_attrs);
> +	mdev_unregister_parent(&mbochs_parent);
>   err_device:
>   	device_unregister(&mbochs_dev);
>   err_class:
> @@ -1478,7 +1470,11 @@ static int __init mbochs_dev_init(void)
>   
>   static void __exit mbochs_dev_exit(void)
>   {
> +	int i;
> +
>   	mbochs_dev.bus = NULL;
> +	for (i = 0; i < ARRAY_SIZE(mbochs_types); i++)
> +		mdev_type_remove(&mbochs_types[i].type, mdev_types_attrs);
>   	mdev_unregister_parent(&mbochs_parent);
>   
>   	device_unregister(&mbochs_dev);
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index 8ab88a1d149cb..a6aa7f6f97095 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -52,7 +52,9 @@ MODULE_PARM_DESC(count, "number of " MDPY_NAME " devices");
>   #define MDPY_TYPE_2 "xga"
>   #define MDPY_TYPE_3 "hd"
>   
> -static const struct mdpy_type {
> +static struct mdpy_type {
> +	struct mdev_type type;
> +	const char *fname;
>   	const char *name;
>   	u32 format;
>   	u32 bytepp;
> @@ -60,18 +62,21 @@ static const struct mdpy_type {
>   	u32 height;
>   } mdpy_types[] = {
>   	{
> +		.fname 	= MDPY_TYPE_1,
>   		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_1,
>   		.format = DRM_FORMAT_XRGB8888,
>   		.bytepp = 4,
>   		.width	= 640,
>   		.height = 480,
>   	}, {
> +		.fname 	= MDPY_TYPE_2,
>   		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_2,
>   		.format = DRM_FORMAT_XRGB8888,
>   		.bytepp = 4,
>   		.width	= 1024,
>   		.height = 768,
>   	}, {
> +		.fname 	= MDPY_TYPE_3,
>   		.name	= MDPY_CLASS_NAME "-" MDPY_TYPE_3,
>   		.format = DRM_FORMAT_XRGB8888,
>   		.bytepp = 4,
> @@ -220,7 +225,7 @@ static int mdpy_reset(struct mdev_state *mdev_state)
>   static int mdpy_probe(struct mdev_device *mdev)
>   {
>   	const struct mdpy_type *type =
> -		&mdpy_types[mdev_get_type_group_id(mdev)];
> +		container_of(mdev->type, struct mdpy_type, type);
>   	struct device *dev = mdev_dev(mdev);
>   	struct mdev_state *mdev_state;
>   	u32 fbsize;
> @@ -645,8 +650,7 @@ static const struct attribute_group *mdev_dev_groups[] = {
>   static ssize_t name_show(struct mdev_type *mtype,
>   			 struct mdev_type_attribute *attr, char *buf)
>   {
> -	const struct mdpy_type *type =
> -		&mdpy_types[mtype_get_type_group_id(mtype)];
> +	struct mdpy_type *type = container_of(mtype, struct mdpy_type, type);
>   
>   	return sprintf(buf, "%s\n", type->name);
>   }
> @@ -655,8 +659,7 @@ static MDEV_TYPE_ATTR_RO(name);
>   static ssize_t description_show(struct mdev_type *mtype,
>   				struct mdev_type_attribute *attr, char *buf)
>   {
> -	const struct mdpy_type *type =
> -		&mdpy_types[mtype_get_type_group_id(mtype)];
> +	struct mdpy_type *type = container_of(mtype, struct mdpy_type, type);
>   
>   	return sprintf(buf, "virtual display, %dx%d framebuffer\n",
>   		       type->width, type->height);
> @@ -678,7 +681,7 @@ static ssize_t device_api_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(device_api);
>   
> -static struct attribute *mdev_types_attrs[] = {
> +static const struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
>   	&mdev_type_attr_device_api.attr,
> @@ -686,28 +689,6 @@ static struct attribute *mdev_types_attrs[] = {
>   	NULL,
>   };
>   
> -static struct attribute_group mdev_type_group1 = {
> -	.name  = MDPY_TYPE_1,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group mdev_type_group2 = {
> -	.name  = MDPY_TYPE_2,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group mdev_type_group3 = {
> -	.name  = MDPY_TYPE_3,
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group *mdev_type_groups[] = {
> -	&mdev_type_group1,
> -	&mdev_type_group2,
> -	&mdev_type_group3,
> -	NULL,
> -};
> -
>   static const struct vfio_device_ops mdpy_dev_ops = {
>   	.read = mdpy_read,
>   	.write = mdpy_write,
> @@ -724,7 +705,6 @@ static struct mdev_driver mdpy_driver = {
>   	},
>   	.probe = mdpy_probe,
>   	.remove	= mdpy_remove,
> -	.supported_type_groups = mdev_type_groups,
>   };
>   
>   static const struct file_operations vd_fops = {
> @@ -738,7 +718,7 @@ static void mdpy_device_release(struct device *dev)
>   
>   static int __init mdpy_dev_init(void)
>   {
> -	int ret = 0;
> +	int i, ret = 0;
>   
>   	ret = alloc_chrdev_region(&mdpy_devt, 0, MINORMASK + 1, MDPY_NAME);
>   	if (ret < 0) {
> @@ -770,9 +750,19 @@ static int __init mdpy_dev_init(void)
>   	ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver);
>   	if (ret)
>   		goto err_device;
> +	for (i = 0; i < ARRAY_SIZE(mdpy_types); i++) {
> +		ret = mdev_type_add(&mdpy_parent, &mdpy_types[i].type,
> +				    mdpy_types[i].fname, mdev_types_attrs);
> +		if (ret)
> +			goto err_types;
> +	}
>   
>   	return 0;
>   
> +err_types:
> +	while (--i >= 0)
> +		mdev_type_remove(&mdpy_types[i].type, mdev_types_attrs);
> +	mdev_unregister_parent(&mdpy_parent);
>   err_device:
>   	device_unregister(&mdpy_dev);
>   err_class:
> @@ -787,7 +777,11 @@ static int __init mdpy_dev_init(void)
>   
>   static void __exit mdpy_dev_exit(void)
>   {
> +	int i;
> +
>   	mdpy_dev.bus = NULL;
> +	for (i = 0; i < ARRAY_SIZE(mdpy_types); i++)
> +		mdev_type_remove(&mdpy_types[i].type, mdev_types_attrs);
>   	mdev_unregister_parent(&mdpy_parent);
>   
>   	device_unregister(&mdpy_dev);
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index 20136def93fdb..28c0811cf2312 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -145,6 +145,16 @@ struct mdev_state {
>   	int nr_ports;
>   };
>   
> +static struct mtty_type {
> +	struct mdev_type type;
> +	int nr_ports;
> +	const char *fname;
> +	const char *name;
> +} mtty_types[2] = {
> +	{ .nr_ports = 1, .fname = "1", .name = "Single port serial" },
> +	{ .nr_ports = 2, .fname = "2", .name = "Dual port serial" },
> +};
> +
>   static atomic_t mdev_avail_ports = ATOMIC_INIT(MAX_MTTYS);
>   
>   static const struct file_operations vd_fops = {
> @@ -706,16 +716,18 @@ static ssize_t mdev_access(struct mdev_state *mdev_state, u8 *buf, size_t count,
>   
>   static int mtty_probe(struct mdev_device *mdev)
>   {
> +	struct mtty_type *type =
> +		container_of(mdev->type, struct mtty_type, type);
>   	struct mdev_state *mdev_state;
> -	int nr_ports = mdev_get_type_group_id(mdev) + 1;
>   	int avail_ports = atomic_read(&mdev_avail_ports);
>   	int ret;
>   
>   	do {
> -		if (avail_ports < nr_ports)
> +		if (avail_ports < type->nr_ports)
>   			return -ENOSPC;
>   	} while (!atomic_try_cmpxchg(&mdev_avail_ports,
> -				     &avail_ports, avail_ports - nr_ports));
> +				     &avail_ports,
> +				     avail_ports - type->nr_ports));
>   
>   	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
>   	if (mdev_state == NULL) {
> @@ -725,13 +737,13 @@ static int mtty_probe(struct mdev_device *mdev)
>   
>   	vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops);
>   
> -	mdev_state->nr_ports = nr_ports;
> +	mdev_state->nr_ports = type->nr_ports;
>   	mdev_state->irq_index = -1;
>   	mdev_state->s[0].max_fifo_size = MAX_FIFO_SIZE;
>   	mdev_state->s[1].max_fifo_size = MAX_FIFO_SIZE;
>   	mutex_init(&mdev_state->rxtx_lock);
> -	mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL);
>   
> +	mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL);
>   	if (mdev_state->vconfig == NULL) {
>   		ret = -ENOMEM;
>   		goto err_state;
> @@ -754,7 +766,7 @@ static int mtty_probe(struct mdev_device *mdev)
>   	vfio_uninit_group_dev(&mdev_state->vdev);
>   	kfree(mdev_state);
>   err_nr_ports:
> -	atomic_add(nr_ports, &mdev_avail_ports);
> +	atomic_add(type->nr_ports, &mdev_avail_ports);
>   	return ret;
>   }
>   
> @@ -1235,11 +1247,9 @@ static const struct attribute_group *mdev_dev_groups[] = {
>   static ssize_t name_show(struct mdev_type *mtype,
>   			 struct mdev_type_attribute *attr, char *buf)
>   {
> -	static const char *name_str[2] = { "Single port serial",
> -					   "Dual port serial" };
> +	struct mtty_type *type = container_of(mtype, struct mtty_type, type);
>   
> -	return sysfs_emit(buf, "%s\n",
> -			  name_str[mtype_get_type_group_id(mtype)]);
> +	return sysfs_emit(buf, "%s\n", type->name);
>   }
>   
>   static MDEV_TYPE_ATTR_RO(name);
> @@ -1248,9 +1258,10 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   					struct mdev_type_attribute *attr,
>   					char *buf)
>   {
> -	unsigned int ports = mtype_get_type_group_id(mtype) + 1;
> +	struct mtty_type *type = container_of(mtype, struct mtty_type, type);
>   
> -	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) / ports);
> +	return sprintf(buf, "%d\n", atomic_read(&mdev_avail_ports) /
> +			type->nr_ports);
>   }
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
> @@ -1263,29 +1274,13 @@ static ssize_t device_api_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(device_api);
>   
> -static struct attribute *mdev_types_attrs[] = {
> +static const struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
>   
> -static struct attribute_group mdev_type_group1 = {
> -	.name  = "1",
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group mdev_type_group2 = {
> -	.name  = "2",
> -	.attrs = mdev_types_attrs,
> -};
> -
> -static struct attribute_group *mdev_type_groups[] = {
> -	&mdev_type_group1,
> -	&mdev_type_group2,
> -	NULL,
> -};
> -
>   static const struct vfio_device_ops mtty_dev_ops = {
>   	.name = "vfio-mtty",
>   	.read = mtty_read,
> @@ -1302,7 +1297,6 @@ static struct mdev_driver mtty_driver = {
>   	},
>   	.probe = mtty_probe,
>   	.remove	= mtty_remove,
> -	.supported_type_groups = mdev_type_groups,
>   };
>   
>   static void mtty_device_release(struct device *dev)
> @@ -1312,7 +1306,7 @@ static void mtty_device_release(struct device *dev)
>   
>   static int __init mtty_dev_init(void)
>   {
> -	int ret = 0;
> +	int i, ret = 0;
>   
>   	pr_info("mtty_dev: %s\n", __func__);
>   
> @@ -1357,8 +1351,19 @@ static int __init mtty_dev_init(void)
>   				   &mtty_driver);
>   	if (ret)
>   		goto err_device;
> +	for (i = 0; i < ARRAY_SIZE(mtty_types); i++) {
> +		ret = mdev_type_add(&mtty_dev.parent, &mtty_types[i].type,
> +				    mtty_types[i].fname, mdev_types_attrs);
> +		if (ret)
> +			goto err_types;
> +	}
> +
>   	return 0;
>   
> +err_types:
> +	while (--i >= 0)
> +		mdev_type_remove(&mtty_types[i].type, mdev_types_attrs);
> +	mdev_unregister_parent(&mtty_dev.parent);
>   err_device:
>   	device_unregister(&mtty_dev.dev);
>   err_class:
> @@ -1373,7 +1378,11 @@ static int __init mtty_dev_init(void)
>   
>   static void __exit mtty_dev_exit(void)
>   {
> +	int i;
> +
>   	mtty_dev.dev.bus = NULL;
> +	for (i = 0; i < ARRAY_SIZE(mtty_types); i++)
> +		mdev_type_remove(&mtty_types[i].type, mdev_types_attrs);
>   	mdev_unregister_parent(&mtty_dev.parent);
>   
>   	device_unregister(&mtty_dev.dev);

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Christoph Hellwig]

On Tue, Jun 07, 2022 at 12:52:49AM +0530, Kirti Wankhede wrote:
>>   	void (*remove)(struct mdev_device *dev);
>> -	struct attribute_group **supported_type_groups;
>>   	struct device_driver driver;
>>   };
>
> mdev_type should be part of mdev_parent, separating it from mdev_parent 
> could result in more errors while using mdev framework.

Why?

> Similarly it should 
> be added as part of mdev_register_device. Below adding types is separated 
> from mdev_register_device which is more error prone.

How so?

> What if driver 
> registering to mdev doesn't add mdev_types? - mdev framework is un-usable 
> in that case.

Yes, so it is if you don't add it to the supported_type_groups field
in the current kernel.  Basic programmer error, and trivially caught.

> We had kept it together with mdev registration so that 
> mdev_types should be mandatory to be defined by driver during registration. 
> How would you mandate mdev_type by such separation?

I would not.  Registering a parent without types is perfectly valid from
the code correctness perspective.  It just isn't very useful.  Just
like say creating a kobject without attributes in the device model.

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Kirti Wankhede]

On 6/7/2022 11:20 AM, Christoph Hellwig wrote:
> On Tue, Jun 07, 2022 at 12:52:49AM +0530, Kirti Wankhede wrote:
>>>    	void (*remove)(struct mdev_device *dev);
>>> -	struct attribute_group **supported_type_groups;
>>>    	struct device_driver driver;
>>>    };
>>
>> mdev_type should be part of mdev_parent, separating it from mdev_parent
>> could result in more errors while using mdev framework.
> 
> Why?
> 
>> Similarly it should
>> be added as part of mdev_register_device. Below adding types is separated
>> from mdev_register_device which is more error prone.
> 
> How so?
> 

Jason has already pointed that about udev event.

>> What if driver
>> registering to mdev doesn't add mdev_types? - mdev framework is un-usable
>> in that case.
> 
> Yes, so it is if you don't add it to the supported_type_groups field
> in the current kernel.  Basic programmer error, and trivially caught.
> 

Current kernel version mandate supported_type_groups, otherwise mdev 
registration fails. But this behavior is being changed with this patch.


>> We had kept it together with mdev registration so that
>> mdev_types should be mandatory to be defined by driver during registration.
>> How would you mandate mdev_type by such separation?
> 
> I would not.  Registering a parent without types is perfectly valid from
> the code correctness perspective.  It just isn't very useful.  Just
> like say creating a kobject without attributes in the device model.

Creating kobject without kobj_type is not allowed in the kernel, 
similarly mdev registration should not be allowed without its type.

Instead of exporting mdev_type_add/mdev_type_remove, these functions 
might be called internally from registration function.

Thanks,
Kirti

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Christoph Hellwig]

On Wed, Jun 08, 2022 at 11:27:02PM +0530, Kirti Wankhede wrote:
>> the code correctness perspective.  It just isn't very useful.  Just
>> like say creating a kobject without attributes in the device model.
>
> Creating kobject without kobj_type is not allowed in the kernel, similarly 
> mdev registration should not be allowed without its type.

But the kobj_type doesn't need to have any attributes.

> Instead of exporting mdev_type_add/mdev_type_remove, these functions might 
> be called internally from registration function.

I very fundamentally disagree.  That is exactly what makes the current
interface so messy and complicated and I want to get rid of that.

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:23AM +0200, Christoph Hellwig wrote:

> @@ -112,9 +88,10 @@ int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
>  			return -ENOMEM;
>  	}
>  
> -	ret = parent_create_sysfs_files(parent);
> -	if (ret)
> -		return ret;
> +	parent->mdev_types_kset = kset_create_and_add("mdev_supported_types",
> +					       NULL, &parent->dev->kobj);
> +	if (!parent->mdev_types_kset)
> +		return -ENOMEM;
>  
>  	ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
>  	if (ret)
[..]
	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);

This changes the ordering with respect to this udev event, now the
mdev_supported_types directory is created but the directory is empty,
then the driver fills it after the KOBJ_CHANGE event is triggered.

Granted this whole abusing some other struct device's sysfs thing is
inherently wrong - I'm not really sure what impact this has. Though at
least someone seemed to care since their is this uevent here...

Maybe we need a 'finish register' call to trigger the uevent? Or
perhaps trigger the uevent when each add_type is done?

Everything else looks fine though, I looked at this for a while a long
time ago and didn't see this nice final arrangement :\

Jason

Re: [PATCH 3/8] vfio/mdev: simplify mdev_type handling

[Christoph Hellwig]

On Mon, Jun 06, 2022 at 08:38:08PM -0300, Jason Gunthorpe wrote:
> Maybe we need a 'finish register' call to trigger the uevent? Or
> perhaps trigger the uevent when each add_type is done?

The finish version would keep the existing behavior as-is, so that
might be a good idea, I'll look into that.

[PATCH 4/8] vfio/mdev: remove mdev_{create,remove}_sysfs_files

[Christoph Hellwig]

Just fold these two trivial helpers into their only callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/vfio/mdev/mdev_core.c    | 12 ++++++++++--
 drivers/vfio/mdev/mdev_private.h |  3 ---
 drivers/vfio/mdev/mdev_sysfs.c   | 28 ----------------------------
 3 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index ff38c9549a55e..34b01d45cfe9f 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -46,7 +46,8 @@ static void mdev_device_remove_common(struct mdev_device *mdev)
 {
 	struct mdev_parent *parent = mdev->type->parent;
 
-	mdev_remove_sysfs_files(mdev);
+	sysfs_remove_link(&mdev->dev.kobj, "mdev_type");
+	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev->dev));
 	device_del(&mdev->dev);
 	lockdep_assert_held(&parent->unreg_sem);
 	/* Balances with device_initialize() */
@@ -193,16 +194,23 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid)
 	if (ret)
 		goto out_del;
 
-	ret = mdev_create_sysfs_files(mdev);
+	ret = sysfs_create_link(type->devices_kobj, &mdev->dev.kobj,
+				dev_name(&mdev->dev));
 	if (ret)
 		goto out_del;
 
+	ret = sysfs_create_link(&mdev->dev.kobj, &type->kobj, "mdev_type");
+	if (ret)
+		goto out_remove_type_link;
+
 	mdev->active = true;
 	dev_dbg(&mdev->dev, "MDEV: created\n");
 	up_read(&parent->unreg_sem);
 
 	return 0;
 
+out_remove_type_link:
+	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev->dev));
 out_del:
 	device_del(&mdev->dev);
 out_unlock:
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 476cc0379ede0..277819f1ebed8 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -20,9 +20,6 @@ extern const struct attribute_group *mdev_device_groups[];
 #define to_mdev_type(_kobj)		\
 	container_of(_kobj, struct mdev_type, kobj)
 
-int  mdev_create_sysfs_files(struct mdev_device *mdev);
-void mdev_remove_sysfs_files(struct mdev_device *mdev);
-
 int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid);
 int  mdev_device_remove(struct mdev_device *dev);
 
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index fb058755d85b8..b6bc623487f06 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -176,31 +176,3 @@ const struct attribute_group *mdev_device_groups[] = {
 	&mdev_device_group,
 	NULL
 };
-
-int mdev_create_sysfs_files(struct mdev_device *mdev)
-{
-	struct mdev_type *type = mdev->type;
-	struct kobject *kobj = &mdev->dev.kobj;
-	int ret;
-
-	ret = sysfs_create_link(type->devices_kobj, kobj, dev_name(&mdev->dev));
-	if (ret)
-		return ret;
-
-	ret = sysfs_create_link(kobj, &type->kobj, "mdev_type");
-	if (ret)
-		goto type_link_failed;
-	return ret;
-
-type_link_failed:
-	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev->dev));
-	return ret;
-}
-
-void mdev_remove_sysfs_files(struct mdev_device *mdev)
-{
-	struct kobject *kobj = &mdev->dev.kobj;
-
-	sysfs_remove_link(kobj, "mdev_type");
-	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev->dev));
-}
-- 
2.30.2

Re: [PATCH 4/8] vfio/mdev: remove mdev_{create,remove}_sysfs_files

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:24AM +0200, Christoph Hellwig wrote:
> Just fold these two trivial helpers into their only callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/vfio/mdev/mdev_core.c    | 12 ++++++++++--
>  drivers/vfio/mdev/mdev_private.h |  3 ---
>  drivers/vfio/mdev/mdev_sysfs.c   | 28 ----------------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 4/8] vfio/mdev: remove mdev_{create,remove}_sysfs_files

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> Just fold these two trivial helpers into their only callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  drivers/vfio/mdev/mdev_core.c    | 12 ++++++++++--
>  drivers/vfio/mdev/mdev_private.h |  3 ---
>  drivers/vfio/mdev/mdev_sysfs.c   | 28 ----------------------------
>  3 files changed, 10 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index ff38c9549a55e..34b01d45cfe9f 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -46,7 +46,8 @@ static void mdev_device_remove_common(struct
> mdev_device *mdev)
>  {
>  	struct mdev_parent *parent = mdev->type->parent;
> 
> -	mdev_remove_sysfs_files(mdev);
> +	sysfs_remove_link(&mdev->dev.kobj, "mdev_type");
> +	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev-
> >dev));
>  	device_del(&mdev->dev);
>  	lockdep_assert_held(&parent->unreg_sem);
>  	/* Balances with device_initialize() */
> @@ -193,16 +194,23 @@ int mdev_device_create(struct mdev_type *type,
> const guid_t *uuid)
>  	if (ret)
>  		goto out_del;
> 
> -	ret = mdev_create_sysfs_files(mdev);
> +	ret = sysfs_create_link(type->devices_kobj, &mdev->dev.kobj,
> +				dev_name(&mdev->dev));
>  	if (ret)
>  		goto out_del;
> 
> +	ret = sysfs_create_link(&mdev->dev.kobj, &type->kobj, "mdev_type");
> +	if (ret)
> +		goto out_remove_type_link;
> +
>  	mdev->active = true;
>  	dev_dbg(&mdev->dev, "MDEV: created\n");
>  	up_read(&parent->unreg_sem);
> 
>  	return 0;
> 
> +out_remove_type_link:
> +	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev-
> >dev));
>  out_del:
>  	device_del(&mdev->dev);
>  out_unlock:
> diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> index 476cc0379ede0..277819f1ebed8 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -20,9 +20,6 @@ extern const struct attribute_group
> *mdev_device_groups[];
>  #define to_mdev_type(_kobj)		\
>  	container_of(_kobj, struct mdev_type, kobj)
> 
> -int  mdev_create_sysfs_files(struct mdev_device *mdev);
> -void mdev_remove_sysfs_files(struct mdev_device *mdev);
> -
>  int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid);
>  int  mdev_device_remove(struct mdev_device *dev);
> 
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> b/drivers/vfio/mdev/mdev_sysfs.c
> index fb058755d85b8..b6bc623487f06 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -176,31 +176,3 @@ const struct attribute_group *mdev_device_groups[]
> = {
>  	&mdev_device_group,
>  	NULL
>  };
> -
> -int mdev_create_sysfs_files(struct mdev_device *mdev)
> -{
> -	struct mdev_type *type = mdev->type;
> -	struct kobject *kobj = &mdev->dev.kobj;
> -	int ret;
> -
> -	ret = sysfs_create_link(type->devices_kobj, kobj, dev_name(&mdev-
> >dev));
> -	if (ret)
> -		return ret;
> -
> -	ret = sysfs_create_link(kobj, &type->kobj, "mdev_type");
> -	if (ret)
> -		goto type_link_failed;
> -	return ret;
> -
> -type_link_failed:
> -	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev-
> >dev));
> -	return ret;
> -}
> -
> -void mdev_remove_sysfs_files(struct mdev_device *mdev)
> -{
> -	struct kobject *kobj = &mdev->dev.kobj;
> -
> -	sysfs_remove_link(kobj, "mdev_type");
> -	sysfs_remove_link(mdev->type->devices_kobj, dev_name(&mdev-
> >dev));
> -}
> --
> 2.30.2

[PATCH 5/8] vfio/mdev: remove mdev_from_dev

[Christoph Hellwig]

Just open code it in the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/vfio/mdev/mdev_core.c | 6 ++----
 include/linux/mdev.h          | 4 ----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 34b01d45cfe9f..3575e893b5e43 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -56,10 +56,8 @@ static void mdev_device_remove_common(struct mdev_device *mdev)
 
 static int mdev_device_remove_cb(struct device *dev, void *data)
 {
-	struct mdev_device *mdev = mdev_from_dev(dev);
-
-	if (mdev)
-		mdev_device_remove_common(mdev);
+	if (dev->bus == &mdev_bus_type)
+		mdev_device_remove_common(to_mdev_device(dev));
 	return 0;
 }
 
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index cbb53dcd20d9d..5811b5a52a511 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -102,9 +102,5 @@ static inline struct device *mdev_dev(struct mdev_device *mdev)
 {
 	return &mdev->dev;
 }
-static inline struct mdev_device *mdev_from_dev(struct device *dev)
-{
-	return dev->bus == &mdev_bus_type ? to_mdev_device(dev) : NULL;
-}
 
 #endif /* MDEV_H */
-- 
2.30.2

Re: [PATCH 5/8] vfio/mdev: remove mdev_from_dev

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:25AM +0200, Christoph Hellwig wrote:
> Just open code it in the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/vfio/mdev/mdev_core.c | 6 ++----
>  include/linux/mdev.h          | 4 ----
>  2 files changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 5/8] vfio/mdev: remove mdev_from_dev

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> Just open code it in the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  drivers/vfio/mdev/mdev_core.c | 6 ++----
>  include/linux/mdev.h          | 4 ----
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 34b01d45cfe9f..3575e893b5e43 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -56,10 +56,8 @@ static void mdev_device_remove_common(struct
> mdev_device *mdev)
> 
>  static int mdev_device_remove_cb(struct device *dev, void *data)
>  {
> -	struct mdev_device *mdev = mdev_from_dev(dev);
> -
> -	if (mdev)
> -		mdev_device_remove_common(mdev);
> +	if (dev->bus == &mdev_bus_type)
> +		mdev_device_remove_common(to_mdev_device(dev));
>  	return 0;
>  }
> 
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index cbb53dcd20d9d..5811b5a52a511 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -102,9 +102,5 @@ static inline struct device *mdev_dev(struct
> mdev_device *mdev)
>  {
>  	return &mdev->dev;
>  }
> -static inline struct mdev_device *mdev_from_dev(struct device *dev)
> -{
> -	return dev->bus == &mdev_bus_type ? to_mdev_device(dev) : NULL;
> -}
> 
>  #endif /* MDEV_H */
> --
> 2.30.2

[PATCH 6/8] vfio/mdev: unexport mdev_bus_type

[Christoph Hellwig]

mdev_bus_type is only used in mdev.ko now, so unexport it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/vfio/mdev/mdev_driver.c  | 1 -
 drivers/vfio/mdev/mdev_private.h | 1 +
 include/linux/mdev.h             | 2 --
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index 9c2af59809e2e..dde6adf23b1db 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -47,7 +47,6 @@ struct bus_type mdev_bus_type = {
 	.remove		= mdev_remove,
 	.match		= mdev_match,
 };
-EXPORT_SYMBOL_GPL(mdev_bus_type);
 
 /**
  * mdev_register_driver - register a new MDEV driver
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 277819f1ebed8..3ecd6ae1dfa7c 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -13,6 +13,7 @@
 int  mdev_bus_register(void);
 void mdev_bus_unregister(void);
 
+extern struct bus_type mdev_bus_type;
 extern const struct attribute_group *mdev_device_groups[];
 
 #define to_mdev_type_attr(_attr)	\
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 5811b5a52a511..f92b4d8edf0e8 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -88,8 +88,6 @@ static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
 	return &mdev->uuid;
 }
 
-extern struct bus_type mdev_bus_type;
-
 int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
 		struct mdev_driver *mdev_driver);
 void mdev_unregister_parent(struct mdev_parent *parent);
-- 
2.30.2

Re: [PATCH 6/8] vfio/mdev: unexport mdev_bus_type

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:26AM +0200, Christoph Hellwig wrote:
> mdev_bus_type is only used in mdev.ko now, so unexport it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/vfio/mdev/mdev_driver.c  | 1 -
>  drivers/vfio/mdev/mdev_private.h | 1 +
>  include/linux/mdev.h             | 2 --
>  3 files changed, 1 insertion(+), 3 deletions(-)

Yay, people trying to sneak in tests of this bus type in platform code
has been a problem in the past

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 6/8] vfio/mdev: unexport mdev_bus_type

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> mdev_bus_type is only used in mdev.ko now, so unexport it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  drivers/vfio/mdev/mdev_driver.c  | 1 -
>  drivers/vfio/mdev/mdev_private.h | 1 +
>  include/linux/mdev.h             | 2 --
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_driver.c
> b/drivers/vfio/mdev/mdev_driver.c
> index 9c2af59809e2e..dde6adf23b1db 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -47,7 +47,6 @@ struct bus_type mdev_bus_type = {
>  	.remove		= mdev_remove,
>  	.match		= mdev_match,
>  };
> -EXPORT_SYMBOL_GPL(mdev_bus_type);
> 
>  /**
>   * mdev_register_driver - register a new MDEV driver
> diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> index 277819f1ebed8..3ecd6ae1dfa7c 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -13,6 +13,7 @@
>  int  mdev_bus_register(void);
>  void mdev_bus_unregister(void);
> 
> +extern struct bus_type mdev_bus_type;
>  extern const struct attribute_group *mdev_device_groups[];
> 
>  #define to_mdev_type_attr(_attr)	\
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 5811b5a52a511..f92b4d8edf0e8 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -88,8 +88,6 @@ static inline const guid_t *mdev_uuid(struct
> mdev_device *mdev)
>  	return &mdev->uuid;
>  }
> 
> -extern struct bus_type mdev_bus_type;
> -
>  int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
>  		struct mdev_driver *mdev_driver);
>  void mdev_unregister_parent(struct mdev_parent *parent);
> --
> 2.30.2

[PATCH 7/8] vfio/mdev: remove mdev_parent_dev

[Christoph Hellwig]

Just open code the dereferences in the only user.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/driver-api/vfio-mediated-device.rst | 3 ---
 drivers/gpu/drm/i915/gvt/kvmgt.c                  | 2 +-
 drivers/vfio/mdev/mdev_core.c                     | 6 ------
 include/linux/mdev.h                              | 1 -
 4 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 8ff463aa9e2be..0d74ac2a218f2 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -201,9 +201,6 @@ Directories and files under the sysfs for Each Physical Device
 
 	sprintf(buf, "%s-%s", dev_driver_string(parent->dev), group->name);
 
-  (or using mdev_parent_dev(mdev) to arrive at the parent device outside
-  of the core mdev code)
-
 * device_api
 
   This attribute should show which device API is being created, for example,
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 06b8acc6a995d..f3c4ce475ce86 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1554,7 +1554,7 @@ static const struct vfio_device_ops intel_vgpu_dev_ops = {
 
 static int intel_vgpu_probe(struct mdev_device *mdev)
 {
-	struct device *pdev = mdev_parent_dev(mdev);
+	struct device *pdev = mdev->type->parent->dev;
 	struct intel_gvt *gvt = kdev_to_i915(pdev)->gvt;
 	struct intel_vgpu_type *type =
 		container_of(mdev->type, struct intel_vgpu_type, type);
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 3575e893b5e43..3bdea77d506d9 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -25,12 +25,6 @@ static struct class_compat *mdev_bus_compat_class;
 static LIST_HEAD(mdev_list);
 static DEFINE_MUTEX(mdev_list_lock);
 
-struct device *mdev_parent_dev(struct mdev_device *mdev)
-{
-	return mdev->type->parent->dev;
-}
-EXPORT_SYMBOL(mdev_parent_dev);
-
 /*
  * Used in mdev_type_attribute sysfs functions to return the parent struct
  * device
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index f92b4d8edf0e8..6d0a9b45a9eb6 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -95,7 +95,6 @@ void mdev_unregister_parent(struct mdev_parent *parent);
 int mdev_register_driver(struct mdev_driver *drv);
 void mdev_unregister_driver(struct mdev_driver *drv);
 
-struct device *mdev_parent_dev(struct mdev_device *mdev);
 static inline struct device *mdev_dev(struct mdev_device *mdev)
 {
 	return &mdev->dev;
-- 
2.30.2

Re: [PATCH 7/8] vfio/mdev: remove mdev_parent_dev

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:27AM +0200, Christoph Hellwig wrote:
> Just open code the dereferences in the only user.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  Documentation/driver-api/vfio-mediated-device.rst | 3 ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c                  | 2 +-
>  drivers/vfio/mdev/mdev_core.c                     | 6 ------
>  include/linux/mdev.h                              | 1 -
>  4 files changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 7/8] vfio/mdev: remove mdev_parent_dev

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> Just open code the dereferences in the only user.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  Documentation/driver-api/vfio-mediated-device.rst | 3 ---
>  drivers/gpu/drm/i915/gvt/kvmgt.c                  | 2 +-
>  drivers/vfio/mdev/mdev_core.c                     | 6 ------
>  include/linux/mdev.h                              | 1 -
>  4 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 8ff463aa9e2be..0d74ac2a218f2 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -201,9 +201,6 @@ Directories and files under the sysfs for Each Physical
> Device
> 
>  	sprintf(buf, "%s-%s", dev_driver_string(parent->dev), group->name);
> 
> -  (or using mdev_parent_dev(mdev) to arrive at the parent device outside
> -  of the core mdev code)
> -
>  * device_api
> 
>    This attribute should show which device API is being created, for example,
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 06b8acc6a995d..f3c4ce475ce86 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1554,7 +1554,7 @@ static const struct vfio_device_ops
> intel_vgpu_dev_ops = {
> 
>  static int intel_vgpu_probe(struct mdev_device *mdev)
>  {
> -	struct device *pdev = mdev_parent_dev(mdev);
> +	struct device *pdev = mdev->type->parent->dev;
>  	struct intel_gvt *gvt = kdev_to_i915(pdev)->gvt;
>  	struct intel_vgpu_type *type =
>  		container_of(mdev->type, struct intel_vgpu_type, type);
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 3575e893b5e43..3bdea77d506d9 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -25,12 +25,6 @@ static struct class_compat *mdev_bus_compat_class;
>  static LIST_HEAD(mdev_list);
>  static DEFINE_MUTEX(mdev_list_lock);
> 
> -struct device *mdev_parent_dev(struct mdev_device *mdev)
> -{
> -	return mdev->type->parent->dev;
> -}
> -EXPORT_SYMBOL(mdev_parent_dev);
> -
>  /*
>   * Used in mdev_type_attribute sysfs functions to return the parent struct
>   * device
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index f92b4d8edf0e8..6d0a9b45a9eb6 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -95,7 +95,6 @@ void mdev_unregister_parent(struct mdev_parent
> *parent);
>  int mdev_register_driver(struct mdev_driver *drv);
>  void mdev_unregister_driver(struct mdev_driver *drv);
> 
> -struct device *mdev_parent_dev(struct mdev_device *mdev);
>  static inline struct device *mdev_dev(struct mdev_device *mdev)
>  {
>  	return &mdev->dev;
> --
> 2.30.2

[PATCH 8/8] vfio/mdev: remove mtype_get_parent_dev

[Christoph Hellwig]

Just open code the dereferences in the only user.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/cio/vfio_ccw_ops.c |  3 +--
 drivers/vfio/mdev/mdev_core.c   | 10 ----------
 include/linux/mdev.h            |  2 --
 3 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index e219ea3c62fc6..4864271cffc1e 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -88,8 +88,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
 {
-	struct vfio_ccw_private *private =
-		dev_get_drvdata(mtype_get_parent_dev(mtype));
+	struct vfio_ccw_private *private = dev_get_drvdata(mtype->parent->dev);
 
 	return sprintf(buf, "%d\n", atomic_read(&private->avail));
 }
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 3bdea77d506d9..3f51fbdf9393c 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -25,16 +25,6 @@ static struct class_compat *mdev_bus_compat_class;
 static LIST_HEAD(mdev_list);
 static DEFINE_MUTEX(mdev_list_lock);
 
-/*
- * Used in mdev_type_attribute sysfs functions to return the parent struct
- * device
- */
-struct device *mtype_get_parent_dev(struct mdev_type *mtype)
-{
-	return mtype->parent->dev;
-}
-EXPORT_SYMBOL(mtype_get_parent_dev);
-
 /* Caller must hold parent unreg_sem read or write lock */
 static void mdev_device_remove_common(struct mdev_device *mdev)
 {
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 6d0a9b45a9eb6..dbf054d084886 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -48,8 +48,6 @@ int mdev_type_add(struct mdev_parent *parent, struct mdev_type *type,
 void mdev_type_remove(struct mdev_type *type,
 		const struct attribute * const *attrs);
 
-struct device *mtype_get_parent_dev(struct mdev_type *mtype);
-
 /* interface for exporting mdev supported type attributes */
 struct mdev_type_attribute {
 	struct attribute attr;
-- 
2.30.2

Re: [PATCH 8/8] vfio/mdev: remove mtype_get_parent_dev

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 08:33:28AM +0200, Christoph Hellwig wrote:
> Just open code the dereferences in the only user.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c |  3 +--
>  drivers/vfio/mdev/mdev_core.c   | 10 ----------
>  include/linux/mdev.h            |  2 --
>  3 files changed, 1 insertion(+), 14 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH 8/8] vfio/mdev: remove mtype_get_parent_dev

[Tian, Kevin]

> From: Christoph Hellwig
> Sent: Friday, June 3, 2022 2:33 PM
> 
> Just open code the dereferences in the only user.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

> ---
>  drivers/s390/cio/vfio_ccw_ops.c |  3 +--
>  drivers/vfio/mdev/mdev_core.c   | 10 ----------
>  include/linux/mdev.h            |  2 --
>  3 files changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> b/drivers/s390/cio/vfio_ccw_ops.c
> index e219ea3c62fc6..4864271cffc1e 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -88,8 +88,7 @@ static ssize_t available_instances_show(struct
> mdev_type *mtype,
>  					struct mdev_type_attribute *attr,
>  					char *buf)
>  {
> -	struct vfio_ccw_private *private =
> -		dev_get_drvdata(mtype_get_parent_dev(mtype));
> +	struct vfio_ccw_private *private = dev_get_drvdata(mtype->parent-
> >dev);
> 
>  	return sprintf(buf, "%d\n", atomic_read(&private->avail));
>  }
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 3bdea77d506d9..3f51fbdf9393c 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -25,16 +25,6 @@ static struct class_compat *mdev_bus_compat_class;
>  static LIST_HEAD(mdev_list);
>  static DEFINE_MUTEX(mdev_list_lock);
> 
> -/*
> - * Used in mdev_type_attribute sysfs functions to return the parent struct
> - * device
> - */
> -struct device *mtype_get_parent_dev(struct mdev_type *mtype)
> -{
> -	return mtype->parent->dev;
> -}
> -EXPORT_SYMBOL(mtype_get_parent_dev);
> -
>  /* Caller must hold parent unreg_sem read or write lock */
>  static void mdev_device_remove_common(struct mdev_device *mdev)
>  {
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 6d0a9b45a9eb6..dbf054d084886 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -48,8 +48,6 @@ int mdev_type_add(struct mdev_parent *parent, struct
> mdev_type *type,
>  void mdev_type_remove(struct mdev_type *type,
>  		const struct attribute * const *attrs);
> 
> -struct device *mtype_get_parent_dev(struct mdev_type *mtype);
> -
>  /* interface for exporting mdev supported type attributes */
>  struct mdev_type_attribute {
>  	struct attribute attr;
> --
> 2.30.2