On Fri, May 13, 2022 at 12:46 PM Yongji Xie <xieyongji@xxxxxxxxxxxxx> wrote:
>
> On Fri, May 13, 2022 at 11:53 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> >
> > 在 2022/5/12 17:40, Greg KH 写道:
> > > On Thu, May 12, 2022 at 05:31:51PM +0800, Yongji Xie wrote:
> > >> On Thu, May 12, 2022 at 4:58 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >>> On Thu, May 12, 2022 at 03:51:38PM +0800, Yongji Xie wrote:
> > >>>> On Thu, May 12, 2022 at 2:19 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >>>>> On Thu, May 12, 2022 at 01:59:00PM +0800, Yongji Xie wrote:
> > >>>>>> On Thu, May 12, 2022 at 1:23 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >>>>>>> On Thu, May 12, 2022 at 01:19:58PM +0800, Yongji Xie wrote:
> > >>>>>>>> On Wed, May 11, 2022 at 10:02 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >>>>>>>>> On Wed, May 11, 2022 at 09:55:22PM +0800, Xie Yongji wrote:
> > >>>>>>>>>> It's not recommended to provide an "empty" release function
> > >>>>>>>>>> for the device object as Documentation/core-api/kobject.rst
> > >>>>>>>>>> mentioned.
> > >>>>>>>>> "it is a bug to have an empty release function" is more like it :)
> > >>>>>>>>>
> > >>>>>>>> OK.
> > >>>>>>>>
> > >>>>>>>>>> So let's allocate the device object dynamically
> > >>>>>>>>>> to get rid of it.
> > >>>>>>>>> Much better, but not quite there, see below for details.
> > >>>>>>>>>
> > >>>>>>>>>> Signed-off-by: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> > >>>>>>>>>> ---
> > >>>>>>>>>> drivers/vdpa/vdpa_user/vduse_dev.c | 43 +++++++++++++++++-------------
> > >>>>>>>>>> 1 file changed, 25 insertions(+), 18 deletions(-)
> > >>>>>>>>>>
> > >>>>>>>>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > >>>>>>>>>> index 160e40d03084..a8a5ebaefa10 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 struct vdpa_mgmt_dev mgmt_dev;
> > >>>>>>>>> Close. This should be a pointer and the device structure within it
> > >>>>>>>>> should control the lifecycle of that structure. It should not be a
> > >>>>>>>>> single static structure like this, that's very odd.
> > >>>>>>>>>
> > >>>>>>>> OK, I can define mgmt_dev as a pointer. But the device is defined as a
> > >>>>>>>> parent device for structure vdpa_mgmt_dev. So I think we can't use it
> > >>>>>>>> to control the lifecycle of the structure vdpa_mgmt_dev.
> > >>>>>>> You should be able to control the lifecycle of it, especially if it is
> > >>>>>>> the parent device of something. To not do that correctly is to have
> > >>>>>>> everything messed up as you should be using the driver model properly.
> > >>>>>>> As it is, you are not :(
> > >>>>>>>
> > >>>>>> I can control the lifecycle of it. What I mean is that I can not free
> > >>>>>> it in the release function of the device object since it is the parent
> > >>>>>> device of mgmt_dev. E.g., in other cases (such as ifcvf_probe()), the
> > >>>>>> device object comes from a pci device but the structure vdpa_mgmt_dev
> > >>>>>> is created during driver probing. The structure vdpa_mgmt_dev just
> > >>>>>> maintains a pointer to the device object. So the structure
> > >>>>>> vdpa_mgmt_dev and the device object have different lifecycles.
> > >>>>> Then something is very very wrong here. The structure's lifespace
> > >>>>> should only be controlled by one reference count, not multiple ones.
> > >>>> But they are different devices (one is vdpa_mgmt_dev and another is
> > >>>> the device I create which will be the parent of vdpa_mgmt_dev), I
> > >>>> didn't get why we need to control their lifecycle in one reference
> > >>>> count.
> > >>>>
> > >>>>> Have it be controlled by the device you create and properly register as
> > >>>>> a child of the pci device and all should be fine.
> > >>>>>
> > >>>> The structure vdpa_mgmt_dev is defined as:
> > >>>>
> > >>>> /**
> > >>>> * struct vdpa_mgmt_dev - vdpa management device
> > >>>> * @device: Management parent 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
> > >>>> * management device support during dev_add callback
> > >>>> * @list: list entry
> > >>>> */
> > >>>> struct vdpa_mgmt_dev {
> > >>>> struct device *device;
> > >>>> const struct vdpa_mgmtdev_ops *ops;
> > >>>> const struct virtio_device_id *id_table;
> > >>>> u64 config_attr_mask;
> > >>>> struct list_head list;
> > >>>> };
> > >>>>
> > >>>> Now the device I create is passed to the struct vdpa_mgmt_dev as a
> > >>>> parent device pointer. If we want to control the lifecycle of the
> > >>>> structure vdpa_mgmt_dev by the device I create, some logic of the vdpa
> > >>>> management device needs to be reworked. For example, define a device
> > >>>> object for structure vdpa_mgmt_dev rather than just maintaining a
> > >>>> pointer to the parent device.
> > >>> But this is a device (it says in the name), so it should have the device
> > >>> structure embedded in it to control the lifespan of it.
> > >>>
> > >> Currently it's not a device as Jason mentioned. So I think the
> > >> question is whether we need to re-define it or just re-name it.
> > > It is a device, you have a pointer to a device structure that is used by
> > > the code!
> > >
> > > Embed it in the structure and you should be fine. Well, maybe, let's
> > > see what falls out from there as it seems like the use of the driver
> > > model is a bit messed up in this codebase. But it's a good first step
> > > forward in fixing things.
> >
> >
> > Right, things became messed up since the introduction of the mgmt device.
> >
> > Yong Ji, wand to do the work (or if you wish I can do rework)?
> >
>
> Sure. I can do it.
Great.
Thanks
>
> Thanks,
> Yongji
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
