Re: [PATCH v2 1/3] vduse: Pass management device pointer to vduse_dev_init_vdpa()

Re: [PATCH v2 1/3] vduse: Pass management device pointer to vduse_dev_init_vdpa()

[Greg KH]

On Mon, May 16, 2022 at 02:03:40PM +0800, Xie Yongji wrote:
> Pass management device pointer from vdpa_dev_add() to
> vduse_dev_init_vdpa() rather than using the static
> variable directly.
> 
> No functional change.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 160e40d03084..d3bf55a58cd2 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1484,9 +1484,8 @@ static struct device vduse_mgmtdev = {
>  	.release = vduse_mgmtdev_release,
>  };
>  
> -static struct vdpa_mgmt_dev mgmt_dev;
> -
> -static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> +static int vduse_dev_init_vdpa(struct vduse_dev *dev,
> +			       struct vdpa_mgmt_dev *mdev, const char *name)
>  {
>  	struct vduse_vdpa *vdev;
>  	int ret;
> @@ -1509,7 +1508,7 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
>  	}
>  	set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops);
>  	vdev->vdpa.dma_dev = &vdev->vdpa.dev;
> -	vdev->vdpa.mdev = &mgmt_dev;
> +	vdev->vdpa.mdev = mdev;
>  
>  	return 0;
>  }
> @@ -1526,7 +1525,7 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  		mutex_unlock(&vduse_lock);
>  		return -EINVAL;
>  	}
> -	ret = vduse_dev_init_vdpa(dev, name);
> +	ret = vduse_dev_init_vdpa(dev, mdev, name);
>  	mutex_unlock(&vduse_lock);
>  	if (ret)
>  		return ret;
> -- 
> 2.20.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Greg KH]

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

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

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

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

[Jason Wang]

在 2022/5/16 14:03, Xie Yongji 写道:
> 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);


Just wonder if it's better to make vDPA device a child of the mgmt 
device instead of the PCI device?

