On Fri, May 13, 2022 at 12:46 PM Yongji Xie <xieyongji at bytedance.com>
wrote:>
> On Fri, May 13, 2022 at 11:53 AM Jason Wang <jasowang at redhat.com>
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 at
linuxfoundation.org> 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 at
linuxfoundation.org> 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 at linuxfoundation.org> 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 at linuxfoundation.org> 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 at bytedance.com>
> > >>>>>>>>>> ---
> > >>>>>>>>>>
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
>