Jason Wang
2022-May-18 08:31 UTC
[PATCH v2 2/3] vdpa: Add a device object for vdpa management device
? 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 at 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 at 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
Parav Pandit
2022-May-23 02:00 UTC
[PATCH v2 2/3] vdpa: Add a device object for vdpa management device
> From: Jason Wang <jasowang at 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.