(Currently we use PCI device as the parent of the vDPA device, or at 
least we can do this for the simulator which doesn't have a parent?)

Thanks


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

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

[Michael S. Tsirkin]

On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > 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);
> >
> >
> > Just wonder if it's better to make vDPA device a child of the mgmt
> > device instead of the PCI device?
> >
> > (Currently we use PCI device as the parent of the vDPA device, or at
> > least we can do this for the simulator which doesn't have a parent?)
> >
> 
> Make sense. I think we can do it for all vDPA drivers. Make sure the
> parent of the vDPA device is the vDPA management device.
> 
> Thanks,
> Yongji


that's an ABI change though isn't it? parent is exposed in sysfs,
right?

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

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

[Michael S. Tsirkin]

On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote:
> On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > > > 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);
> > > >
> > > >
> > > > Just wonder if it's better to make vDPA device a child of the mgmt
> > > > device instead of the PCI device?
> > > >
> > > > (Currently we use PCI device as the parent of the vDPA device, or at
> > > > least we can do this for the simulator which doesn't have a parent?)
> > > >
> > >
> > > Make sense. I think we can do it for all vDPA drivers. Make sure the
> > > parent of the vDPA device is the vDPA management device.
> > >
> > > Thanks,
> > > Yongji
> >
> >
> > that's an ABI change though isn't it? parent is exposed in sysfs,
> > right?
> >
> 
> Hmm...yes. So it looks like we can't change it, right?
> 
> Thanks,
> Yongji

Afraid so. a way to find the pci device already exists I think, right?

-- 
MST

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

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

[Greg KH]

On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote:
> On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote:
> > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > >
> > > > > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > > > > 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);
> > > > >
> > > > >
> > > > > Just wonder if it's better to make vDPA device a child of the mgmt
> > > > > device instead of the PCI device?
> > > > >
> > > > > (Currently we use PCI device as the parent of the vDPA device, or at
> > > > > least we can do this for the simulator which doesn't have a parent?)
> > > > >
> > > >
> > > > Make sense. I think we can do it for all vDPA drivers. Make sure the
> > > > parent of the vDPA device is the vDPA management device.
> > > >
> > > > Thanks,
> > > > Yongji
> > >
> > >
> > > that's an ABI change though isn't it? parent is exposed in sysfs,
> > > right?
> > >
> > 
> > Hmm...yes. So it looks like we can't change it, right?
> > 
> > Thanks,
> > Yongji
> 
> Afraid so. a way to find the pci device already exists I think, right?

You can change it, any tools should be going through the bus/device
links, not walking the sysfs tree directly, right?  That's what those
symlinks are for, in order to properly be able to enumerate different
device types.

A specific topology in sysfs should not ever be assumed to be static
over time, that's not an accurate representation of how the kernel
works.

So try it and see?  Odds are there are no tools that even care about
these devices, right?  Or is there?

thanks,

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

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

[Jason Wang]

On Mon, May 16, 2022 at 7:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote:
> > On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote:
> > > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> > > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > >
> > > > > > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > > > > > 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);
> > > > > >
> > > > > >
> > > > > > Just wonder if it's better to make vDPA device a child of the mgmt
> > > > > > device instead of the PCI device?
> > > > > >
> > > > > > (Currently we use PCI device as the parent of the vDPA device, or at
> > > > > > least we can do this for the simulator which doesn't have a parent?)
> > > > > >
> > > > >
> > > > > Make sense. I think we can do it for all vDPA drivers. Make sure the
> > > > > parent of the vDPA device is the vDPA management device.
> > > > >
> > > > > Thanks,
> > > > > Yongji
> > > >
> > > >
> > > > that's an ABI change though isn't it? parent is exposed in sysfs,
> > > > right?
> > > >
> > >
> > > Hmm...yes. So it looks like we can't change it, right?
> > >
> > > Thanks,
> > > Yongji
> >
> > Afraid so. a way to find the pci device already exists I think, right?
>
> You can change it, any tools should be going through the bus/device
> links, not walking the sysfs tree directly, right?  That's what those
> symlinks are for, in order to properly be able to enumerate different
> device types.
>
> A specific topology in sysfs should not ever be assumed to be static
> over time, that's not an accurate representation of how the kernel
> works.
>
> So try it and see?  Odds are there are no tools that even care about
> these devices, right?  Or is there?

I think there's no tool that depends on the sysfs tree now. The
management layer is only expected to talk to the management device via
vdpa(8) which is integrated in iproute2.

Thanks

>
> thanks,
>
> greg k-h
>

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

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

[Greg KH]

On Tue, May 17, 2022 at 02:29:38PM +0800, Jason Wang wrote:
> On Mon, May 16, 2022 at 7:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote:
> > > > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> > > > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > > > > > > 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);
> > > > > > >
> > > > > > >
> > > > > > > Just wonder if it's better to make vDPA device a child of the mgmt
> > > > > > > device instead of the PCI device?
> > > > > > >
> > > > > > > (Currently we use PCI device as the parent of the vDPA device, or at
> > > > > > > least we can do this for the simulator which doesn't have a parent?)
> > > > > > >
> > > > > >
> > > > > > Make sense. I think we can do it for all vDPA drivers. Make sure the
> > > > > > parent of the vDPA device is the vDPA management device.
> > > > > >
> > > > > > Thanks,
> > > > > > Yongji
> > > > >
> > > > >
> > > > > that's an ABI change though isn't it? parent is exposed in sysfs,
> > > > > right?
> > > > >
> > > >
> > > > Hmm...yes. So it looks like we can't change it, right?
> > > >
> > > > Thanks,
> > > > Yongji
> > >
> > > Afraid so. a way to find the pci device already exists I think, right?
> >
> > You can change it, any tools should be going through the bus/device
> > links, not walking the sysfs tree directly, right?  That's what those
> > symlinks are for, in order to properly be able to enumerate different
> > device types.
> >
> > A specific topology in sysfs should not ever be assumed to be static
> > over time, that's not an accurate representation of how the kernel
> > works.
> >
> > So try it and see?  Odds are there are no tools that even care about
> > these devices, right?  Or is there?
> 
> I think there's no tool that depends on the sysfs tree now. The
> management layer is only expected to talk to the management device via
> vdpa(8) which is integrated in iproute2.

Great, then change away!
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Parav Pandit via Virtualization]

