Re: [PATCH rdma-next v2 3/7] RDMA/ucma: Extend ucma_connect to receive ECE parameters

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Leon Romanovsky <leon@kernel.org>
Cc: Doug Ledford <dledford@redhat.com>, linux-rdma@vger.kernel.org
Subject: Re: [PATCH rdma-next v2 3/7] RDMA/ucma: Extend ucma_connect to receive ECE parameters
Date: Mon, 25 May 2020 15:15:34 -0300	[thread overview]
Message-ID: <20200525181534.GH744@ziepe.ca> (raw)
In-Reply-To: <20200525174739.GH10591@unreal>

On Mon, May 25, 2020 at 08:47:39PM +0300, Leon Romanovsky wrote:
> On Mon, May 25, 2020 at 02:41:41PM -0300, Jason Gunthorpe wrote:
> > On Mon, Apr 13, 2020 at 05:15:34PM +0300, Leon Romanovsky wrote:
> >
> > > -	if (copy_from_user(&cmd, inbuf, sizeof(cmd)))
> > > +	in_size = min_t(size_t, in_len, sizeof(cmd));
> > > +	if (copy_from_user(&cmd, inbuf, in_size))
> > >  		return -EFAULT;
> > >
> > >  	if (!cmd.conn_param.valid)
> > > @@ -1086,8 +1089,13 @@ static ssize_t ucma_connect(struct ucma_file *file, const char __user *inbuf,
> > >  		return PTR_ERR(ctx);
> > >
> > >  	ucma_copy_conn_param(ctx->cm_id, &conn_param, &cmd.conn_param);
> > > +	if (offsetofend(typeof(cmd), ece) <= in_size) {
> > > +		ece.vendor_id = cmd.ece.vendor_id;
> > > +		ece.attr_mod = cmd.ece.attr_mod;
> > > +	}
> >
> > The uapi changes in the prior patch should be placed in the patches
> > that actually implement them, eg one here..
> 
> I wanted to simplify the series and keep its bisectable at the same
> time. Should I squash them?

Yes, the two structs should be introduced in the patches that populate
them, that is the usual wy.

> > > diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
> > > index 71f48cfdc24c..86a849214c84 100644
> > > +++ b/include/rdma/rdma_cm.h
> > > @@ -264,6 +264,17 @@ int rdma_init_qp_attr(struct rdma_cm_id *id, struct ib_qp_attr *qp_attr,
> > >   */
> > >  int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param);
> > >
> > > +/**
> > > + * rdma_connect_ece - Initiate an active connection request with ECE data.
> > > + * @id: Connection identifier to connect.
> > > + * @conn_param: Connection information used for connected QPs.
> > > + * @ece: ECE parameters
> > > + *
> > > + * See rdma_connect() explanation.
> > > + */
> > > +int rdma_connect_ece(struct rdma_cm_id *id, struct rdma_conn_param *conn_param,
> > > +		     struct rdma_ucm_ece *ece);
> >
> > kdoc's go in the C files
> 
> I know, but didn't know if to follow existing pattern or not.

I've been of the opinion that new code should follow the expected
style.

Trying to keep some huge matrix of certain files have certain legacy
exceptions is too hard on my brain.

Jason