Re: [PATCH for-next v7 10/12] RDMA/erdma: Add the erdma module

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, May 16, 2022 at 09:59:28PM +0800, Cheng Xu wrote:
> 
> 
> On 5/16/22 8:40 PM, Cheng Xu wrote:
> > 
> > 
> > On 5/16/22 7:49 PM, Jason Gunthorpe wrote:
> > > On Mon, May 16, 2022 at 11:15:32AM +0800, Cheng Xu wrote:
> > > > 
> > > > 
> > > > On 5/10/22 9:17 PM, Jason Gunthorpe wrote:
> > > > > On Thu, Apr 21, 2022 at 03:17:45PM +0800, Cheng Xu wrote:
> > > > > 
> > > > > > +static struct rdma_link_ops erdma_link_ops = {
> > > > > > +    .type = "erdma",
> > > > > > +    .newlink = erdma_newlink,
> > > > > > +};
> > > > > 
> > > > > Why is there still a newlink?
> > > > > 
> > > > > Jason
> > > > 
> > > > 
> > > > Yeah, I remember your suggestion that the ibdev should keep the same
> > > > lifecycle with its PCI device, and so does it now.
> > > > 
> > > > The initialization flow for erdma now:
> > > >       probe:
> > > >         - Hardware specified initialization
> > > >         - IB device initialization
> > > >         - Calling ib_register_device with ibdev->netdev == NULL
> > > > 
> > > > After probe, The ib device has been registered, but we left it in
> > > > invalid state.
> > > > To fully complete the initialization, we should set the ibdev->netdev.
> > > > 
> > > > And the newlink command in erdma only do one thing now: set the
> > > > ibdev->netdev if the rule matches, and it is uncorrelated with the
> > > > ib device lifecycle.
> > > 
> > > This is not what newlink is for, it can't be used like this
> > > 
> > > Jason
> > 
> > 
> > [A typo in previous reply, so I re-edit it]
> > 
> <...>
> > Or, Is it ok that erdma registers a net notifier in our module to handle
> > this?
> > 
> 
> I think this twice, use a net notifier of module can not solve this, due
> to the leak of all ib devices information in erdma module.

I don't understand what this means

> The solution may be simple: register net notifier per device in probe,
> and call ib_device_set_netdev if matches in the notifier callback.

That sounds wrong

Jason