> From: Xie Yongji <xieyongji@bytedance.com>
> Sent: Monday, May 16, 2022 2:04 AM
> 
> Introduce a device object for vdpa management device to control its
> lifecycle. 
Why is this needed?
What is the limitation of current approach that requires new device for mgmtdev?
The primary motivation is missing in the commit log here.

> 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.
What is the bug, can you please explain?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Parav Pandit via Virtualization]

Hi Greg, Yongji,

> From: Yongji Xie <xieyongji@bytedance.com>
> Sent: Tuesday, May 17, 2022 3:25 AM
> 
> On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> >
> > > From: Xie Yongji <xieyongji@bytedance.com>
> > > Sent: Monday, May 16, 2022 2:04 AM
> > >
> > > Introduce a device object for vdpa management device to control its
> > > lifecycle.
> > Why is this needed?
> > What is the limitation of current approach that requires new device for
> mgmtdev?
> > The primary motivation is missing in the commit log here.
> >
> 
> OK, I will add one. This patch actually comes from the discussion:
> 
> 
> https://www.spinics.net/lists/linux-virtualization/msg56371.html
> 
> The "vdpa_mgmt_dev" makes things a little confusing. Embedding the
> device struct in it to control its lifecycle simplifies the logic and makes the
> driver model clear.
> 
No. it doesn't.

vdpa_mgmt_dev is really the handle for all the netlink socket messages targeted.
It is not really a struct device.
We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing.

And regarding vduse_dev_release() and existing empty release function, they can be dynamically allocated.
This is because they are really the struct device.


> > > 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.
> > What is the bug, can you please explain?
> 
> I'm not sure if "buggy" is the right word. But the empty release function
> should be removed as mentioned in Documentation/core-api/kobject.rst.
> 
Sure. So, struct device in the vdpa_sim and vduse can allocate the struct device using kzalloc() and release callback can free it.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[gregkh]

On Tue, May 17, 2022 at 12:21:03PM +0000, Parav Pandit wrote:
> Hi Greg, Yongji,
> 
> > From: Yongji Xie <xieyongji@bytedance.com>
> > Sent: Tuesday, May 17, 2022 3:25 AM
> > 
> > On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > >
> > > > From: Xie Yongji <xieyongji@bytedance.com>
> > > > Sent: Monday, May 16, 2022 2:04 AM
> > > >
> > > > Introduce a device object for vdpa management device to control its
> > > > lifecycle.
> > > Why is this needed?
> > > What is the limitation of current approach that requires new device for
> > mgmtdev?
> > > The primary motivation is missing in the commit log here.
> > >
> > 
> > OK, I will add one. This patch actually comes from the discussion:
> > 
> > 
> > https://www.spinics.net/lists/linux-virtualization/msg56371.html
> > 
> > The "vdpa_mgmt_dev" makes things a little confusing. Embedding the
> > device struct in it to control its lifecycle simplifies the logic and makes the
> > driver model clear.
> > 
> No. it doesn't.
> 
> vdpa_mgmt_dev is really the handle for all the netlink socket messages targeted.
> It is not really a struct device.

Why can it not be one?  It has lifetime rules that must be followed, so
might as well use the built-in code to handle this, right?

What is wrong with it being a struct device?

> We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing.

Where is the "management" device if this is not that?

What does "handle" mean here?

> And regarding vduse_dev_release() and existing empty release function, they can be dynamically allocated.
> This is because they are really the struct device.

I do not understand this statement, sorry.

thanks,

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

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

[Parav Pandit via Virtualization]

> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Tuesday, May 17, 2022 9:54 AM
> 
> On Tue, May 17, 2022 at 12:21:03PM +0000, Parav Pandit wrote:
> > Hi Greg, Yongji,
> >
> > > From: Yongji Xie <xieyongji@bytedance.com>
> > > Sent: Tuesday, May 17, 2022 3:25 AM
> > >
> > > On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav@nvidia.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > From: Xie Yongji <xieyongji@bytedance.com>
> > > > > Sent: Monday, May 16, 2022 2:04 AM
> > > > >
> > > > > Introduce a device object for vdpa management device to control
> > > > > its lifecycle.
> > > > Why is this needed?
> > > > What is the limitation of current approach that requires new
> > > > device for
> > > mgmtdev?
> > > > The primary motivation is missing in the commit log here.
> > > >
> > >
> > > OK, I will add one. This patch actually comes from the discussion:
> > >
> > >
> > > https://www.spinics.net/lists/linux-virtualization/msg56371.html
> > >
> > > The "vdpa_mgmt_dev" makes things a little confusing. Embedding the
> > > device struct in it to control its lifecycle simplifies the logic
> > > and makes the driver model clear.
> > >
> > No. it doesn't.
> >
> > vdpa_mgmt_dev is really the handle for all the netlink socket messages
> targeted.
> > It is not really a struct device.
> 
> Why can it not be one?  It has lifetime rules that must be followed, so might
> as well use the built-in code to handle this, right?
> 
Lifetime rules are followed.
That is, during unloading of the driver for the struct device* dev, mgmtdev must be unregistered.
Following the exact mirror of creation.
I general before deleting the struct device *dev, its upper layer user mgmt. dev must be unregistered.

There will be zero user of this struct mgmt_dev, after invocation of current API vdpa_mgmtdev_unregister().
This is the onus on above unregister API.

Any cleanup cannot be differed until newly thought dev->release().

> What is wrong with it being a struct device?

I am not sure if its fully wrong.
First for this struct device, there is no real driver bind to it.

Secondly, it is simply an overkill without any visible use.
vdpa_mgmt_dev struct has only register/unregister life cycle.
Unregistration of it stops and destroy any outstanding operation on it.
Any code accessing mgmtdev post unregistration simply won't work.

Anything pending cannot be freed in the newly sighted release() callback.
Freeing anything in the release is too late because hw resources are destroy after unregister and before release().

Contrary to it, doing so will hide bugs because such struct will live longer now if done as struct device.

On a side note, these devices are in 500 to 1K range and expected to grow more.
Creating yet another struct device and linking it to its parent would result in additional sysfs links, udev event explosion for no absolute use or doesn't bring any object lifecycle changes.

vduse is a special case in the kernel, that has to create a dummy device because there is no real device (like pci) linked to it that can service vdpa commands.

> 
> > We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing.
> 
> Where is the "management" device if this is not that?
> 
> What does "handle" mean here?
It a handle to on which commands are arriving via socket from user space.
These commands are serviced by the struct device *dev.
DMA operation etc also happen on *dev.
You can think of it as unique handle to refer to the kernel like netdev->ifindex on netlink messages.

> 
> > And regarding vduse_dev_release() and existing empty release function,
> they can be dynamically allocated.
> > This is because they are really the struct device.
> 
> I do not understand this statement, sorry.

It was bad sentence, my bad.

What I wanted to say is, probably better explained with real patch below.
In context of [1], struct vduse_mgmtdev empty release function can be avoided by below inline patch [2].

[1] https://www.spinics.net/lists/linux-virtualization/msg56371.html

[2] patch:

From f87d557fe939a1632473dd11526a87301adbab8d Mon Sep 17 00:00:00 2001
From: Parav Pandit <parav@nvidia.com>
Date: Wed, 18 May 2022 01:22:21 +0300
Subject: [PATCH] vduse: Tie vduse mgmtdev and its device

vduse management device is not backed by any real device such as PCI. Hence it
doesn't have any parent device linked to it.

Kernel driver model in [1] suggests avoiding an empty device release callback.

