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

[Kirti Wankhede <kwankhede@xxxxxxxxxx>]

<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);