Re: [PATCH v3 for-next 07/16] IB/ipoib: Increase ipoib Datagram mode MTU's upper limit

From: Ira Weiny <ira.weiny@intel.com>
To: Nathan Chancellor <natechancellor@gmail.com>
Cc: Leon Romanovsky <leon@kernel.org>, Jason Gunthorpe <jgg@ziepe.ca>,
	Dennis Dalessandro <dennis.dalessandro@intel.com>,
	dledford@redhat.com, linux-rdma@vger.kernel.org,
	Mike Marciniszyn <mike.marciniszyn@intel.com>,
	Sadanand Warrier <sadanand.warrier@intel.com>,
	Kaike Wan <kaike.wan@intel.com>
Subject: Re: [PATCH v3 for-next 07/16] IB/ipoib: Increase ipoib Datagram mode MTU's upper limit
Date: Tue, 16 Jun 2020 12:17:08 -0700	[thread overview]
Message-ID: <20200616191708.GF4160762@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200616184231.GA3734396@ubuntu-n2-xlarge-x86>

On Tue, Jun 16, 2020 at 11:42:31AM -0700, Nathan Chancellor wrote:
> On Tue, Jun 16, 2020 at 09:25:34AM +0300, Leon Romanovsky wrote:
> > On Mon, Jun 15, 2020 at 05:56:50PM -0700, Nathan Chancellor wrote:
> > > On Mon, Jun 01, 2020 at 10:57:22AM -0300, Jason Gunthorpe wrote:
> > > > On Mon, Jun 01, 2020 at 09:48:47AM -0400, Dennis Dalessandro wrote:
> > > >
> > > > > They should probably all be in "enum ib_mtu". Jason any issues with us donig
> > > > > that? I can't for certain recall the real reason they were kept separate in
> > > > > the first place.
> > > >
> > > > It is probably OK
> > > >
> > > > Jason
> > >
> > > I don't mind taking a wack at this if you guys are too busy (I'm rather
> > > tired of seeing the warning across all of my builds). However, I am
> > > wondering how far should this be unwound? Should 'enum opa_mtu' be
> > > collapsed into 'enum ib_mtu' and then all of the opa conversion
> > > functions be eliminated in favor of the ib ones? It looks like
> > > OPA_MTU_8192 and OPA_MTU_10240 are used in a few places within
> > > drivers/infiniband/hw/hfi1, should all of those instances be converted
> > > over to IB_MTU_* and the defines at the top of
> > > drivers/infiniband/hw/hfi1/hfi.h be eliminated?
> > 
> > We rather keep separation due to logic separation.
> > 
> > While ib_* defines come from IBTA and interoperable across different
> > devices and vendors, opa_* definitions are Intel proprietary ones used
> > for the product that was canceled.
> > 
> > Thanks
> > 
> > >
> > > Cheers,
> > > Nathan
> 
> Fair enough, could someone take care of properly fixing the warning that
> was introduced by this patch then? I can send a patch that just adds an
> explicit cast but that looks like an eye sore to me. Otherwise, I am not
> familiar enough with this code to know what is an appropriate fix or
> not.

Leon is correct.  opa_* values should only be used on devices which are opa.

IPoIB needs to be 'opa aware' to do the right thing (be more efficient in this
case) when running on an OPA device.

Is that the case with the current code?

Ira

> 
> Cheers,
> Nathan