Hence tie the mgmtdevice object's life cycle to an allocate dummy struct
device instead of having it static.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/kobject.rst?h=v5.18-rc7#n284

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 60 ++++++++++++++++++------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index f85d1a08ed87..ebe272575fb8 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1475,16 +1475,12 @@ 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,
+struct vduse_mgmt_dev {
+	struct vdpa_mgmt_dev mgmt_dev;
+	struct device dev;
 };
 
-static struct vdpa_mgmt_dev mgmt_dev;
+static struct vduse_mgmt_dev *vduse_mgmt;
 
 static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
 {
@@ -1509,7 +1505,7 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
 	}
 	set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops);
 	vdev->vdpa.dma_dev = &vdev->vdpa.dev;
-	vdev->vdpa.mdev = &mgmt_dev;
+	vdev->vdpa.mdev = &vduse_mgmt->mgmt_dev;
 
 	return 0;
 }
@@ -1555,34 +1551,52 @@ 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 void vduse_mgmtdev_release(struct device *dev)
+{
+	struct vduse_mgmt_dev *mgmt_dev;
+
+	mgmt_dev = container_of(dev, struct vduse_mgmt_dev, dev);
+	kfree(mgmt_dev);
+}
 
 static int vduse_mgmtdev_init(void)
 {
 	int ret;
 
-	ret = device_register(&vduse_mgmtdev);
-	if (ret)
+	vduse_mgmt = kzalloc(sizeof(*vduse_mgmt), GFP_KERNEL);
+	if (!vduse_mgmt)
+		return -ENOMEM;
+
+	ret = dev_set_name(&vduse_mgmt->dev, "vduse-la");
+	if (ret) {
+		kfree(vduse_mgmt);
 		return ret;
+	}
 
-	ret = vdpa_mgmtdev_register(&mgmt_dev);
+	vduse_mgmt->dev.release = vduse_mgmtdev_release;
+
+	ret = device_register(&vduse_mgmt->dev);
 	if (ret)
-		goto err;
+		goto dev_reg_err;
 
-	return 0;
-err:
-	device_unregister(&vduse_mgmtdev);
+	vduse_mgmt->mgmt_dev.id_table = id_table;
+	vduse_mgmt->mgmt_dev.ops = &vdpa_dev_mgmtdev_ops;
+	vduse_mgmt->mgmt_dev.device = &vduse_mgmt->dev;
+	ret = vdpa_mgmtdev_register(&vduse_mgmt->mgmt_dev);
+	if (ret)
+		device_unregister(&vduse_mgmt->dev);
+
+	return ret;
+
+dev_reg_err:
+	put_device(&vduse_mgmt->dev);
 	return ret;
 }
 
 static void vduse_mgmtdev_exit(void)
 {
-	vdpa_mgmtdev_unregister(&mgmt_dev);
-	device_unregister(&vduse_mgmtdev);
+	vdpa_mgmtdev_unregister(&vduse_mgmt->mgmt_dev);
+	device_unregister(&vduse_mgmt->dev);
 }
 
 static int vduse_init(void)
-- 
2.26.2

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

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

[Jason Wang]

在 2022/5/18 07:03, Parav Pandit 写道:
>>> And regarding vduse_dev_release() and existing empty release function,
>> they can be dynamically allocated.
>>> This is because they are really the struct device.
>> I do not understand this statement, sorry.
> It was bad sentence, my bad.
>
> What I wanted to say is, probably better explained with real patch below.
> In context of [1], struct vduse_mgmtdev empty release function can be avoided by below inline patch [2].
>
> [1]https://www.spinics.net/lists/linux-virtualization/msg56371.html
>
> [2] patch:


Ok, if we go this way (looks more like mdev_parent). I think we need at 
least rename the vdpa_mgmt_device, maybe vdpa_parent (like mdev_parent)?

Thanks


