Messages in this thread | | | Subject | Re: [PATCH V6 8/8] virtio: Intel IFC VF driver for VDPA | From | Jason Wang <> | Date | Fri, 20 Mar 2020 17:17:31 +0800 |
| |
On 2020/3/19 下午9:02, Jason Gunthorpe wrote: > On Thu, Mar 19, 2020 at 04:14:37PM +0800, Jason Wang wrote: >> On 2020/3/18 下午8:22, Jason Gunthorpe wrote: >>> On Wed, Mar 18, 2020 at 04:03:27PM +0800, Jason Wang wrote: >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>> + >>>> +static int ifcvf_vdpa_attach(struct ifcvf_adapter *adapter) >>>> +{ >>>> + int ret; >>>> + >>>> + adapter->vdpa_dev = vdpa_alloc_device(adapter->dev, adapter->dev, >>>> + &ifc_vdpa_ops); >>>> + if (IS_ERR(adapter->vdpa_dev)) { >>>> + IFCVF_ERR(adapter->dev, "Failed to init ifcvf on vdpa bus"); >>>> + put_device(&adapter->vdpa_dev->dev); >>>> + return -ENODEV; >>>> + } >>> The point of having an alloc call is so that the drivers >>> ifcvf_adaptor memory could be placed in the same struct - eg use >>> container_of to flip between them, and have a kref for both memories. >>> >>> It seem really weird to have an alloc followed immediately by >>> register. >> >> I admit the ifcvf_adapter is not correctly ref-counted. What you suggest >> should work. But it looks to me the following is more cleaner since the >> members of ifcvf_adapter are all related to PCI device not vDPA itself. > I've done it both ways (eg tpm is as you describe, ib is using alloc). > > I tend to prefer the alloc method today, allowing the driver memory to > have a proper refcount makes the driver structure usable with RCU and > allows simple solutions to some tricky cases. It is a bit hard to > switch to this later.. > >> - keep the current layout of ifcvf_adapter >> - merge vdpa_alloc_device() and vdpa_register_device() >> - use devres to bind ifcvf_adapter refcnt/lifcycle to the under PCI device > This is almost what tpm does. Keep in mind the lifecycle with devm is > just slightly past the driver remove call, so remove still > must revoke all external references to the memory. > > The merging alloc and register rarely works out, the register must be > the very last thing done, and usually you need the subsystem pointer > to do pre-registration setup in anything but the most trivial of > subsystems and drivers. > >> If we go for the container_of method, we probably need >> >> - accept a size of parent parent structure in vdpa_alloc_device() and >> mandate vdpa_device to be the first member of ifcvf_adapter >> - we need provide a way to free resources of parent structure when we >> destroy vDPA device > Yep. netdev and rdma work this way with a free memory callback in the > existing ops structures. > > Jason
Ok, I get your points now. Will go for way of container_of in next version.
Thanks
| |