Virtualization - May 2022 - [PATCH v2 2/3] vdpa: Add a device object for vdpa management device

Hi Greg, Yongji,
> From: Yongji Xie <xieyongji at bytedance.com>
> Sent: Tuesday, May 17, 2022 3:25 AM
> 
> On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav at nvidia.com>
wrote:
> >
> >
> >
> > > From: Xie Yongji <xieyongji at bytedance.com>
> > > Sent: Monday, May 16, 2022 2:04 AM
> > >
> > > Introduce a device object for vdpa management device to control
its
> > > lifecycle.
> > Why is this needed?
> > What is the limitation of current approach that requires new device
for
> mgmtdev?
> > The primary motivation is missing in the commit log here.
> >
> 
> OK, I will add one. This patch actually comes from the discussion:
> 
> 
> https://www.spinics.net/lists/linux-virtualization/msg56371.html
> 
> The "vdpa_mgmt_dev" makes things a little confusing. Embedding
the
> device struct in it to control its lifecycle simplifies the logic and makes
the
> driver model clear.
> 
No. it doesn't.

vdpa_mgmt_dev is really the handle for all the netlink socket messages targeted.
It is not really a struct device.
We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing.

And regarding vduse_dev_release() and existing empty release function, they can
be dynamically allocated.
This is because they are really the struct device.

> > > And the device name will be used to match
> VDPA_ATTR_MGMTDEV_DEV_NAME
> > > field of netlink message rather than using parent device name.
> > >
> > > With this patch applied, drivers should use vdpa_mgmtdev_alloc()
or
> > > _vdpa_mgmtdev_alloc() to allocate a vDPA management device before
> > > calling vdpa_mgmtdev_register(). And some buggy empty release
> > > function can also be removed from the driver codes.
> > What is the bug, can you please explain?
> 
> I'm not sure if "buggy" is the right word. But the empty
release function
> should be removed as mentioned in Documentation/core-api/kobject.rst.
> 
Sure. So, struct device in the vdpa_sim and vduse can allocate the struct device
using kzalloc() and release callback can free it.

On Tue, May 17, 2022 at 12:21:03PM +0000, Parav Pandit
wrote:
> Hi Greg, Yongji,
> 
> > From: Yongji Xie <xieyongji at bytedance.com>
> > Sent: Tuesday, May 17, 2022 3:25 AM
> > 
> > On Tue, May 17, 2022 at 4:06 AM Parav Pandit <parav at
nvidia.com> wrote:
> > >
> > >
> > >
> > > > From: Xie Yongji <xieyongji at bytedance.com>
> > > > Sent: Monday, May 16, 2022 2:04 AM
> > > >
> > > > Introduce a device object for vdpa management device to
control its
> > > > lifecycle.
> > > Why is this needed?
> > > What is the limitation of current approach that requires new
device for
> > mgmtdev?
> > > The primary motivation is missing in the commit log here.
> > >
> > 
> > OK, I will add one. This patch actually comes from the discussion:
> > 
> > 
> > https://www.spinics.net/lists/linux-virtualization/msg56371.html
> > 
> > The "vdpa_mgmt_dev" makes things a little confusing.
Embedding the
> > device struct in it to control its lifecycle simplifies the logic and
makes the
> > driver model clear.
> > 
> No. it doesn't.
> 
> vdpa_mgmt_dev is really the handle for all the netlink socket messages
targeted.
> It is not really a struct device.
Why can it not be one?  It has lifetime rules that must be followed, so
might as well use the built-in code to handle this, right?

What is wrong with it being a struct device?
> We can rename it to vdpa_mgmt_handle, if the _dev suffix is confusing.
Where is the "management" device if this is not that?

What does "handle" mean here?
> And regarding vduse_dev_release() and existing empty release function, they
can be dynamically allocated.
> This is because they are really the struct device.
I do not understand this statement, sorry.

thanks,

greg k-h