>
>  From f87d557fe939a1632473dd11526a87301adbab8d Mon Sep 17 00:00:00 2001
> From: Parav Pandit<parav@nvidia.com>
> Date: Wed, 18 May 2022 01:22:21 +0300
> Subject: [PATCH] vduse: Tie vduse mgmtdev and its device
>
> vduse management device is not backed by any real device such as PCI. Hence it
> doesn't have any parent device linked to it.
>
> Kernel driver model in [1] suggests avoiding an empty device release callback.
>
> Hence tie the mgmtdevice object's life cycle to an allocate dummy struct
> device instead of having it static.
>
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/kobject.rst?h=v5.18-rc7#n284
>
> Signed-off-by: Parav Pandit<parav@nvidia.com>
> ---
>   drivers/vdpa/vdpa_user/vduse_dev.c | 60 ++++++++++++++++++------------
>   1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f85d1a08ed87..ebe272575fb8 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1475,16 +1475,12 @@ 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,
> +struct vduse_mgmt_dev {
> +	struct vdpa_mgmt_dev mgmt_dev;
> +	struct device dev;
>   };
>   
> -static struct vdpa_mgmt_dev mgmt_dev;
> +static struct vduse_mgmt_dev *vduse_mgmt;
>   
>   static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
>   {
> @@ -1509,7 +1505,7 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
>   	}
>   	set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops);
>   	vdev->vdpa.dma_dev = &vdev->vdpa.dev;
> -	vdev->vdpa.mdev = &mgmt_dev;
> +	vdev->vdpa.mdev = &vduse_mgmt->mgmt_dev;
>   
>   	return 0;
>   }
> @@ -1555,34 +1551,52 @@ 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 void vduse_mgmtdev_release(struct device *dev)
> +{
> +	struct vduse_mgmt_dev *mgmt_dev;
> +
> +	mgmt_dev = container_of(dev, struct vduse_mgmt_dev, dev);
> +	kfree(mgmt_dev);
> +}
>   
>   static int vduse_mgmtdev_init(void)
>   {
>   	int ret;
>   
> -	ret = device_register(&vduse_mgmtdev);
> -	if (ret)
> +	vduse_mgmt = kzalloc(sizeof(*vduse_mgmt), GFP_KERNEL);
> +	if (!vduse_mgmt)
> +		return -ENOMEM;
> +
> +	ret = dev_set_name(&vduse_mgmt->dev, "vduse-la");
> +	if (ret) {
> +		kfree(vduse_mgmt);
>   		return ret;
> +	}
>   
> -	ret = vdpa_mgmtdev_register(&mgmt_dev);
> +	vduse_mgmt->dev.release = vduse_mgmtdev_release;
> +
> +	ret = device_register(&vduse_mgmt->dev);
>   	if (ret)
> -		goto err;
> +		goto dev_reg_err;
>   
> -	return 0;
> -err:
> -	device_unregister(&vduse_mgmtdev);
> +	vduse_mgmt->mgmt_dev.id_table = id_table;
> +	vduse_mgmt->mgmt_dev.ops = &vdpa_dev_mgmtdev_ops;
> +	vduse_mgmt->mgmt_dev.device = &vduse_mgmt->dev;
> +	ret = vdpa_mgmtdev_register(&vduse_mgmt->mgmt_dev);
> +	if (ret)
> +		device_unregister(&vduse_mgmt->dev);
> +
> +	return ret;
> +
> +dev_reg_err:
> +	put_device(&vduse_mgmt->dev);
>   	return ret;
>   }
>   
>   static void vduse_mgmtdev_exit(void)
>   {
> -	vdpa_mgmtdev_unregister(&mgmt_dev);
> -	device_unregister(&vduse_mgmtdev);
> +	vdpa_mgmtdev_unregister(&vduse_mgmt->mgmt_dev);
> +	device_unregister(&vduse_mgmt->dev);
>   }
>   
>   static int vduse_init(void)
> -- 2.26.2

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

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

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, May 18, 2022 4:32 AM
> 
> 在 2022/5/18 07:03, Parav Pandit 写道:
> >>> And regarding vduse_dev_release() and existing empty release
> >>> function,
> >> they can be dynamically allocated.
> >>> This is because they are really the struct device.
> >> I do not understand this statement, sorry.
> > It was bad sentence, my bad.
> >
> > What I wanted to say is, probably better explained with real patch below.
> > In context of [1], struct vduse_mgmtdev empty release function can be
> avoided by below inline patch [2].
> >
> > [1]https://www.spinics.net/lists/linux-virtualization/msg56371.html
> >
> > [2] patch:
> 
> 
> Ok, if we go this way (looks more like mdev_parent). I think we need at least
> rename the vdpa_mgmt_device, maybe vdpa_parent (like mdev_parent)?
> 
It can be. 
Parent is also a device. So...
Since there are no further comments on the inline patch, I will send it as separate patch this week.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jason Wang]

