Hi Leon,
> -----Original Message-----
> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Sunday, January 20, 2019 11:58 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe
> <jgg@xxxxxxxxxxxx>; Danit Goldberg <danitg@xxxxxxxxxxxx>; RDMA
> mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Moni Shoua
> <monis@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next] IB/cma: Define option to set ack timeout and
> pack tos_set
>
> On Sun, Jan 20, 2019 at 01:08:53AM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> > > owner@xxxxxxxxxxxxxxx> On Behalf Of Leon Romanovsky
> > > Sent: Thursday, January 17, 2019 11:54 AM
> > > To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe
> > > <jgg@xxxxxxxxxxxx>
> > > Cc: Danit Goldberg <danitg@xxxxxxxxxxxx>; RDMA mailing list <linux-
> > > rdma@xxxxxxxxxxxxxxx>; Moni Shoua <monis@xxxxxxxxxxxx>; Leon
> > > Romanovsky <leonro@xxxxxxxxxxxx>
> > > Subject: [PATCH rdma-next] IB/cma: Define option to set ack timeout
> > > and pack tos_set
> > >
> > > From: Danit Goldberg <danitg@xxxxxxxxxxxx>
> > >
> > > Define new option in 'rdma_set_option' to override calculated QP
> > > timeout when requested to provide QP attributes to modify a QP.
> > >
> > > At the same time, pack tos_set to be bitfield.
> > >
> > > Signed-off-by: Danit Goldberg <danitg@xxxxxxxxxxxx>
> > > Reviewed-by: Moni Shoua <monis@xxxxxxxxxxxx>
> > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > ---
> > > drivers/infiniband/core/cma.c | 14 ++++++++++++++
> > > drivers/infiniband/core/cma_priv.h | 4 +++-
> > > drivers/infiniband/core/ucma.c | 7 +++++++
> > > include/rdma/rdma_cm.h | 7 +++++++
> > > include/uapi/rdma/rdma_user_cm.h | 4 ++++
> > > 5 files changed, 35 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/core/cma.c
> > > b/drivers/infiniband/core/cma.c index 3880fbb3bee7..d4c861f64194
> > > 100644
> > > --- a/drivers/infiniband/core/cma.c
> > > +++ b/drivers/infiniband/core/cma.c
> > > @@ -888,6 +888,7 @@ struct rdma_cm_id *__rdma_create_id(struct net
> > > *net,
> > > id_priv->id.ps = ps;
> > > id_priv->id.qp_type = qp_type;
> > > id_priv->tos_set = false;
> > > + id_priv->timeout_set = false;
> > > id_priv->gid_type = IB_GID_TYPE_IB;
> > > spin_lock_init(&id_priv->lock);
> > > mutex_init(&id_priv->qp_mutex);
> > > @@ -1130,6 +1131,9 @@ int rdma_init_qp_attr(struct rdma_cm_id *id,
> > > struct ib_qp_attr *qp_attr,
> > > } else
> > > ret = -ENOSYS;
> > >
> > > + if ((*qp_attr_mask & IB_QP_TIMEOUT) && id_priv->timeout_set)
> > > + qp_attr->timeout = id_priv->timeout;
> > > +
> > > return ret;
> > > }
> > > EXPORT_SYMBOL(rdma_init_qp_attr);
> > > @@ -2490,6 +2494,16 @@ void rdma_set_service_type(struct
> rdma_cm_id
> > > *id, int tos) } EXPORT_SYMBOL(rdma_set_service_type);
> > >
> > > +void rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout) {
> > > + struct rdma_id_private *id_priv;
> > > +
> > > + id_priv = container_of(id, struct rdma_id_private, id);
> > > + id_priv->timeout = timeout;
> > > + id_priv->timeout_set = true;
> > > +}
> > > +EXPORT_SYMBOL(rdma_set_ack_timeout);
> > > +
> > Please change to return error if the cm_id when qp_type != RC.
> > Or move is_qp_type_connected() from core/verbs.c to core_priv.h and
> reuse here.
> >
> > Also validating by modifying is not very elegant way.
> > I actually have a patch that extend several qp params via netlink which will
> allows to verify this as well.
> > I posted in internal gerrit. Please bundle it with v1 version of it.
> > I didn't have iproute2 enhanced so was holding to queue to Leon, but its
> good to do now in along with this patch.
>
> kdoc and checks of rdma_cm_id are very good suggestions and we will do
> them, but netlink is a little bit out-of-scope for this patch. As you said, it will
> be separate patch together with iproute2.
>
I posted both patches in internal gerrit iproute2 user space and kernel part and tested it too.
This allows us to do full code coverage of validating what is being set.
So if you find time for review, please review else can do it as follow on patch.
