> Subject: Re: [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA
>
[....]
> > /**
> > @@ -275,6 +281,27 @@ void i40e_client_update_msix_info(struct i40e_pf *pf)
> > cdev->lan_info.msix_entries =
> > &pf->msix_entries[pf->iwarp_base_vector];
> > }
> >
> > +static int i40e_init_client_virtdev(struct i40e_pf *pf) {
> > + struct i40e_info *ldev = &pf->cinst->lan_info;
> > + struct pci_dev *pdev = pf->pdev;
> > + struct virtbus_device *vdev;
> > + int ret;
> > +
> > + vdev = &ldev->vdev;
> > + vdev->name = I40E_PEER_RDMA_NAME;
> > + vdev->dev.parent = &pf->pdev->dev;
>
> What a total and complete mess of a tangled web you just wove here.
>
> Ok, so you pass in a single pointer, that then dereferences 3 pointers deep to find
> the pointer to the virtbus_device structure, but then you point the parent of that
> device, back at the original structure's sub-pointer's device itself.
>
> WTF?
OK. This is convoluted. Passing a pointer to the i40e_info object should suffice. So something like,
+static int i40e_init_client_virtdev(struct i40e_info *ldev) {
+ struct pci_dev *pdev = ldev->pcidev;
+ struct virtbus_device *vdev = &ldev->vdev;
+ int ret;
+
+ vdev->name = I40E_PEER_RDMA_NAME;
+ vdev->dev.parent = &pdev->dev;
+ ret = virtbus_dev_register(vdev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
>
> And who owns the memory of this thing that is supposed to be dynamically
> controlled by something OUTSIDE of this driver? Who created that thing 3
> pointers deep? What happens when you leak the memory below (hint, you did),
> and who is supposed to clean it up if you need to properly clean it up if something
> bad happens?
The i40e_info object memory is tied to the PF driver.
The object hierarchy is,
i40e_pf: pointer to i40e_client_instance
----- i40e_client_instance: i40e_info
----- i40e_info: virtbus_device
For each PF, there is a client_instance object allocated.
The i40e_info object is populated and the virtbus_device hanging off this object is registered.
In irdma probe(), we use the container_of macro to get to this i40e_info object from the
virtbus_device. It contains all the ops/info which RDMA driver needs from the PCI function driver.
The lifetime of the i40e_info object (and the virtbus device) is tied to the PF.
When PF goes away, virtbus_device is unregistered and the client_instance object memory
is freed.
>
> > +
> > + ret = virtbus_dev_register(vdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failure adding client virtbus dev %s %d\n",
> > + I40E_PEER_RDMA_NAME, ret);
>
> Again, the core should handle this, right?
Right. Will fix.
>
> > + return ret;
>
> Did you just leak memory?
Thanks! Will fix.
> Also, what ever happened to my "YOU ALL MUST AGREE TO WORK TOGETHER"
> requirement between this group, and the other group trying to do the same thing? I
> want to see signed-off-by from EVERYONE involved before we are going to
> consider this thing.
>
We will have all parties cc'ed in the next submission. Would encourage folks to review
and hopefully we can get some consensus.
