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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Dec 2019 23:06:45 +0000
From:   "Saleem, Shiraz" <shiraz.saleem@...el.com>
To:     Jason Gunthorpe <jgg@...pe.ca>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "parav@...lanox.com" <parav@...lanox.com>
Subject: RE: [PATCH v3 19/20] RDMA: Add irdma Kconfig/Makefile and remove
 i40iw

> 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@...el.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

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