Re: [PATCH v3 19/20] RDMA: Add irdma Kconfig/Makefile and remove i40iw

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Dec 13, 2019 at 11:06:45PM +0000, Saleem, Shiraz wrote:
> > Subject: Re: [PATCH v3 19/20] RDMA: Add irdma Kconfig/Makefile and remove
> > i40iw
> > 
> > On Mon, Dec 09, 2019 at 02:49:34PM -0800, Jeff Kirsher wrote:
> > > From: Shiraz Saleem <shiraz.saleem@intel.com>
> > >
> > > Add Kconfig and Makefile to build irdma driver.
> > >
> > > Remove i40iw driver. irdma is the replacement driver that supports
> > > X722.
> > 
> > I looked through this for a litle while, it is very very big. I'd like some of the other
> > people who have sent drivers lately to give it a go over as well..
> > 
> > A few broad comments
> >  - Do not use the 'err1', 'err2', etc labels for goto unwind
> >  - Please check all uses of rcu, I could not see why some existed
> >  - Use the new rdma mmap api. The whole mmap flow looked wonky to me
> Presume your referring to this series?
> https://github.com/jgunthorpe/linux/commits/rdma_mmap

Yes, it is merged now

> At the time it was published, I didn't think it applied to irdma, but rather
> benefit those drivers that keyed off an mmap database in their mmap function.

All drivers using mmap should be using it.

New drivers should not be using mmap via hard coded keys. The offset
to pass to mmap should always be returned from a system call.

For compatibility insert the hard coded key with the mmap stuff and
use the APIs for lifetime management.

> In irdma, there is a doorbell and a push page that are mapped. 

Pretty much all hardware requires these to be per-security domain, so
you have a lifecycle model that matches what the mmap API is now
providing.

> >  - New drivers should use the ops->driver_unregister flow
> https://www.spinics.net/lists/linux-rdma/msg75466.html
> "These APIs are intended to support drivers that exist outside the usual
> driver core probe()/remove() callbacks. Normally the driver core will
> prevent remove() from running concurrently with probe(), once this safety
> is lost drivers need more support to get the locking and lifetimes right."
> 
> As per this description, it seems ib_unregister_driver() would be
> redundant for irdma to use in module exit? 

Yes, this driver doesn't need that call

> Or did you mean just instrument ops->dealloc_driver?

Yes

> >  - The whole cqp_compl_thread thing looks really weird
> What is the concern?

It looks like an open coded work queue

Jason