On Tue, 28 Jun 2022 07:14:26 +0200
Christoph Hellwig <hch@xxxxxx> 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
