Greg KH
2022-May-16 11:54 UTC
[PATCH v2 2/3] vdpa: Add a device object for vdpa management device
On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote:> On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote: > > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote: > > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang at redhat.com> wrote: > > > > > > > > > > > > > > > ? 2022/5/16 14:03, Xie Yongji ??: > > > > > > Introduce a device object for vdpa management device to control > > > > > > its lifecycle. 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. > > > > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > > > > > > --- > > > > > > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-- > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 11 ++-- > > > > > > drivers/vdpa/vdpa.c | 92 ++++++++++++++++++++++++---- > > > > > > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 39 ++++-------- > > > > > > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 46 +++++--------- > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 38 ++++-------- > > > > > > include/linux/vdpa.h | 38 +++++++++++- > > > > > > 7 files changed, 168 insertions(+), 107 deletions(-) > > > > > > > > > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > index 4366320fb68d..d4087c37cfdf 100644 > > > > > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > @@ -821,10 +821,11 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > u32 dev_type; > > > > > > int ret; > > > > > > > > > > > > - ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL); > > > > > > - if (!ifcvf_mgmt_dev) { > > > > > > + ifcvf_mgmt_dev = vdpa_mgmtdev_alloc(struct ifcvf_vdpa_mgmt_dev, > > > > > > + mdev, dev_name(dev), dev); > > > > > > > > > > > > > > > Just wonder if it's better to make vDPA device a child of the mgmt > > > > > device instead of the PCI device? > > > > > > > > > > (Currently we use PCI device as the parent of the vDPA device, or at > > > > > least we can do this for the simulator which doesn't have a parent?) > > > > > > > > > > > > > Make sense. I think we can do it for all vDPA drivers. Make sure the > > > > parent of the vDPA device is the vDPA management device. > > > > > > > > Thanks, > > > > Yongji > > > > > > > > > that's an ABI change though isn't it? parent is exposed in sysfs, > > > right? > > > > > > > Hmm...yes. So it looks like we can't change it, right? > > > > Thanks, > > Yongji > > Afraid so. a way to find the pci device already exists I think, right?You can change it, any tools should be going through the bus/device links, not walking the sysfs tree directly, right? That's what those symlinks are for, in order to properly be able to enumerate different device types. A specific topology in sysfs should not ever be assumed to be static over time, that's not an accurate representation of how the kernel works. So try it and see? Odds are there are no tools that even care about these devices, right? Or is there? thanks, greg k-h
Jason Wang
2022-May-17 06:29 UTC
[PATCH v2 2/3] vdpa: Add a device object for vdpa management device
On Mon, May 16, 2022 at 7:54 PM Greg KH <gregkh at linuxfoundation.org> wrote:> > On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote: > > On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote: > > > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > > > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote: > > > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang at redhat.com> wrote: > > > > > > > > > > > > > > > > > > ? 2022/5/16 14:03, Xie Yongji ??: > > > > > > > Introduce a device object for vdpa management device to control > > > > > > > its lifecycle. 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. > > > > > > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > > > > > > > --- > > > > > > > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++-- > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 11 ++-- > > > > > > > drivers/vdpa/vdpa.c | 92 ++++++++++++++++++++++++---- > > > > > > > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 39 ++++-------- > > > > > > > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 46 +++++--------- > > > > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 38 ++++-------- > > > > > > > include/linux/vdpa.h | 38 +++++++++++- > > > > > > > 7 files changed, 168 insertions(+), 107 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > > index 4366320fb68d..d4087c37cfdf 100644 > > > > > > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > > > > > > > @@ -821,10 +821,11 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > > > > > u32 dev_type; > > > > > > > int ret; > > > > > > > > > > > > > > - ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL); > > > > > > > - if (!ifcvf_mgmt_dev) { > > > > > > > + ifcvf_mgmt_dev = vdpa_mgmtdev_alloc(struct ifcvf_vdpa_mgmt_dev, > > > > > > > + mdev, dev_name(dev), dev); > > > > > > > > > > > > > > > > > > Just wonder if it's better to make vDPA device a child of the mgmt > > > > > > device instead of the PCI device? > > > > > > > > > > > > (Currently we use PCI device as the parent of the vDPA device, or at > > > > > > least we can do this for the simulator which doesn't have a parent?) > > > > > > > > > > > > > > > > Make sense. I think we can do it for all vDPA drivers. Make sure the > > > > > parent of the vDPA device is the vDPA management device. > > > > > > > > > > Thanks, > > > > > Yongji > > > > > > > > > > > > that's an ABI change though isn't it? parent is exposed in sysfs, > > > > right? > > > > > > > > > > Hmm...yes. So it looks like we can't change it, right? > > > > > > Thanks, > > > Yongji > > > > Afraid so. a way to find the pci device already exists I think, right? > > You can change it, any tools should be going through the bus/device > links, not walking the sysfs tree directly, right? That's what those > symlinks are for, in order to properly be able to enumerate different > device types. > > A specific topology in sysfs should not ever be assumed to be static > over time, that's not an accurate representation of how the kernel > works. > > So try it and see? Odds are there are no tools that even care about > these devices, right? Or is there?I think there's no tool that depends on the sysfs tree now. The management layer is only expected to talk to the management device via vdpa(8) which is integrated in iproute2. Thanks> > thanks, > > greg k-h >