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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