On Mon, May 23, 2022 at 10:00 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, May 18, 2022 4:32 AM
> >
> > 在 2022/5/18 07:03, Parav Pandit 写道:
> > >>> And regarding vduse_dev_release() and existing empty release
> > >>> function,
> > >> they can be dynamically allocated.
> > >>> This is because they are really the struct device.
> > >> I do not understand this statement, sorry.
> > > It was bad sentence, my bad.
> > >
> > > What I wanted to say is, probably better explained with real patch below.
> > > In context of [1], struct vduse_mgmtdev empty release function can be
> > avoided by below inline patch [2].
> > >
> > > [1]https://www.spinics.net/lists/linux-virtualization/msg56371.html
> > >
> > > [2] patch:
> >
> >
> > Ok, if we go this way (looks more like mdev_parent). I think we need at least
> > rename the vdpa_mgmt_device, maybe vdpa_parent (like mdev_parent)?
> >
> It can be.
> Parent is also a device. So...

How about "vdpa_parent_data"? Not a native speaker but it's better to
remove "device" from the name anhow.

Thanks

> Since there are no further comments on the inline patch, I will send it as separate patch this week.

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

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

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Sunday, May 22, 2022 11:41 PM
> To: Parav Pandit <parav@nvidia.com>
> Cc: gregkh@linuxfoundation.org; Yongji Xie <xieyongji@bytedance.com>;
> mst@redhat.com; lingshan.zhu@intel.com; Eli Cohen <elic@nvidia.com>;
> virtualization@lists.linux-foundation.org
> Subject: Re: [PATCH v2 2/3] vdpa: Add a device object for vdpa management
> device
> 
> On Mon, May 23, 2022 at 10:00 AM Parav Pandit <parav@nvidia.com>
> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, May 18, 2022 4:32 AM
> > >
> > > 在 2022/5/18 07:03, Parav Pandit 写道:
> > > >>> And regarding vduse_dev_release() and existing empty release
> > > >>> function,
> > > >> they can be dynamically allocated.
> > > >>> This is because they are really the struct device.
> > > >> I do not understand this statement, sorry.
> > > > It was bad sentence, my bad.
> > > >
> > > > What I wanted to say is, probably better explained with real patch
> below.
> > > > In context of [1], struct vduse_mgmtdev empty release function can
> > > > be
> > > avoided by below inline patch [2].
> > > >
> > > > [1]https://www.spinics.net/lists/linux-virtualization/msg56371.htm
> > > > l
> > > >
> > > > [2] patch:
> > >
> > >
> > > Ok, if we go this way (looks more like mdev_parent). I think we need
> > > at least rename the vdpa_mgmt_device, maybe vdpa_parent (like
> mdev_parent)?
> > >
> > It can be.
> > Parent is also a device. So...
> 
> How about "vdpa_parent_data"? Not a native speaker but it's better to
> remove "device" from the name anhow.
> 
_data is diluting the role what this object does as has ops.
And object is more than a parent.
So may be vdpa_mgmt_link as it enables handling vdpa device management and linkage to the *device.

I still feel that its overkill.
Mgmt_dev is probably just fine, as its taking care of life cycling multiple devices...

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

Re: [PATCH v2 3/3] Docs/ABI/testing: Add VDUSE sysfs interface ABI document

[Greg KH]

