[PATCH 00/18] Make vfio_mdev type safe

[PATCH 00/18] Make vfio_mdev type safe

[Jason Gunthorpe]

Prologue
========

This is series #2 in part of a larger work that arose from the minor
remark that the mdev_parent_ops indirection shim is useless and
complicates things.

It follows the "Embed struct vfio_device in all sub-structures" already
sent, and must be applied on top of it.

A preview of the future series's is here:
  https://github.com/jgunthorpe/linux/pull/3/commits

========
This series:

vfio_mdev has a number of different objects: mdev_parent, mdev_type and
mdev_device.

Unfortunately the types of these have been erased in various places
throughout the API, and this makes it very hard to understand this code or
maintain it by the time it reaches all of the drivers.

This series puts in all the types and aligns some of the design with the
driver core standard for a driver core bus driver:

 - Replace 'struct device *' with 'struct mdev_device *
 - Replace 'struct device *' with 'struct mdev_type *' and
   mtype_get_parent_dev()
 - Replace 'struct kobject *' with 'struct mdev_type *'

Now that types are clear it is easy to spot a few places that have
duplicated information.

More significantly we can now understand how to directly fix the
obfuscated 'kobj->name' matching by realizing the the kobj is a mdev_type,
which is linked to the supported_types_list provided by the driver, and
thus the core code can directly return the array indexes all the drivers
actually want.

Jason

Jason Gunthorpe (18):
  vfio/mdev: Fix missing static's on MDEV_TYPE_ATTR's
  vfio/mdev: Add missing typesafety around mdev_device
  vfio/mdev: Simplify driver registration
  vfio/mdev: Use struct mdev_type in struct mdev_device
  vfio/mdev: Do not allow a mdev_type to have a NULL parent pointer
  vfio/mdev: Expose mdev_get/put_parent to mdev_private.h
  vfio/mdev: Add missing reference counting to mdev_type
  vfio/mdev: Reorganize mdev_device_create()
  vfio/mdev: Add missing error handling to dev_set_name()
  vfio/mdev: Remove duplicate storage of parent in mdev_device
  vfio/mdev: Add mdev/mtype_get_type_group_id()
  vfio/mtty: Use mdev_get_type_group_id()
  vfio/mdpy: Use mdev_get_type_group_id()
  vfio/mbochs: Use mdev_get_type_group_id()
  vfio/gvt: Make DRM_I915_GVT depend on VFIO_MDEV
  vfio/gvt: Use mdev_get_type_group_id()
  vfio/mdev: Remove kobj from mdev_parent_ops->create()
  vfio/mdev: Correct the function signatures for the
    mdev_type_attributes

 .../driver-api/vfio-mediated-device.rst       |   9 +-
 drivers/gpu/drm/i915/Kconfig                  |   1 +
 drivers/gpu/drm/i915/gvt/gvt.c                |  41 ++---
 drivers/gpu/drm/i915/gvt/gvt.h                |   4 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c              |   7 +-
 drivers/s390/cio/vfio_ccw_ops.c               |  17 +-
 drivers/s390/crypto/vfio_ap_ops.c             |  14 +-
 drivers/vfio/mdev/mdev_core.c                 | 160 ++++++------------
 drivers/vfio/mdev/mdev_driver.c               |  19 +--
 drivers/vfio/mdev/mdev_private.h              |  40 ++---
 drivers/vfio/mdev/mdev_sysfs.c                |  59 ++++---
 drivers/vfio/mdev/vfio_mdev.c                 |  29 ++--
 drivers/vfio/vfio_iommu_type1.c               |  25 +--
 include/linux/mdev.h                          |  80 ++++++---
 samples/vfio-mdev/mbochs.c                    |  55 +++---
 samples/vfio-mdev/mdpy.c                      |  56 +++---
 samples/vfio-mdev/mtty.c                      |  66 ++------
 17 files changed, 306 insertions(+), 376 deletions(-)

-- 
2.31.0

[PATCH 02/18] vfio/mdev: Add missing typesafety around mdev_device

[Jason Gunthorpe]

The mdev API should accept and pass a 'struct mdev_device *' in all
places, not pass a 'struct device *' and cast it internally with
to_mdev_device(). Particularly in its struct mdev_driver functions, the
whole point of a bus's struct device_driver wrapper is to provide type
safety compared to the default struct device_driver.

Further, the driver core standard is for bus drivers to expose their
device structure in their public headers that can be used with
container_of() inlines and '&foo->dev' to go between the class levels, and
'&foo->dev' to be used with dev_err/etc driver core helper functions. Move
'struct mdev_device' to mdev.h

Once done this allows moving some one instruction exported functions to
static inlines, which in turns allows removing one of the two grotesque
symbol_get()'s related to mdev in the core code.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .../driver-api/vfio-mediated-device.rst       |  4 +-
 drivers/vfio/mdev/mdev_core.c                 | 64 ++-----------------
 drivers/vfio/mdev/mdev_driver.c               |  4 +-
 drivers/vfio/mdev/mdev_private.h              | 23 +------
 drivers/vfio/mdev/mdev_sysfs.c                | 26 ++++----
 drivers/vfio/mdev/vfio_mdev.c                 |  7 +-
 drivers/vfio/vfio_iommu_type1.c               | 25 ++------
 include/linux/mdev.h                          | 58 +++++++++++++----
 8 files changed, 83 insertions(+), 128 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834ba3..c43c1dc3333373 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -105,8 +105,8 @@ structure to represent a mediated device's driver::
       */
      struct mdev_driver {
 	     const char *name;
-	     int  (*probe)  (struct device *dev);
-	     void (*remove) (struct device *dev);
+	     int  (*probe)  (struct mdev_device *dev);
+	     void (*remove) (struct mdev_device *dev);
 	     struct device_driver    driver;
      };
 
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 6de97d25a3f87d..057922a1707e04 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -33,36 +33,6 @@ struct device *mdev_parent_dev(struct mdev_device *mdev)
 }
 EXPORT_SYMBOL(mdev_parent_dev);
 
