Re: [PATCH v2 2/3] vdpa: Add a device object for vdpa management device

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 16, 2022 at 02:03:41PM +0800, Xie Yongji wrote:
> Introduce a device object for vdpa management device to control
> its lifecycle. And the device name will be used to match
> VDPA_ATTR_MGMTDEV_DEV_NAME field of netlink message rather than
> using parent device name.
> 
> With this patch applied, drivers should use vdpa_mgmtdev_alloc()
> or _vdpa_mgmtdev_alloc() to allocate a vDPA management device
> before calling vdpa_mgmtdev_register(). And some buggy empty
> release function can also be removed from the driver codes.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c      | 11 ++--
>  drivers/vdpa/mlx5/net/mlx5_vnet.c    | 11 ++--
>  drivers/vdpa/vdpa.c                  | 92 ++++++++++++++++++++++++----
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 39 ++++--------
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 46 +++++---------
>  drivers/vdpa/vdpa_user/vduse_dev.c   | 38 ++++--------
>  include/linux/vdpa.h                 | 38 +++++++++++-
>  7 files changed, 168 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 4366320fb68d..d4087c37cfdf 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -821,10 +821,11 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	u32 dev_type;
>  	int ret;
>  
> -	ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
> -	if (!ifcvf_mgmt_dev) {
> +	ifcvf_mgmt_dev = vdpa_mgmtdev_alloc(struct ifcvf_vdpa_mgmt_dev,
> +					    mdev, dev_name(dev), dev);
> +	if (IS_ERR(ifcvf_mgmt_dev)) {
>  		IFCVF_ERR(pdev, "Failed to alloc memory for the vDPA management device\n");
> -		return -ENOMEM;
> +		return PTR_ERR(ifcvf_mgmt_dev);
>  	}
>  
>  	dev_type = get_dev_type(pdev);
> @@ -842,7 +843,6 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	}
>  
>  	ifcvf_mgmt_dev->mdev.ops = &ifcvf_vdpa_mgmt_dev_ops;
> -	ifcvf_mgmt_dev->mdev.device = dev;
>  	ifcvf_mgmt_dev->pdev = pdev;
>  
>  	ret = pcim_enable_device(pdev);
> @@ -883,7 +883,7 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return 0;
>  
>  err:
> -	kfree(ifcvf_mgmt_dev);
> +	put_device(&ifcvf_mgmt_dev->mdev.device);
>  	return ret;
>  }
>  
> @@ -893,7 +893,6 @@ static void ifcvf_remove(struct pci_dev *pdev)
>  
>  	ifcvf_mgmt_dev = pci_get_drvdata(pdev);
>  	vdpa_mgmtdev_unregister(&ifcvf_mgmt_dev->mdev);
> -	kfree(ifcvf_mgmt_dev);
>  }
>  
>  static struct pci_device_id ifcvf_pci_ids[] = {
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 79001301b383..3a88609dcf13 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2828,12 +2828,12 @@ static int mlx5v_probe(struct auxiliary_device *adev,
>  	struct mlx5_vdpa_mgmtdev *mgtdev;
>  	int err;
>  
> -	mgtdev = kzalloc(sizeof(*mgtdev), GFP_KERNEL);
> -	if (!mgtdev)
> -		return -ENOMEM;
> +	mgtdev = vdpa_mgmtdev_alloc(struct mlx5_vdpa_mgmtdev, mgtdev,
> +				    dev_name(mdev->device), mdev->device);
> +	if (IS_ERR(mgtdev))
> +		return PTR_ERR(mgtdev);
>  
>  	mgtdev->mgtdev.ops = &mdev_ops;
> -	mgtdev->mgtdev.device = mdev->device;
>  	mgtdev->mgtdev.id_table = id_table;
>  	mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
>  					  BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP) |
> @@ -2852,7 +2852,7 @@ static int mlx5v_probe(struct auxiliary_device *adev,
>  	return 0;
>  
>  reg_err:
> -	kfree(mgtdev);
> +	put_device(&mgtdev->mgtdev.device);
>  	return err;
>  }
>  
> @@ -2862,7 +2862,6 @@ static void mlx5v_remove(struct auxiliary_device *adev)
>  
>  	mgtdev = auxiliary_get_drvdata(adev);
>  	vdpa_mgmtdev_unregister(&mgtdev->mgtdev);
> -	kfree(mgtdev);
>  }
>  
>  static const struct auxiliary_device_id mlx5v_id_table[] = {
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..3d3f98dd2bb6 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -337,21 +337,80 @@ void vdpa_unregister_driver(struct vdpa_driver *drv)
>  }
>  EXPORT_SYMBOL_GPL(vdpa_unregister_driver);
>  
> +static inline struct vdpa_mgmt_dev *to_vdpa_mgmt_dev(struct device *dev)
> +{
> +	return container_of(dev, struct vdpa_mgmt_dev, device);
> +}
> +
> +static void vdpa_mgmtdev_release(struct device *dev)
> +{
> +	kfree(to_vdpa_mgmt_dev(dev));
> +}
> +
>  /**
> - * vdpa_mgmtdev_register - register a vdpa management device
> + * __vdpa_mgmtdev_alloc - allocate and initilaize a vDPA management device

initialize


> + * @name: name of the vdpa management device
> + * @parent: the parent device; optional
> + * @size: size of the data structure that might contain private datia
> + *
> + * Driver should use vdpa_mgmtdev_alloc() or _vdpa_mgmtdev_alloc() wrapper
> + * macro instead of using this directly.

-> do not call this directly, use vdpa_mgmtdev_alloc or _vdpa_mgmtdev_alloc
wrappers instead.

>   *
> + * Return: Return allocated data structure or ERR_PTR upon error
> + */
> +struct vdpa_mgmt_dev *__vdpa_mgmtdev_alloc(const char *name,
> +					   struct device *parent,
> +					   size_t size)
> +{
> +	struct vdpa_mgmt_dev *dev;
> +	int ret = -EINVAL;
> +
> +	if (!name)
> +		goto err;
> +
> +	ret = -ENOMEM;

well it seems this is always ENOMEM, you can just do
	return ERR_PTR(-ENOMEM);

> +	dev = kzalloc(size, GFP_KERNEL);
> +	if (!dev)
> +		goto err;
> +
> +	INIT_LIST_HEAD(&dev->list);
> +	dev->device.parent = parent;
> +	dev->device.release = vdpa_mgmtdev_release;
> +	ret = dev_set_name(&dev->device, "%s", name);
> +	if (ret)
> +		goto err_name;
> +
> +	device_initialize(&dev->device);
> +	return dev;
> +err_name:
> +	kfree(dev);
> +err:
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(__vdpa_mgmtdev_alloc);
> +
> +/**
> + * vdpa_mgmtdev_register - register a vdpa management device
>   * @mdev: Pointer to vdpa management device
> + *
>   * vdpa_mgmtdev_register() register a vdpa management device which supports

btw repeating vdpa_mgmtdev_register() is not useful. just start with
Register

> - * vdpa device management.
> - * Return: Returns 0 on success or failure when required callback ops are not
> - *         initialized.
> + * vdpa device management. Caller must have a succeed call of vdpa_mgmtdev_alloc()
> + * or _vdpa_mgmtdev_alloc() before.

->
@mdev: Pointer to a vdpa management device, received from a previous
	successful call to vdpa_mgmtdev_alloc or _vdpa_mgmtdev_alloc.

> + *
> + * Return: Returns 0 on success or failure when device is failed to be added


is failed to be added -> on failure to add the device

> + *         or required callback ops are not initialized.
>   */
>  int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev)
>  {
> -	if (!mdev->device || !mdev->ops || !mdev->ops->dev_add || !mdev->ops->dev_del)
> +	int ret;
> +
> +	if (!mdev->ops || !mdev->ops->dev_add || !mdev->ops->dev_del)
>  		return -EINVAL;
>  
> -	INIT_LIST_HEAD(&mdev->list);
> +	ret = device_add(&mdev->device);
> +	if (ret)
> +		return ret;
> +
>  	mutex_lock(&vdpa_dev_mutex);
>  	list_add_tail(&mdev->list, &mdev_head);
>  	mutex_unlock(&vdpa_dev_mutex);
> @@ -379,6 +438,8 @@ void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev)
>  	bus_for_each_dev(&vdpa_bus, NULL, mdev, vdpa_match_remove);
>  
>  	mutex_unlock(&vdpa_dev_mutex);
> +
> +	device_unregister(&mdev->device);
>  }
>  EXPORT_SYMBOL_GPL(vdpa_mgmtdev_unregister);
>  
> @@ -432,17 +493,20 @@ EXPORT_SYMBOL_GPL(vdpa_set_config);
>  static bool mgmtdev_handle_match(const struct vdpa_mgmt_dev *mdev,
>  				 const char *busname, const char *devname)
>  {
> +	struct bus_type *bus = mdev->device.parent ?
> +				mdev->device.parent->bus : NULL;
> +
>  	/* Bus name is optional for simulated management device, so ignore the
>  	 * device with bus if bus attribute is provided.
>  	 */
> -	if ((busname && !mdev->device->bus) || (!busname && mdev->device->bus))
> +	if ((busname && !bus) || (!busname && bus))
>  		return false;
>  
> -	if (!busname && strcmp(dev_name(mdev->device), devname) == 0)
> +	if (!busname && strcmp(dev_name(&mdev->device), devname) == 0)
>  		return true;
>  
> -	if (busname && (strcmp(mdev->device->bus->name, busname) == 0) &&
> -	    (strcmp(dev_name(mdev->device), devname) == 0))
> +	if (busname && (strcmp(bus->name, busname) == 0) &&
> +	    (strcmp(dev_name(&mdev->device), devname) == 0))
>  		return true;
>  
>  	return false;
> @@ -469,10 +533,12 @@ static struct vdpa_mgmt_dev *vdpa_mgmtdev_get_from_attr(struct nlattr **attrs)
>  
>  static int vdpa_nl_mgmtdev_handle_fill(struct sk_buff *msg, const struct vdpa_mgmt_dev *mdev)
>  {
> -	if (mdev->device->bus &&
> -	    nla_put_string(msg, VDPA_ATTR_MGMTDEV_BUS_NAME, mdev->device->bus->name))
> +	struct bus_type *bus = mdev->device.parent ?
> +				mdev->device.parent->bus : NULL;
> +
> +	if (bus && nla_put_string(msg, VDPA_ATTR_MGMTDEV_BUS_NAME, bus->name))
>  		return -EMSGSIZE;
> -	if (nla_put_string(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, dev_name(mdev->device)))
> +	if (nla_put_string(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, dev_name(&mdev->device)))
>  		return -EMSGSIZE;
>  	return 0;
>  }
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index 42d401d43911..e48b061b6c1b 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -239,15 +239,6 @@ static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
>  	blk_config->blk_size = cpu_to_vdpasim32(vdpasim, SECTOR_SIZE);
>  }
>  
> -static void vdpasim_blk_mgmtdev_release(struct device *dev)
> -{
> -}
> -
> -static struct device vdpasim_blk_mgmtdev = {
> -	.init_name = "vdpasim_blk",
> -	.release = vdpasim_blk_mgmtdev_release,
> -};
> -
>  static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  			       const struct vdpa_dev_set_config *config)
>  {
> @@ -298,35 +289,31 @@ static struct virtio_device_id id_table[] = {
>  	{ 0 },
>  };
>  
> -static struct vdpa_mgmt_dev mgmt_dev = {
> -	.device = &vdpasim_blk_mgmtdev,
> -	.id_table = id_table,
> -	.ops = &vdpasim_blk_mgmtdev_ops,
> -};
> +static struct vdpa_mgmt_dev *mgmt_dev;
>  
>  static int __init vdpasim_blk_init(void)
>  {
>  	int ret;
>  
> -	ret = device_register(&vdpasim_blk_mgmtdev);
> -	if (ret)
> -		return ret;
> +	mgmt_dev = _vdpa_mgmtdev_alloc("vdpasim_blk", NULL);
> +	if (IS_ERR(mgmt_dev))
> +		return PTR_ERR(mgmt_dev);
>  
> -	ret = vdpa_mgmtdev_register(&mgmt_dev);
> -	if (ret)
> -		goto parent_err;
> +	mgmt_dev->id_table = id_table;
> +	mgmt_dev->ops = &vdpasim_blk_mgmtdev_ops;
>  
> -	return 0;
> +	ret = vdpa_mgmtdev_register(mgmt_dev);
> +	if (ret) {
> +		put_device(&mgmt_dev->device);
> +		return ret;
> +	}
>  
> -parent_err:
> -	device_unregister(&vdpasim_blk_mgmtdev);
> -	return ret;
> +	return 0;
>  }
>  
>  static void __exit vdpasim_blk_exit(void)
>  {
> -	vdpa_mgmtdev_unregister(&mgmt_dev);
> -	device_unregister(&vdpasim_blk_mgmtdev);
> +	vdpa_mgmtdev_unregister(mgmt_dev);
>  }
>  
>  module_init(vdpasim_blk_init)
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index d5324f6fd8c7..1adb26f8ac32 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -124,15 +124,6 @@ static void vdpasim_net_setup_config(struct vdpasim *vdpasim,
>  		vio_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
>  }
>  
> -static void vdpasim_net_mgmtdev_release(struct device *dev)
> -{
> -}
> -
> -static struct device vdpasim_net_mgmtdev = {
> -	.init_name = "vdpasim_net",
> -	.release = vdpasim_net_mgmtdev_release,
> -};
> -
>  static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  			       const struct vdpa_dev_set_config *config)
>  {
> @@ -185,38 +176,35 @@ static struct virtio_device_id id_table[] = {
>  	{ 0 },
>  };
>  
> -static struct vdpa_mgmt_dev mgmt_dev = {
> -	.device = &vdpasim_net_mgmtdev,
> -	.id_table = id_table,
> -	.ops = &vdpasim_net_mgmtdev_ops,
> -	.config_attr_mask = (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR |
> -			     1 << VDPA_ATTR_DEV_NET_CFG_MTU),
> -	.max_supported_vqs = VDPASIM_NET_VQ_NUM,
> -	.supported_features = VDPASIM_NET_FEATURES,
> -};
> +static struct vdpa_mgmt_dev *mgmt_dev;
>  
>  static int __init vdpasim_net_init(void)
>  {
>  	int ret;
>  
> -	ret = device_register(&vdpasim_net_mgmtdev);
> -	if (ret)
> +	mgmt_dev = _vdpa_mgmtdev_alloc("vdpasim_net", NULL);
> +	if (IS_ERR(mgmt_dev))
> +		return PTR_ERR(mgmt_dev);
> +
> +	mgmt_dev->id_table = id_table;
> +	mgmt_dev->ops = &vdpasim_net_mgmtdev_ops;
> +	mgmt_dev->config_attr_mask = (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR |
> +				      1 << VDPA_ATTR_DEV_NET_CFG_MTU);
> +	mgmt_dev->max_supported_vqs = VDPASIM_NET_VQ_NUM;
> +	mgmt_dev->supported_features = VDPASIM_NET_FEATURES;
> +
> +	ret = vdpa_mgmtdev_register(mgmt_dev);
> +	if (ret) {
> +		put_device(&mgmt_dev->device);
>  		return ret;
> +	}
>  
> -	ret = vdpa_mgmtdev_register(&mgmt_dev);
> -	if (ret)
> -		goto parent_err;
>  	return 0;
> -
> -parent_err:
> -	device_unregister(&vdpasim_net_mgmtdev);
> -	return ret;
>  }
>  
>  static void __exit vdpasim_net_exit(void)
>  {
> -	vdpa_mgmtdev_unregister(&mgmt_dev);
> -	device_unregister(&vdpasim_net_mgmtdev);
> +	vdpa_mgmtdev_unregister(mgmt_dev);
>  }
>  
>  module_init(vdpasim_net_init);
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index d3bf55a58cd2..aa4b5f6fe674 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1475,15 +1475,6 @@ static char *vduse_devnode(struct device *dev, umode_t *mode)
>  	return kasprintf(GFP_KERNEL, "vduse/%s", dev_name(dev));
>  }
>  
> -static void vduse_mgmtdev_release(struct device *dev)
> -{
> -}
> -
> -static struct device vduse_mgmtdev = {
> -	.init_name = "vduse",
> -	.release = vduse_mgmtdev_release,
> -};
> -
>  static int vduse_dev_init_vdpa(struct vduse_dev *dev,
>  			       struct vdpa_mgmt_dev *mdev, const char *name)
>  {
> @@ -1554,34 +1545,31 @@ static struct virtio_device_id id_table[] = {
>  	{ 0 },
>  };
>  
> -static struct vdpa_mgmt_dev mgmt_dev = {
> -	.device = &vduse_mgmtdev,
> -	.id_table = id_table,
> -	.ops = &vdpa_dev_mgmtdev_ops,
> -};
> +static struct vdpa_mgmt_dev *mgmt_dev;
>  
>  static int vduse_mgmtdev_init(void)
>  {
>  	int ret;
>  
> -	ret = device_register(&vduse_mgmtdev);
> -	if (ret)
> -		return ret;
> +	mgmt_dev = _vdpa_mgmtdev_alloc("vduse", NULL);
> +	if (IS_ERR(mgmt_dev))
> +		return PTR_ERR(mgmt_dev);
>  
> -	ret = vdpa_mgmtdev_register(&mgmt_dev);
> -	if (ret)
> -		goto err;
> +	mgmt_dev->id_table = id_table;
> +	mgmt_dev->ops = &vdpa_dev_mgmtdev_ops;
> +
> +	ret = vdpa_mgmtdev_register(mgmt_dev);
> +	if (ret) {
> +		put_device(&mgmt_dev->device);
> +		return ret;
> +	}
>  
>  	return 0;
> -err:
> -	device_unregister(&vduse_mgmtdev);
> -	return ret;
>  }
>  
>  static void vduse_mgmtdev_exit(void)
>  {
> -	vdpa_mgmtdev_unregister(&mgmt_dev);
> -	device_unregister(&vduse_mgmtdev);
> +	vdpa_mgmtdev_unregister(mgmt_dev);
>  }
>  
>  static int vduse_init(void)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 8943a209202e..bf82a239a17b 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -453,7 +453,7 @@ struct vdpa_mgmtdev_ops {
>  
>  /**
>   * struct vdpa_mgmt_dev - vdpa management device
> - * @device: Management parent device
> + * @device: Management device
>   * @ops: operations supported by management device
>   * @id_table: Pointer to device id table of supported ids
>   * @config_attr_mask: bit mask of attributes of type enum vdpa_attr that
> @@ -461,7 +461,7 @@ struct vdpa_mgmtdev_ops {
>   * @list: list entry
>   */
>  struct vdpa_mgmt_dev {
> -	struct device *device;
> +	struct device device;
>  	const struct vdpa_mgmtdev_ops *ops;
>  	const struct virtio_device_id *id_table;
>  	u64 config_attr_mask;
> @@ -470,6 +470,40 @@ struct vdpa_mgmt_dev {
>  	u32 max_supported_vqs;
>  };
>  
> +struct vdpa_mgmt_dev *__vdpa_mgmtdev_alloc(const char *name,
> +					   struct device *parent,
> +					   size_t size);
> +
> +/**
> + * vdpa_mgmtdev_alloc - allocate and initilaize a vDPA management device with private data
> + *
> + * @dev_struct: the type of the parent structure
> + * @member: the name of struct vdpa_mgmt_dev within the @dev_struct
> + * @name: name of the vdpa management device
> + * @parent: the parent device
> + *
> + * Return allocated data structure or ERR_PTR upon error

Returns

> + */
> +#define vdpa_mgmtdev_alloc(dev_struct, member, name, parent) \
> +			container_of(__vdpa_mgmtdev_alloc( \
> +				     name, parent, \
> +				     sizeof(dev_struct) + \
> +				     BUILD_BUG_ON_ZERO(offsetof( \
> +				     dev_struct, member))), \
> +				     dev_struct, member)
> +
> +/**
> + * _vdpa_mgmtdev_alloc - allocate and initilaize a vDPA management device
> + *
> + * @name: name of the vdpa management device
> + * @parent: the parent device
> + *
> + * Return vdpa_mgmt_dev structure or ERR_PTR upon error


Returns

> + */
> +#define _vdpa_mgmtdev_alloc(name, parent) \
> +			__vdpa_mgmtdev_alloc(name, parent, \
> +					     sizeof(struct vdpa_mgmt_dev))
> +
>  int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev);
>  void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev);
>  
> -- 
> 2.20.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization