? 2022/5/12 15:51, Yongji Xie ??:> 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.
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
>