Re: [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA

['Greg KH' <gregkh@linuxfoundation.org>]

On Wed, Dec 18, 2019 at 06:57:10PM +0000, Saleem, Shiraz wrote:
> > > > Subject: Re: [PATCH v3 04/20] i40e: Register a virtbus device to
> > > > provide RDMA
> [.....]
> 
> > > >
> > > > 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.
> > 
> > What is a "PF"?
> 
> physical function.
> 
> > 
> > > The object hierarchy is,
> > >
> > > i40e_pf: pointer to i40e_client_instance
> > > 	----- i40e_client_instance: i40e_info
> > > 		----- i40e_info: virtbus_device
> > 
> > So you are 3 pointers deep to get a structure that is dynamically controlled?  Why
> > are those "3 pointers" not also represented in sysfs?
> > You have a heiarchy within the kernel that is not being represented that way to
> > userspace, why?
> > 
> > Hint, I think this is totally wrong, you need to rework this to be sane.
> > 
> > > For each PF, there is a client_instance object allocated.
> > 
> > Great, make it dynamic and in the device tree.
> > 
> > > The i40e_info object is populated and the virtbus_device hanging off this object
> > is registered.
> > 
> > Great, make that dynamic and inthe device tree.
> > 
> > If you think this is too much, then your whole mess here is too much and needs to
> > be made a lot simpler.
> >
> 
> I think we can decouple the virtbus_device object from i40e_info object.
> 
> Instead allocate a i40e_virtbus_device object which contains
> the virtbus_device and a pointer to i40e_info object for the
> RDMA driver to consume on probe(). Register it the virtbus, and provide
> a release callback to free up its memory.
> 
> Sending a patch snippet to hopefully make it clearer.

Yes, this looks a little bit more sane.

greg k-h