> 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@xxxxxxxxx> > > > > 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 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. In irdma, there is a doorbell and a push page that are mapped. And the offset passed in is used to distinguish the 2 (0-for doorbell) and determine the address to map. Also, in the db scheme, I think there is presumption the mmap comes down with the key passed back in kernel response struct. For the doorbell page at least, the mmap in library provider just uses 0. [....] > - 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? virtbus_driver_unregister should guarantee the remove() callbacks and ib device unregistration. Or did you mean just instrument ops->dealloc_driver? Surely I am missing something. [....] > - The whole cqp_compl_thread thing looks really weird What is the concern? Thanks for the feedback. Will work on it. Shiraz