-void *mdev_get_drvdata(struct mdev_device *mdev)
-{
-	return mdev->driver_data;
-}
-EXPORT_SYMBOL(mdev_get_drvdata);
-
-void mdev_set_drvdata(struct mdev_device *mdev, void *data)
-{
-	mdev->driver_data = data;
-}
-EXPORT_SYMBOL(mdev_set_drvdata);
-
-struct device *mdev_dev(struct mdev_device *mdev)
-{
-	return &mdev->dev;
-}
-EXPORT_SYMBOL(mdev_dev);
-
-struct mdev_device *mdev_from_dev(struct device *dev)
-{
-	return dev_is_mdev(dev) ? to_mdev_device(dev) : NULL;
-}
-EXPORT_SYMBOL(mdev_from_dev);
-
-const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
-EXPORT_SYMBOL(mdev_uuid);
-
 /* Should be called holding parent_list_lock */
 static struct mdev_parent *__find_parent_device(struct device *dev)
 {
@@ -107,7 +77,7 @@ static void mdev_device_remove_common(struct mdev_device *mdev)
 	int ret;
 
 	type = to_mdev_type(mdev->type_kobj);
-	mdev_remove_sysfs_files(&mdev->dev, type);
+	mdev_remove_sysfs_files(mdev, type);
 	device_del(&mdev->dev);
 	parent = mdev->parent;
 	lockdep_assert_held(&parent->unreg_sem);
@@ -122,12 +92,10 @@ static void mdev_device_remove_common(struct mdev_device *mdev)
 
 static int mdev_device_remove_cb(struct device *dev, void *data)
 {
-	if (dev_is_mdev(dev)) {
-		struct mdev_device *mdev;
+	struct mdev_device *mdev = mdev_from_dev(dev);
 
-		mdev = to_mdev_device(dev);
+	if (mdev)
 		mdev_device_remove_common(mdev);
-	}
 	return 0;
 }
 
@@ -332,7 +300,7 @@ int mdev_device_create(struct kobject *kobj,
 	if (ret)
 		goto add_fail;
 
-	ret = mdev_create_sysfs_files(&mdev->dev, type);
+	ret = mdev_create_sysfs_files(mdev, type);
 	if (ret)
 		goto sysfs_fail;
 
@@ -354,13 +322,11 @@ int mdev_device_create(struct kobject *kobj,
 	return ret;
 }
 
-int mdev_device_remove(struct device *dev)
+int mdev_device_remove(struct mdev_device *mdev)
 {
-	struct mdev_device *mdev, *tmp;
+	struct mdev_device *tmp;
 	struct mdev_parent *parent;
 
-	mdev = to_mdev_device(dev);
-
 	mutex_lock(&mdev_list_lock);
 	list_for_each_entry(tmp, &mdev_list, next) {
 		if (tmp == mdev)
@@ -390,24 +356,6 @@ int mdev_device_remove(struct device *dev)
 	return 0;
 }
 
-int mdev_set_iommu_device(struct device *dev, struct device *iommu_device)
-{
-	struct mdev_device *mdev = to_mdev_device(dev);
-
-	mdev->iommu_device = iommu_device;
-
-	return 0;
-}
-EXPORT_SYMBOL(mdev_set_iommu_device);
-
-struct device *mdev_get_iommu_device(struct device *dev)
-{
-	struct mdev_device *mdev = to_mdev_device(dev);
-
-	return mdev->iommu_device;
-}
-EXPORT_SYMBOL(mdev_get_iommu_device);
-
 static int __init mdev_init(void)
 {
 	return mdev_bus_register();
diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index 0d3223aee20b83..44c3ba7e56d923 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -48,7 +48,7 @@ static int mdev_probe(struct device *dev)
 		return ret;
 
 	if (drv && drv->probe) {
-		ret = drv->probe(dev);
+		ret = drv->probe(mdev);
 		if (ret)
 			mdev_detach_iommu(mdev);
 	}
@@ -62,7 +62,7 @@ static int mdev_remove(struct device *dev)
 	struct mdev_device *mdev = to_mdev_device(dev);
 
 	if (drv && drv->remove)
-		drv->remove(dev);
+		drv->remove(mdev);
 
 	mdev_detach_iommu(mdev);
 
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 74c2e541146999..bb60ec4a8d9d21 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -24,23 +24,6 @@ struct mdev_parent {
 	struct rw_semaphore unreg_sem;
 };
 
-struct mdev_device {
-	struct device dev;
-	struct mdev_parent *parent;
-	guid_t uuid;
-	void *driver_data;
-	struct list_head next;
-	struct kobject *type_kobj;
-	struct device *iommu_device;
-	bool active;
-};
-
-static inline struct mdev_device *to_mdev_device(struct device *dev)
-{
-	return container_of(dev, struct mdev_device, dev);
-}
-#define dev_is_mdev(d)		((d)->bus == &mdev_bus_type)
-
 struct mdev_type {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -57,11 +40,11 @@ struct mdev_type {
 int  parent_create_sysfs_files(struct mdev_parent *parent);
 void parent_remove_sysfs_files(struct mdev_parent *parent);
 
-int  mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
-void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
+int  mdev_create_sysfs_files(struct mdev_device *mdev, struct mdev_type *type);
+void mdev_remove_sysfs_files(struct mdev_device *mdev, struct mdev_type *type);
 
 int  mdev_device_create(struct kobject *kobj,
 			struct device *dev, const guid_t *uuid);
-int  mdev_device_remove(struct device *dev);
+int  mdev_device_remove(struct mdev_device *dev);
 
 #endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 917fd84c1c6f24..6a5450587b79e9 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -225,6 +225,7 @@ int parent_create_sysfs_files(struct mdev_parent *parent)
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
+	struct mdev_device *mdev = to_mdev_device(dev);
 	unsigned long val;
 
 	if (kstrtoul(buf, 0, &val) < 0)
@@ -233,7 +234,7 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 	if (val && device_remove_file_self(dev, attr)) {
 		int ret;
 
-		ret = mdev_device_remove(dev);
+		ret = mdev_device_remove(mdev);
 		if (ret)
 			return ret;
 	}
@@ -248,34 +249,37 @@ static const struct attribute *mdev_device_attrs[] = {
 	NULL,
 };
 
-int  mdev_create_sysfs_files(struct device *dev, struct mdev_type *type)
+int mdev_create_sysfs_files(struct mdev_device *mdev, struct mdev_type *type)
 {
+	struct kobject *kobj = &mdev->dev.kobj;
 	int ret;
 
-	ret = sysfs_create_link(type->devices_kobj, &dev->kobj, dev_name(dev));
+	ret = sysfs_create_link(type->devices_kobj, kobj, dev_name(&mdev->dev));
 	if (ret)
 		return ret;
 
-	ret = sysfs_create_link(&dev->kobj, &type->kobj, "mdev_type");
+	ret = sysfs_create_link(kobj, &type->kobj, "mdev_type");
 	if (ret)
 		goto type_link_failed;
 
-	ret = sysfs_create_files(&dev->kobj, mdev_device_attrs);
+	ret = sysfs_create_files(kobj, mdev_device_attrs);
 	if (ret)
 		goto create_files_failed;
 
 	return ret;
 
 create_files_failed:
-	sysfs_remove_link(&dev->kobj, "mdev_type");
+	sysfs_remove_link(kobj, "mdev_type");
 type_link_failed:
-	sysfs_remove_link(type->devices_kobj, dev_name(dev));
+	sysfs_remove_link(type->devices_kobj, dev_name(&mdev->dev));
 	return ret;
 }
 
-void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type)
+void mdev_remove_sysfs_files(struct mdev_device *mdev, struct mdev_type *type)
 {
-	sysfs_remove_files(&dev->kobj, mdev_device_attrs);
-	sysfs_remove_link(&dev->kobj, "mdev_type");
-	sysfs_remove_link(type->devices_kobj, dev_name(dev));
+	struct kobject *kobj = &mdev->dev.kobj;
+
+	sysfs_remove_files(kobj, mdev_device_attrs);
+	sysfs_remove_link(kobj, "mdev_type");
+	sysfs_remove_link(type->devices_kobj, dev_name(&mdev->dev));
 }
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index ae7e322fbe3c26..91b7b8b9eb9cb8 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -124,9 +124,8 @@ static const struct vfio_device_ops vfio_mdev_dev_ops = {
 	.request	= vfio_mdev_request,
 };
 
-static int vfio_mdev_probe(struct device *dev)
+static int vfio_mdev_probe(struct mdev_device *mdev)
 {
-	struct mdev_device *mdev = to_mdev_device(dev);
 	struct vfio_device *vdev;
 	int ret;
 
@@ -144,9 +143,9 @@ static int vfio_mdev_probe(struct device *dev)
 	return 0;
 }
 
-static void vfio_mdev_remove(struct device *dev)
+static void vfio_mdev_remove(struct mdev_device *mdev)
 {
-	struct vfio_device *vdev = dev_get_drvdata(dev);
+	struct vfio_device *vdev = dev_get_drvdata(&mdev->dev);
 
 	vfio_unregister_group_dev(vdev);
 	kfree(vdev);
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4bb162c1d649b3..90b45ff1d87a7b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1923,28 +1923,13 @@ static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
 	return ret;
 }
 
-static struct device *vfio_mdev_get_iommu_device(struct device *dev)
-{
-	struct device *(*fn)(struct device *dev);
-	struct device *iommu_device;
-
-	fn = symbol_get(mdev_get_iommu_device);
-	if (fn) {
-		iommu_device = fn(dev);
-		symbol_put(mdev_get_iommu_device);
-
-		return iommu_device;
-	}
-
-	return NULL;
-}
-
 static int vfio_mdev_attach_domain(struct device *dev, void *data)
 {
+	struct mdev_device *mdev = to_mdev_device(dev);
 	struct iommu_domain *domain = data;
 	struct device *iommu_device;
 
-	iommu_device = vfio_mdev_get_iommu_device(dev);
+	iommu_device = mdev_get_iommu_device(mdev);
 	if (iommu_device) {
 		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
 			return iommu_aux_attach_device(domain, iommu_device);
@@ -1957,10 +1942,11 @@ static int vfio_mdev_attach_domain(struct device *dev, void *data)
 
 static int vfio_mdev_detach_domain(struct device *dev, void *data)
 {
+	struct mdev_device *mdev = to_mdev_device(dev);
 	struct iommu_domain *domain = data;
 	struct device *iommu_device;
 
-	iommu_device = vfio_mdev_get_iommu_device(dev);
+	iommu_device = mdev_get_iommu_device(mdev);
 	if (iommu_device) {
 		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
 			iommu_aux_detach_device(domain, iommu_device);
@@ -2008,9 +1994,10 @@ static bool vfio_bus_is_mdev(struct bus_type *bus)
 
 static int vfio_mdev_iommu_device(struct device *dev, void *data)
 {
+	struct mdev_device *mdev = to_mdev_device(dev);
 	struct device **old = data, *new;
 
-	new = vfio_mdev_get_iommu_device(dev);
+	new = mdev_get_iommu_device(mdev);
 	if (!new || (*old && *old != new))
 		return -EINVAL;
 
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 27eb383cb95de0..52f7ea19dd0f56 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -10,7 +10,21 @@
 #ifndef MDEV_H
 #define MDEV_H
 
-struct mdev_device;
+struct mdev_device {
+	struct device dev;
+	struct mdev_parent *parent;
+	guid_t uuid;
+	void *driver_data;
+	struct list_head next;
+	struct kobject *type_kobj;
+	struct device *iommu_device;
+	bool active;
+};
+
+static inline struct mdev_device *to_mdev_device(struct device *dev)
+{
+	return container_of(dev, struct mdev_device, dev);
+}
 
 /*
  * Called by the parent device driver to set the device which represents
@@ -19,12 +33,17 @@ struct mdev_device;
  *
  * @dev: the mediated device that iommu will isolate.
  * @iommu_device: a pci device which represents the iommu for @dev.
- *
- * Return 0 for success, otherwise negative error value.
  */
-int mdev_set_iommu_device(struct device *dev, struct device *iommu_device);
+static inline void mdev_set_iommu_device(struct mdev_device *mdev,
+					 struct device *iommu_device)
+{
+	mdev->iommu_device = iommu_device;
+}
 
-struct device *mdev_get_iommu_device(struct device *dev);
+static inline struct device *mdev_get_iommu_device(struct mdev_device *mdev)
+{
+	return mdev->iommu_device;
+}
 
 /**
  * struct mdev_parent_ops - Structure to be registered for each parent device to
@@ -126,16 +145,25 @@ struct mdev_type_attribute mdev_type_attr_##_name =		\
  **/
 struct mdev_driver {
 	const char *name;
-	int  (*probe)(struct device *dev);
-	void (*remove)(struct device *dev);
+	int (*probe)(struct mdev_device *dev);
+	void (*remove)(struct mdev_device *dev);
 	struct device_driver driver;
 };
 
 #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
 
-void *mdev_get_drvdata(struct mdev_device *mdev);
-void mdev_set_drvdata(struct mdev_device *mdev, void *data);
-const guid_t *mdev_uuid(struct mdev_device *mdev);
+static inline void *mdev_get_drvdata(struct mdev_device *mdev)
+{
+	return mdev->driver_data;
+}
+static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
+{
+	mdev->driver_data = data;
+}
+static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
+{
+	return &mdev->uuid;
+}
 
 extern struct bus_type mdev_bus_type;
 
@@ -146,7 +174,13 @@ int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
 void mdev_unregister_driver(struct mdev_driver *drv);
 
 struct device *mdev_parent_dev(struct mdev_device *mdev);
-struct device *mdev_dev(struct mdev_device *mdev);
-struct mdev_device *mdev_from_dev(struct device *dev);
+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.31.0

[PATCH 03/18] vfio/mdev: Simplify driver registration

[Jason Gunthorpe]

This is only done once, we don't need to generate code to initialize a
structure stored in the ELF .data segment. Fill in the three required
.driver members directly instead of copying data into them during
mdev_register_driver().

Further the to_mdev_driver() function doesn't belong in a public header,
just inline it into the two places that need it. Finally, we can now
clearly see that 'drv' derived from dev->driver cannot be NULL, firstly
because the driver core forbids it, and secondly because NULL won't pass
through the container_of(). Remove the dead code.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 Documentation/driver-api/vfio-mediated-device.rst |  5 +----
 drivers/vfio/mdev/mdev_driver.c                   | 15 +++++++--------
 drivers/vfio/mdev/vfio_mdev.c                     |  8 ++++++--
 include/linux/mdev.h                              |  6 +-----
 4 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index c43c1dc3333373..1779b85f014e2f 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -98,13 +98,11 @@ structure to represent a mediated device's driver::
 
      /*
       * struct mdev_driver [2] - Mediated device's driver
-      * @name: driver name
       * @probe: called when new device created
       * @remove: called when device removed
       * @driver: device driver structure
       */
      struct mdev_driver {
-	     const char *name;
 	     int  (*probe)  (struct mdev_device *dev);
 	     void (*remove) (struct mdev_device *dev);
 	     struct device_driver    driver;
@@ -115,8 +113,7 @@ to register and unregister itself with the core driver:
 
 * Register::
 
-    extern int  mdev_register_driver(struct mdev_driver *drv,
-				   struct module *owner);
+    extern int  mdev_register_driver(struct mdev_driver *drv);
 
 * Unregister::
 
diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index 44c3ba7e56d923..041699571b7e55 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -39,7 +39,8 @@ static void mdev_detach_iommu(struct mdev_device *mdev)
 
 static int mdev_probe(struct device *dev)
 {
-	struct mdev_driver *drv = to_mdev_driver(dev->driver);
+	struct mdev_driver *drv =
+		container_of(dev->driver, struct mdev_driver, driver);
 	struct mdev_device *mdev = to_mdev_device(dev);
 	int ret;
 
@@ -47,7 +48,7 @@ static int mdev_probe(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (drv && drv->probe) {
+	if (drv->probe) {
 		ret = drv->probe(mdev);
 		if (ret)
 			mdev_detach_iommu(mdev);
@@ -58,10 +59,11 @@ static int mdev_probe(struct device *dev)
 
 static int mdev_remove(struct device *dev)
 {
-	struct mdev_driver *drv = to_mdev_driver(dev->driver);
+	struct mdev_driver *drv =
+		container_of(dev->driver, struct mdev_driver, driver);
 	struct mdev_device *mdev = to_mdev_device(dev);
 
-	if (drv && drv->remove)
+	if (drv->remove)
 		drv->remove(mdev);
 
 	mdev_detach_iommu(mdev);
@@ -79,16 +81,13 @@ EXPORT_SYMBOL_GPL(mdev_bus_type);
 /**
  * mdev_register_driver - register a new MDEV driver
  * @drv: the driver to register
- * @owner: module owner of driver to be registered
  *
  * Returns a negative value on error, otherwise 0.
  **/
-int mdev_register_driver(struct mdev_driver *drv, struct module *owner)
+int mdev_register_driver(struct mdev_driver *drv)
 {
 	/* initialize common driver fields */
-	drv->driver.name = drv->name;
 	drv->driver.bus = &mdev_bus_type;
-	drv->driver.owner = owner;
 
 	/* register with core */
 	return driver_register(&drv->driver);
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 91b7b8b9eb9cb8..cc9507ed85a181 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -152,14 +152,18 @@ static void vfio_mdev_remove(struct mdev_device *mdev)
 }
 
 static struct mdev_driver vfio_mdev_driver = {
-	.name	= "vfio_mdev",
+	.driver = {
+		.name = "vfio_mdev",
+		.owner = THIS_MODULE,
+		.mod_name = KBUILD_MODNAME,
+	},
 	.probe	= vfio_mdev_probe,
 	.remove	= vfio_mdev_remove,
 };
 
 static int __init vfio_mdev_init(void)
 {
-	return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE);
+	return mdev_register_driver(&vfio_mdev_driver);
 }
 
 static void __exit vfio_mdev_exit(void)
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 52f7ea19dd0f56..cb771c712da0f4 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -137,21 +137,17 @@ struct mdev_type_attribute mdev_type_attr_##_name =		\
 
 /**
  * struct mdev_driver - Mediated device driver
- * @name: driver name
  * @probe: called when new device created
  * @remove: called when device removed
  * @driver: device driver structure
  *
  **/
 struct mdev_driver {
-	const char *name;
 	int (*probe)(struct mdev_device *dev);
 	void (*remove)(struct mdev_device *dev);
 	struct device_driver driver;
 };
 
-#define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
-
 static inline void *mdev_get_drvdata(struct mdev_device *mdev)
 {
 	return mdev->driver_data;
@@ -170,7 +166,7 @@ extern struct bus_type mdev_bus_type;
 int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops);
 void mdev_unregister_device(struct device *dev);
 
-int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
+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);
-- 
2.31.0

Re: [PATCH 03/18] vfio/mdev: Simplify driver registration

[Christoph Hellwig]

>  static struct mdev_driver vfio_mdev_driver = {
> +	.driver = {
> +		.name = "vfio_mdev",
> +		.owner = THIS_MODULE,
> +		.mod_name = KBUILD_MODNAME,
> +	},

What is the mod_name initialization for?  I've not really seen
that in anywere else, and the only user seems to be
module_add_driver for a rather odd case we shouldn't hit here.

The rest looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

RE: [PATCH 02/18] vfio/mdev: Add missing typesafety around mdev_device

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 24, 2021 1:55 AM
> 
> The mdev API should accept and pass a 'struct mdev_device *' in all
> places, not pass a 'struct device *' and cast it internally with
> to_mdev_device(). Particularly in its struct mdev_driver functions, the
> whole point of a bus's struct device_driver wrapper is to provide type
> safety compared to the default struct device_driver.
> 
> Further, the driver core standard is for bus drivers to expose their
> device structure in their public headers that can be used with
> container_of() inlines and '&foo->dev' to go between the class levels, and
> '&foo->dev' to be used with dev_err/etc driver core helper functions. Move
> 'struct mdev_device' to mdev.h
> 
> Once done this allows moving some one instruction exported functions to
> static inlines, which in turns allows removing one of the two grotesque
> symbol_get()'s related to mdev in the core code.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

> ---
>  .../driver-api/vfio-mediated-device.rst       |  4 +-
>  drivers/vfio/mdev/mdev_core.c                 | 64 ++-----------------
>  drivers/vfio/mdev/mdev_driver.c               |  4 +-
>  drivers/vfio/mdev/mdev_private.h              | 23 +------
>  drivers/vfio/mdev/mdev_sysfs.c                | 26 ++++----
>  drivers/vfio/mdev/vfio_mdev.c                 |  7 +-
>  drivers/vfio/vfio_iommu_type1.c               | 25 ++------
>  include/linux/mdev.h                          | 58 +++++++++++++----
>  8 files changed, 83 insertions(+), 128 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 25eb7d5b834ba3..c43c1dc3333373 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -105,8 +105,8 @@ structure to represent a mediated device's driver::
>        */
>       struct mdev_driver {
>  	     const char *name;
> -	     int  (*probe)  (struct device *dev);
> -	     void (*remove) (struct device *dev);
> +	     int  (*probe)  (struct mdev_device *dev);
> +	     void (*remove) (struct mdev_device *dev);
>  	     struct device_driver    driver;
>       };
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c
> b/drivers/vfio/mdev/mdev_core.c
> index 6de97d25a3f87d..057922a1707e04 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -33,36 +33,6 @@ struct device *mdev_parent_dev(struct mdev_device
> *mdev)
>  }
>  EXPORT_SYMBOL(mdev_parent_dev);
> 
> -void *mdev_get_drvdata(struct mdev_device *mdev)
> -{
> -	return mdev->driver_data;
> -}
> -EXPORT_SYMBOL(mdev_get_drvdata);
> -
> -void mdev_set_drvdata(struct mdev_device *mdev, void *data)
> -{
> -	mdev->driver_data = data;
> -}
> -EXPORT_SYMBOL(mdev_set_drvdata);
> -
> -struct device *mdev_dev(struct mdev_device *mdev)
> -{
> -	return &mdev->dev;
> -}
> -EXPORT_SYMBOL(mdev_dev);
> -
> -struct mdev_device *mdev_from_dev(struct device *dev)
> -{
> -	return dev_is_mdev(dev) ? to_mdev_device(dev) : NULL;
> -}
> -EXPORT_SYMBOL(mdev_from_dev);
> -
> -const guid_t *mdev_uuid(struct mdev_device *mdev)
> -{
> -	return &mdev->uuid;
> -}
> -EXPORT_SYMBOL(mdev_uuid);
> -
>  /* Should be called holding parent_list_lock */
>  static struct mdev_parent *__find_parent_device(struct device *dev)
>  {
> @@ -107,7 +77,7 @@ static void mdev_device_remove_common(struct
> mdev_device *mdev)
>  	int ret;
> 
>  	type = to_mdev_type(mdev->type_kobj);
> -	mdev_remove_sysfs_files(&mdev->dev, type);
> +	mdev_remove_sysfs_files(mdev, type);
>  	device_del(&mdev->dev);
>  	parent = mdev->parent;
>  	lockdep_assert_held(&parent->unreg_sem);
> @@ -122,12 +92,10 @@ static void mdev_device_remove_common(struct
> mdev_device *mdev)
> 
>  static int mdev_device_remove_cb(struct device *dev, void *data)
>  {
> -	if (dev_is_mdev(dev)) {
> -		struct mdev_device *mdev;
> +	struct mdev_device *mdev = mdev_from_dev(dev);
> 
> -		mdev = to_mdev_device(dev);
> +	if (mdev)
>  		mdev_device_remove_common(mdev);
> -	}
>  	return 0;
>  }
> 
> @@ -332,7 +300,7 @@ int mdev_device_create(struct kobject *kobj,
>  	if (ret)
>  		goto add_fail;
> 
> -	ret = mdev_create_sysfs_files(&mdev->dev, type);
> +	ret = mdev_create_sysfs_files(mdev, type);
>  	if (ret)
>  		goto sysfs_fail;
> 
> @@ -354,13 +322,11 @@ int mdev_device_create(struct kobject *kobj,
>  	return ret;
>  }
> 
> -int mdev_device_remove(struct device *dev)
> +int mdev_device_remove(struct mdev_device *mdev)
>  {
> -	struct mdev_device *mdev, *tmp;
> +	struct mdev_device *tmp;
>  	struct mdev_parent *parent;
> 
> -	mdev = to_mdev_device(dev);
> -
>  	mutex_lock(&mdev_list_lock);
>  	list_for_each_entry(tmp, &mdev_list, next) {
>  		if (tmp == mdev)
> @@ -390,24 +356,6 @@ int mdev_device_remove(struct device *dev)
>  	return 0;
>  }
> 
> -int mdev_set_iommu_device(struct device *dev, struct device
> *iommu_device)
> -{
> -	struct mdev_device *mdev = to_mdev_device(dev);
> -
> -	mdev->iommu_device = iommu_device;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(mdev_set_iommu_device);
> -
> -struct device *mdev_get_iommu_device(struct device *dev)
> -{
> -	struct mdev_device *mdev = to_mdev_device(dev);
> -
> -	return mdev->iommu_device;
> -}
> -EXPORT_SYMBOL(mdev_get_iommu_device);
> -
>  static int __init mdev_init(void)
>  {
>  	return mdev_bus_register();
> diff --git a/drivers/vfio/mdev/mdev_driver.c
> b/drivers/vfio/mdev/mdev_driver.c
> index 0d3223aee20b83..44c3ba7e56d923 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -48,7 +48,7 @@ static int mdev_probe(struct device *dev)
>  		return ret;
> 
>  	if (drv && drv->probe) {
> -		ret = drv->probe(dev);
> +		ret = drv->probe(mdev);
>  		if (ret)
>  			mdev_detach_iommu(mdev);
>  	}
> @@ -62,7 +62,7 @@ static int mdev_remove(struct device *dev)
>  	struct mdev_device *mdev = to_mdev_device(dev);
> 
>  	if (drv && drv->remove)
> -		drv->remove(dev);
> +		drv->remove(mdev);
> 
>  	mdev_detach_iommu(mdev);
> 
> diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> index 74c2e541146999..bb60ec4a8d9d21 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -24,23 +24,6 @@ struct mdev_parent {
>  	struct rw_semaphore unreg_sem;
>  };
> 
> -struct mdev_device {
> -	struct device dev;
> -	struct mdev_parent *parent;
> -	guid_t uuid;
> -	void *driver_data;
> -	struct list_head next;
> -	struct kobject *type_kobj;
> -	struct device *iommu_device;
> -	bool active;
> -};
> -
> -static inline struct mdev_device *to_mdev_device(struct device *dev)
> -{
> -	return container_of(dev, struct mdev_device, dev);
> -}
> -#define dev_is_mdev(d)		((d)->bus == &mdev_bus_type)
> -
>  struct mdev_type {
>  	struct kobject kobj;
>  	struct kobject *devices_kobj;
> @@ -57,11 +40,11 @@ struct mdev_type {
>  int  parent_create_sysfs_files(struct mdev_parent *parent);
>  void parent_remove_sysfs_files(struct mdev_parent *parent);
> 
> -int  mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
> -void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
> +int  mdev_create_sysfs_files(struct mdev_device *mdev, struct mdev_type
> *type);
> +void mdev_remove_sysfs_files(struct mdev_device *mdev, struct
> mdev_type *type);
> 
>  int  mdev_device_create(struct kobject *kobj,
>  			struct device *dev, const guid_t *uuid);
> -int  mdev_device_remove(struct device *dev);
> +int  mdev_device_remove(struct mdev_device *dev);
> 
>  #endif /* MDEV_PRIVATE_H */
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c
> b/drivers/vfio/mdev/mdev_sysfs.c
> index 917fd84c1c6f24..6a5450587b79e9 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -225,6 +225,7 @@ int parent_create_sysfs_files(struct mdev_parent
> *parent)
>  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>  			    const char *buf, size_t count)
>  {
> +	struct mdev_device *mdev = to_mdev_device(dev);
>  	unsigned long val;
> 
>  	if (kstrtoul(buf, 0, &val) < 0)
> @@ -233,7 +234,7 @@ static ssize_t remove_store(struct device *dev, struct
> device_attribute *attr,
>  	if (val && device_remove_file_self(dev, attr)) {
>  		int ret;
> 
> -		ret = mdev_device_remove(dev);
> +		ret = mdev_device_remove(mdev);
>  		if (ret)
>  			return ret;
>  	}
> @@ -248,34 +249,37 @@ static const struct attribute *mdev_device_attrs[] =
> {
>  	NULL,
>  };
> 
> -int  mdev_create_sysfs_files(struct device *dev, struct mdev_type *type)
> +int mdev_create_sysfs_files(struct mdev_device *mdev, struct mdev_type
> *type)
>  {
> +	struct kobject *kobj = &mdev->dev.kobj;
>  	int ret;
> 
> -	ret = sysfs_create_link(type->devices_kobj, &dev->kobj,
> dev_name(dev));
> +	ret = sysfs_create_link(type->devices_kobj, kobj, dev_name(&mdev-
> >dev));
>  	if (ret)
>  		return ret;
> 
> -	ret = sysfs_create_link(&dev->kobj, &type->kobj, "mdev_type");
> +	ret = sysfs_create_link(kobj, &type->kobj, "mdev_type");
>  	if (ret)
>  		goto type_link_failed;
> 
> -	ret = sysfs_create_files(&dev->kobj, mdev_device_attrs);
> +	ret = sysfs_create_files(kobj, mdev_device_attrs);
>  	if (ret)
>  		goto create_files_failed;
> 
>  	return ret;
> 
>  create_files_failed:
> -	sysfs_remove_link(&dev->kobj, "mdev_type");
> +	sysfs_remove_link(kobj, "mdev_type");
>  type_link_failed:
> -	sysfs_remove_link(type->devices_kobj, dev_name(dev));
> +	sysfs_remove_link(type->devices_kobj, dev_name(&mdev->dev));
>  	return ret;
>  }
> 
> -void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type)
> +void mdev_remove_sysfs_files(struct mdev_device *mdev, struct
> mdev_type *type)
>  {
> -	sysfs_remove_files(&dev->kobj, mdev_device_attrs);
> -	sysfs_remove_link(&dev->kobj, "mdev_type");
> -	sysfs_remove_link(type->devices_kobj, dev_name(dev));
> +	struct kobject *kobj = &mdev->dev.kobj;
> +
> +	sysfs_remove_files(kobj, mdev_device_attrs);
> +	sysfs_remove_link(kobj, "mdev_type");
> +	sysfs_remove_link(type->devices_kobj, dev_name(&mdev->dev));
>  }
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index ae7e322fbe3c26..91b7b8b9eb9cb8 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -124,9 +124,8 @@ static const struct vfio_device_ops
> vfio_mdev_dev_ops = {
>  	.request	= vfio_mdev_request,
>  };
> 
> -static int vfio_mdev_probe(struct device *dev)
> +static int vfio_mdev_probe(struct mdev_device *mdev)
>  {
> -	struct mdev_device *mdev = to_mdev_device(dev);
>  	struct vfio_device *vdev;
>  	int ret;
> 
> @@ -144,9 +143,9 @@ static int vfio_mdev_probe(struct device *dev)
>  	return 0;
>  }
> 
> -static void vfio_mdev_remove(struct device *dev)
> +static void vfio_mdev_remove(struct mdev_device *mdev)
>  {
> -	struct vfio_device *vdev = dev_get_drvdata(dev);
> +	struct vfio_device *vdev = dev_get_drvdata(&mdev->dev);
> 
>  	vfio_unregister_group_dev(vdev);
>  	kfree(vdev);
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> index 4bb162c1d649b3..90b45ff1d87a7b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1923,28 +1923,13 @@ static bool vfio_iommu_has_sw_msi(struct
> list_head *group_resv_regions,
>  	return ret;
>  }
> 
> -static struct device *vfio_mdev_get_iommu_device(struct device *dev)
> -{
> -	struct device *(*fn)(struct device *dev);
> -	struct device *iommu_device;
> -
> -	fn = symbol_get(mdev_get_iommu_device);
> -	if (fn) {
> -		iommu_device = fn(dev);
> -		symbol_put(mdev_get_iommu_device);
> -
> -		return iommu_device;
> -	}
> -
> -	return NULL;
> -}
> -
>  static int vfio_mdev_attach_domain(struct device *dev, void *data)
>  {
> +	struct mdev_device *mdev = to_mdev_device(dev);
>  	struct iommu_domain *domain = data;
>  	struct device *iommu_device;
> 
> -	iommu_device = vfio_mdev_get_iommu_device(dev);
> +	iommu_device = mdev_get_iommu_device(mdev);
>  	if (iommu_device) {
>  		if (iommu_dev_feature_enabled(iommu_device,
> IOMMU_DEV_FEAT_AUX))
>  			return iommu_aux_attach_device(domain,
> iommu_device);
> @@ -1957,10 +1942,11 @@ static int vfio_mdev_attach_domain(struct
> device *dev, void *data)
> 
>  static int vfio_mdev_detach_domain(struct device *dev, void *data)
>  {
> +	struct mdev_device *mdev = to_mdev_device(dev);
>  	struct iommu_domain *domain = data;
>  	struct device *iommu_device;
> 
> -	iommu_device = vfio_mdev_get_iommu_device(dev);
> +	iommu_device = mdev_get_iommu_device(mdev);
>  	if (iommu_device) {
>  		if (iommu_dev_feature_enabled(iommu_device,
> IOMMU_DEV_FEAT_AUX))
>  			iommu_aux_detach_device(domain, iommu_device);
> @@ -2008,9 +1994,10 @@ static bool vfio_bus_is_mdev(struct bus_type
> *bus)
> 
>  static int vfio_mdev_iommu_device(struct device *dev, void *data)
>  {
> +	struct mdev_device *mdev = to_mdev_device(dev);
>  	struct device **old = data, *new;
> 
> -	new = vfio_mdev_get_iommu_device(dev);
> +	new = mdev_get_iommu_device(mdev);
>  	if (!new || (*old && *old != new))
>  		return -EINVAL;
> 
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 27eb383cb95de0..52f7ea19dd0f56 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -10,7 +10,21 @@
>  #ifndef MDEV_H
>  #define MDEV_H
> 
> -struct mdev_device;
> +struct mdev_device {
> +	struct device dev;
> +	struct mdev_parent *parent;
> +	guid_t uuid;
> +	void *driver_data;
> +	struct list_head next;
> +	struct kobject *type_kobj;
> +	struct device *iommu_device;
> +	bool active;
> +};
> +
> +static inline struct mdev_device *to_mdev_device(struct device *dev)
> +{
> +	return container_of(dev, struct mdev_device, dev);
> +}
> 
>  /*
>   * Called by the parent device driver to set the device which represents
> @@ -19,12 +33,17 @@ struct mdev_device;
>   *
>   * @dev: the mediated device that iommu will isolate.
>   * @iommu_device: a pci device which represents the iommu for @dev.
> - *
> - * Return 0 for success, otherwise negative error value.
>   */
> -int mdev_set_iommu_device(struct device *dev, struct device
> *iommu_device);
> +static inline void mdev_set_iommu_device(struct mdev_device *mdev,
> +					 struct device *iommu_device)
> +{
> +	mdev->iommu_device = iommu_device;
> +}
> 
> -struct device *mdev_get_iommu_device(struct device *dev);
> +static inline struct device *mdev_get_iommu_device(struct mdev_device
> *mdev)
> +{
> +	return mdev->iommu_device;
> +}
> 
>  /**
>   * struct mdev_parent_ops - Structure to be registered for each parent
> device to
> @@ -126,16 +145,25 @@ struct mdev_type_attribute
> mdev_type_attr_##_name =		\
>   **/
>  struct mdev_driver {
>  	const char *name;
> -	int  (*probe)(struct device *dev);
> -	void (*remove)(struct device *dev);
> +	int (*probe)(struct mdev_device *dev);
> +	void (*remove)(struct mdev_device *dev);
>  	struct device_driver driver;
>  };
> 
>  #define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
> 
> -void *mdev_get_drvdata(struct mdev_device *mdev);
> -void mdev_set_drvdata(struct mdev_device *mdev, void *data);
> -const guid_t *mdev_uuid(struct mdev_device *mdev);
> +static inline void *mdev_get_drvdata(struct mdev_device *mdev)
> +{
> +	return mdev->driver_data;
> +}
> +static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
> +{
> +	mdev->driver_data = data;
> +}
> +static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
> +{
> +	return &mdev->uuid;
> +}
> 
>  extern struct bus_type mdev_bus_type;
> 
> @@ -146,7 +174,13 @@ int mdev_register_driver(struct mdev_driver *drv,
> struct module *owner);
>  void mdev_unregister_driver(struct mdev_driver *drv);
> 
>  struct device *mdev_parent_dev(struct mdev_device *mdev);
> -struct device *mdev_dev(struct mdev_device *mdev);
> -struct mdev_device *mdev_from_dev(struct device *dev);
> +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.31.0

RE: [PATCH 03/18] vfio/mdev: Simplify driver registration

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 24, 2021 1:55 AM
> 
> This is only done once, we don't need to generate code to initialize a
> structure stored in the ELF .data segment. Fill in the three required
> .driver members directly instead of copying data into them during
> mdev_register_driver().
> 
> Further the to_mdev_driver() function doesn't belong in a public header,
> just inline it into the two places that need it. Finally, we can now
> clearly see that 'drv' derived from dev->driver cannot be NULL, firstly
> because the driver core forbids it, and secondly because NULL won't pass
> through the container_of(). Remove the dead code.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

> ---
>  Documentation/driver-api/vfio-mediated-device.rst |  5 +----
>  drivers/vfio/mdev/mdev_driver.c                   | 15 +++++++--------
>  drivers/vfio/mdev/vfio_mdev.c                     |  8 ++++++--
>  include/linux/mdev.h                              |  6 +-----
>  4 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index c43c1dc3333373..1779b85f014e2f 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -98,13 +98,11 @@ structure to represent a mediated device's driver::
> 
>       /*
>        * struct mdev_driver [2] - Mediated device's driver
> -      * @name: driver name
>        * @probe: called when new device created
>        * @remove: called when device removed
>        * @driver: device driver structure
>        */
>       struct mdev_driver {
> -	     const char *name;
>  	     int  (*probe)  (struct mdev_device *dev);
>  	     void (*remove) (struct mdev_device *dev);
>  	     struct device_driver    driver;
> @@ -115,8 +113,7 @@ to register and unregister itself with the core driver:
> 
>  * Register::
> 
> -    extern int  mdev_register_driver(struct mdev_driver *drv,
> -				   struct module *owner);
> +    extern int  mdev_register_driver(struct mdev_driver *drv);
> 
>  * Unregister::
> 
> diff --git a/drivers/vfio/mdev/mdev_driver.c
> b/drivers/vfio/mdev/mdev_driver.c
> index 44c3ba7e56d923..041699571b7e55 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -39,7 +39,8 @@ static void mdev_detach_iommu(struct mdev_device
> *mdev)
> 
>  static int mdev_probe(struct device *dev)
>  {
> -	struct mdev_driver *drv = to_mdev_driver(dev->driver);
> +	struct mdev_driver *drv =
> +		container_of(dev->driver, struct mdev_driver, driver);
>  	struct mdev_device *mdev = to_mdev_device(dev);
>  	int ret;
> 
> @@ -47,7 +48,7 @@ static int mdev_probe(struct device *dev)
>  	if (ret)
>  		return ret;
> 
> -	if (drv && drv->probe) {
> +	if (drv->probe) {
>  		ret = drv->probe(mdev);
>  		if (ret)
>  			mdev_detach_iommu(mdev);
> @@ -58,10 +59,11 @@ static int mdev_probe(struct device *dev)
> 
>  static int mdev_remove(struct device *dev)
>  {
> -	struct mdev_driver *drv = to_mdev_driver(dev->driver);
> +	struct mdev_driver *drv =
> +		container_of(dev->driver, struct mdev_driver, driver);
>  	struct mdev_device *mdev = to_mdev_device(dev);
> 
> -	if (drv && drv->remove)
> +	if (drv->remove)
>  		drv->remove(mdev);
> 
>  	mdev_detach_iommu(mdev);
> @@ -79,16 +81,13 @@ EXPORT_SYMBOL_GPL(mdev_bus_type);
>  /**
>   * mdev_register_driver - register a new MDEV driver
>   * @drv: the driver to register
> - * @owner: module owner of driver to be registered
>   *
>   * Returns a negative value on error, otherwise 0.
>   **/
> -int mdev_register_driver(struct mdev_driver *drv, struct module *owner)
> +int mdev_register_driver(struct mdev_driver *drv)
>  {
>  	/* initialize common driver fields */
> -	drv->driver.name = drv->name;
>  	drv->driver.bus = &mdev_bus_type;
> -	drv->driver.owner = owner;
> 
>  	/* register with core */
>  	return driver_register(&drv->driver);
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index 91b7b8b9eb9cb8..cc9507ed85a181 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -152,14 +152,18 @@ static void vfio_mdev_remove(struct mdev_device
> *mdev)
>  }
> 
>  static struct mdev_driver vfio_mdev_driver = {
> -	.name	= "vfio_mdev",
> +	.driver = {
> +		.name = "vfio_mdev",
> +		.owner = THIS_MODULE,
> +		.mod_name = KBUILD_MODNAME,
> +	},
>  	.probe	= vfio_mdev_probe,
>  	.remove	= vfio_mdev_remove,
>  };
> 
>  static int __init vfio_mdev_init(void)
>  {
> -	return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE);
> +	return mdev_register_driver(&vfio_mdev_driver);
>  }
> 
>  static void __exit vfio_mdev_exit(void)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 52f7ea19dd0f56..cb771c712da0f4 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -137,21 +137,17 @@ struct mdev_type_attribute
> mdev_type_attr_##_name =		\
> 
>  /**
>   * struct mdev_driver - Mediated device driver
> - * @name: driver name
>   * @probe: called when new device created
>   * @remove: called when device removed
>   * @driver: device driver structure
>   *
>   **/
>  struct mdev_driver {
> -	const char *name;
>  	int (*probe)(struct mdev_device *dev);
>  	void (*remove)(struct mdev_device *dev);
>  	struct device_driver driver;
>  };
> 
> -#define to_mdev_driver(drv)	container_of(drv, struct mdev_driver, driver)
> -
>  static inline void *mdev_get_drvdata(struct mdev_device *mdev)
>  {
>  	return mdev->driver_data;
> @@ -170,7 +166,7 @@ extern struct bus_type mdev_bus_type;
>  int mdev_register_device(struct device *dev, const struct mdev_parent_ops
> *ops);
>  void mdev_unregister_device(struct device *dev);
> 
> -int mdev_register_driver(struct mdev_driver *drv, struct module *owner);
> +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);
> --
> 2.31.0

Re: [PATCH 03/18] vfio/mdev: Simplify driver registration

[Jason Gunthorpe]

On Tue, Mar 23, 2021 at 08:14:15PM +0100, Christoph Hellwig wrote:
> >  static struct mdev_driver vfio_mdev_driver = {
> > +	.driver = {
> > +		.name = "vfio_mdev",
> > +		.owner = THIS_MODULE,
> > +		.mod_name = KBUILD_MODNAME,
> > +	},
> 
> What is the mod_name initialization for?  

It is usually hidden and works like this:

 /* pci_register_driver() must be a macro so KBUILD_MODNAME can be expanded */
 #define pci_register_driver(driver)		\
 	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
 
 int __pci_register_driver(struct pci_driver *drv, struct module *owner,
 			  const char *mod_name)
 {
	drv->driver.owner = owner;
  	drv->driver.mod_name = mod_name;

> I've not really seen that in anywere else, and the only user seems
> to be module_add_driver for a rather odd case we shouldn't hit here.

vfio_mdev could be compiled built in? 

AFAICT it handles the case where THIS_MODULE==NULL so we still need to
create sysfs links to the built in module.

If it is left NULL then a few sysfs files go missing for the built in
mode but no harm done?

I think it is correct to have it

Thanks,
Jason

Re: [PATCH 03/18] vfio/mdev: Simplify driver registration

[Christoph Hellwig]

On Fri, Mar 26, 2021 at 09:10:48AM -0300, Jason Gunthorpe wrote:
> It is usually hidden and works like this:
> 
>  /* pci_register_driver() must be a macro so KBUILD_MODNAME can be expanded */
>  #define pci_register_driver(driver)		\
>  	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>  
>  int __pci_register_driver(struct pci_driver *drv, struct module *owner,
>  			  const char *mod_name)
>  {
> 	drv->driver.owner = owner;
>   	drv->driver.mod_name = mod_name;

Indeed, there seem to be about two handful of instance of that.

> > I've not really seen that in anywere else, and the only user seems
> > to be module_add_driver for a rather odd case we shouldn't hit here.
> 
> vfio_mdev could be compiled built in? 
> 
> AFAICT it handles the case where THIS_MODULE==NULL so we still need to
> create sysfs links to the built in module.
> 
> If it is left NULL then a few sysfs files go missing for the built in
> mode but no harm done?

Yes, it seems to be needed for a few driver-specific files.  So it looks
ok, even if rather unexpected.

Re: [PATCH 02/18] vfio/mdev: Add missing typesafety around mdev_device

[Cornelia Huck]

On Tue, 23 Mar 2021 14:55:19 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> The mdev API should accept and pass a 'struct mdev_device *' in all
> places, not pass a 'struct device *' and cast it internally with
> to_mdev_device(). Particularly in its struct mdev_driver functions, the
> whole point of a bus's struct device_driver wrapper is to provide type
> safety compared to the default struct device_driver.
> 
> Further, the driver core standard is for bus drivers to expose their
> device structure in their public headers that can be used with
> container_of() inlines and '&foo->dev' to go between the class levels, and
> '&foo->dev' to be used with dev_err/etc driver core helper functions. Move
> 'struct mdev_device' to mdev.h
> 
> Once done this allows moving some one instruction exported functions to
> static inlines, which in turns allows removing one of the two grotesque
> symbol_get()'s related to mdev in the core code.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  .../driver-api/vfio-mediated-device.rst       |  4 +-
>  drivers/vfio/mdev/mdev_core.c                 | 64 ++-----------------
>  drivers/vfio/mdev/mdev_driver.c               |  4 +-
>  drivers/vfio/mdev/mdev_private.h              | 23 +------
>  drivers/vfio/mdev/mdev_sysfs.c                | 26 ++++----
>  drivers/vfio/mdev/vfio_mdev.c                 |  7 +-
>  drivers/vfio/vfio_iommu_type1.c               | 25 ++------
>  include/linux/mdev.h                          | 58 +++++++++++++----
>  8 files changed, 83 insertions(+), 128 deletions(-)

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

Re: [PATCH 03/18] vfio/mdev: Simplify driver registration

[Cornelia Huck]

On Tue, 23 Mar 2021 14:55:20 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> This is only done once, we don't need to generate code to initialize a
> structure stored in the ELF .data segment. Fill in the three required
> .driver members directly instead of copying data into them during
> mdev_register_driver().
> 
> Further the to_mdev_driver() function doesn't belong in a public header,
> just inline it into the two places that need it. Finally, we can now
> clearly see that 'drv' derived from dev->driver cannot be NULL, firstly
> because the driver core forbids it, and secondly because NULL won't pass
> through the container_of(). Remove the dead code.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  Documentation/driver-api/vfio-mediated-device.rst |  5 +----
>  drivers/vfio/mdev/mdev_driver.c                   | 15 +++++++--------
>  drivers/vfio/mdev/vfio_mdev.c                     |  8 ++++++--
>  include/linux/mdev.h                              |  6 +-----
>  4 files changed, 15 insertions(+), 19 deletions(-)

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