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

[Jason Wang <jasowang@redhat.com>]

在 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