Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Greg KH]

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

> 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@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.

thanks,

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Greg KH]

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@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@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 :(

thanks,

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Greg KH]

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@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@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@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.
Have it be controlled by the device you create and properly register as
a child of the pci device and all should be fine.

thanks,

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Jason Wang]

在 2022/5/12 15:51, Yongji Xie 写道:
> On Thu, May 12, 2022 at 2:19 PM Greg KH <gregkh@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@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@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@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.


I think the name "vdpa_mgmt_dev" is probably confusing since it's not a 
device (no embedded device struct).

We probably need a better name.

Thanks


>
>> 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.
>
> Thanks,
> Yongji
>

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Greg KH]

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@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@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@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@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.

If the lifespan is somehow different, then you have a real problem no
matter what (if it is or is not embedded), so that shows this really
needs to be resolved properly.

thanks,

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[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@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@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@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@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@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.

thanks,

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Jason Wang]

在 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@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@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@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@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@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)?

Thanks


>
> thanks,
>
> greg k-h
>

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

Re: [PATCH 1/2] vduse: Remove empty vduse_mgmtdev_release()

[Jason Wang]

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

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