Re: [PATCH 04/13] vfio/mdev: simplify mdev_type handling

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 28 Jun 2022 07:14:26 +0200
Christoph Hellwig <hch@lst.de> wrote:
...
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 5c828556cefd7..cea8113d2200e 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -131,6 +131,11 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
>  	if (!gvt->types)
>  		return -ENOMEM;
>  
> +	gvt->mdev_types = kcalloc(num_types, sizeof(*gvt->mdev_types),
> +			     GFP_KERNEL);
> +	if (!gvt->mdev_types)
> +		goto out_free_types;
> +
>  	min_low = MB_TO_BYTES(32);
>  	for (i = 0; i < num_types; ++i) {
>  		if (low_avail / vgpu_types[i].low_mm == 0)
> @@ -142,7 +147,7 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
>  
>  		if (vgpu_types[i].weight < 1 ||
>  					vgpu_types[i].weight > VGPU_MAX_WEIGHT)
> -			goto out_free_types;
> +			goto out_free_mdev_types;
>  
>  		gvt->types[i].weight = vgpu_types[i].weight;
>  		gvt->types[i].resolution = vgpu_types[i].edid;
> @@ -150,24 +155,28 @@ int intel_gvt_init_vgpu_types(struct intel_gvt *gvt)
>  						   high_avail / vgpu_types[i].high_mm);
>  
>  		if (GRAPHICS_VER(gvt->gt->i915) == 8)
> -			sprintf(gvt->types[i].name, "GVTg_V4_%s",
> +			sprintf(gvt->types[i].type.sysfs_name, "GVTg_V4_%s",
>  				vgpu_types[i].name);
>  		else if (GRAPHICS_VER(gvt->gt->i915) == 9)
> -			sprintf(gvt->types[i].name, "GVTg_V5_%s",
> +			sprintf(gvt->types[i].type.sysfs_name, "GVTg_V5_%s",
>  				vgpu_types[i].name);

Nit, sysfs_name is an arbitrary size, shouldn't we use snprintf() here
to make a good example?

>  		gvt_dbg_core("type[%d]: %s avail %u low %u high %u fence %u weight %u res %s\n",
> -			     i, gvt->types[i].name,
> +			     i, gvt->types[i].type.sysfs_name,
>  			     gvt->types[i].avail_instance,
>  			     gvt->types[i].low_gm_size,
>  			     gvt->types[i].high_gm_size, gvt->types[i].fence,
>  			     gvt->types[i].weight,
>  			     vgpu_edid_str(gvt->types[i].resolution));
> +
> +		gvt->mdev_types[i] = &gvt->types[i].type;
>  	}
>  
>  	gvt->num_types = i;
>  	return 0;
>  
> +out_free_mdev_types:
> +	kfree(gvt->mdev_types);
>  out_free_types:
>  	kfree(gvt->types);
>  	return -EINVAL;
...
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 9192a21085ce4..25b8d42a522ac 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,13 +644,16 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>  	},
>  	.probe = vfio_ccw_mdev_probe,
>  	.remove = vfio_ccw_mdev_remove,
> -	.supported_type_groups  = mdev_type_groups,
> +	.types_attrs = mdev_types_attrs,
>  };
>  
>  int vfio_ccw_mdev_reg(struct subchannel *sch)
>  {
> +	sprintf(sch->mdev_type.sysfs_name, "io");

Here too, but this gets randomly changed to an strcat() in patch 09/
and pretty_name is added in 10/, also with an strcat().  Staying with
snprintf() seems easier to get both overflow and termination.

> +	sch->mdev_types[0] = &sch->mdev_type;
>  	return mdev_register_parent(&sch->parent, &sch->dev,
> -				    &vfio_ccw_mdev_driver);
> +				    &vfio_ccw_mdev_driver, sch->mdev_types,
> +				    1);
>  }
>  
>  void vfio_ccw_mdev_unreg(struct subchannel *sch)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 834945150dc9f..ff25858b2ebbe 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,7 @@ 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,
> +	.types_attrs = vfio_ap_mdev_type_attrs,
>  };
>  
>  int vfio_ap_mdev_register(void)
> @@ -1485,8 +1475,11 @@ int vfio_ap_mdev_register(void)
>  	if (ret)
>  		return ret;
>  
> +	strcpy(matrix_dev->mdev_type.sysfs_name, VFIO_AP_MDEV_TYPE_HWVIRT);

And then this might as well be an snprintf() as well too.

Series looks good to me otherwise, hopefully the mdev driver owners
will add some acks.  Thanks,

Alex