On Mon, May 16, 2022 at 02:03:42PM +0800, Xie Yongji wrote:
> This adds missing documentation for VDUSE sysfs interface ABI
> under Documentation/ABI/testing.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  Documentation/ABI/testing/sysfs-class-vduse | 33 +++++++++++++++++++++
>  MAINTAINERS                                 |  1 +
>  2 files changed, 34 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-vduse
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-vduse b/Documentation/ABI/testing/sysfs-class-vduse
> new file mode 100644
> index 000000000000..2f2bc5c8fc48
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-vduse
> @@ -0,0 +1,33 @@
> +What:		/sys/class/vduse/
> +Date:		Oct 2021
> +KernelVersion:	5.15
> +Contact:	Yongji Xie <xieyongji@bytedance.com>
> +Description:
> +		The vduse/ class sub-directory belongs to the VDUSE
> +		framework and provides a sysfs interface for configuring
> +		VDUSE devices.
> +
> +What:		/sys/class/vduse/control/
> +Date:		Oct 2021
> +KernelVersion:	5.15
> +Contact:	Yongji Xie <xieyongji@bytedance.com>
> +Description:
> +		This directory entry is created for the control device
> +		of VDUSE framework.
> +
> +What:		/sys/class/vduse/<device-name>/
> +Date:		Oct 2021
> +KernelVersion:	5.15
> +Contact:	Yongji Xie <xieyongji@bytedance.com>
> +Description:
> +		This directory entry is created when a VDUSE device is
> +		created via the control device.
> +
> +What:		/sys/class/vduse/<device-name>/msg_timeout
> +Date:		Oct 2021
> +KernelVersion:	5.15
> +Contact:	Yongji Xie <xieyongji@bytedance.com>
> +Description:
> +		(RW) The timeout (in seconds) for waiting for the control
> +		message's response from userspace. Default value is 30s.
> +		Writing a '0' to the file means to disable the timeout.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6d879cb0afd..d9a423de2f4d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20842,6 +20842,7 @@ M:	Jason Wang <jasowang@redhat.com>
>  L:	virtualization@lists.linux-foundation.org
>  S:	Maintained
>  F:	Documentation/ABI/testing/sysfs-bus-vdpa
> +F:	Documentation/ABI/testing/sysfs-class-vduse
>  F:	Documentation/devicetree/bindings/virtio/
>  F:	drivers/block/virtio_blk.c
>  F:	drivers/crypto/virtio/
> -- 
> 2.20.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/3] vduse: Pass management device pointer to vduse_dev_init_vdpa()

[Michael S. Tsirkin]

On Mon, May 16, 2022 at 02:03:40PM +0800, Xie Yongji wrote:
> Pass management device pointer from vdpa_dev_add() to
> vduse_dev_init_vdpa() rather than using the static
> variable directly.
> 
> No functional change.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>

Could you pls add a cover letter explaining what is the patchset
trying to achieve? I think I can guess but I'd rather not guess ...


> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 160e40d03084..d3bf55a58cd2 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1484,9 +1484,8 @@ static struct device vduse_mgmtdev = {
>  	.release = vduse_mgmtdev_release,
>  };
>  
> -static struct vdpa_mgmt_dev mgmt_dev;
> -
> -static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> +static int vduse_dev_init_vdpa(struct vduse_dev *dev,
> +			       struct vdpa_mgmt_dev *mdev, const char *name)
>  {
>  	struct vduse_vdpa *vdev;
>  	int ret;
> @@ -1509,7 +1508,7 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
>  	}
>  	set_dma_ops(&vdev->vdpa.dev, &vduse_dev_dma_ops);
>  	vdev->vdpa.dma_dev = &vdev->vdpa.dev;
> -	vdev->vdpa.mdev = &mgmt_dev;
> +	vdev->vdpa.mdev = mdev;
>  
>  	return 0;
>  }
> @@ -1526,7 +1525,7 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  		mutex_unlock(&vduse_lock);
>  		return -EINVAL;
>  	}
> -	ret = vduse_dev_init_vdpa(dev, name);
> +	ret = vduse_dev_init_vdpa(dev, mdev, name);
>  	mutex_unlock(&vduse_lock);
>  	if (ret)
>  		return ret;
> -- 
> 2.20.1

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