diff mbox series

[for-next,1/2] RDMA/cma: Remove unnecessary INIT->INIT transition

Message ID 1624368030-23214-2-git-send-email-haakon.bugge@oracle.com (mailing list archive)
State Superseded
Headers show
Series Fix RMW to bit-fields and remove one superfluous ib_modify_qp | expand

Commit Message

Haakon Bugge June 22, 2021, 1:20 p.m. UTC 
In rdma_create_qp(), a connected QP will be transitioned to the INIT
state.

Afterwards, the QP will be transitioned to the RTR state by the
cma_modify_qp_rtr() function. But this function starts by performing
an ib_modify_qp() to the INIT state again, before another
ib_modify_qp() is performed to transition the QP to the RTR state.

Hence, there is no need to transition the QP to the INIT state in
rdma_create_qp().

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

---

	v1 -> v2:
	   * Fixed uninitialized ret variable as:
	     Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/infiniband/core/cma.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

Comments

Jason Gunthorpe June 22, 2021, 2:47 p.m. UTC | #1 
On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote:
> In rdma_create_qp(), a connected QP will be transitioned to the INIT
> state.
> 
> Afterwards, the QP will be transitioned to the RTR state by the
> cma_modify_qp_rtr() function. But this function starts by performing
> an ib_modify_qp() to the INIT state again, before another
> ib_modify_qp() is performed to transition the QP to the RTR state.

This makes me really nervous that something depends on this since the
API is split up??

Jason
Haakon Bugge June 22, 2021, 2:53 p.m. UTC | #2 
> On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote:
>> In rdma_create_qp(), a connected QP will be transitioned to the INIT
>> state.
>> 
>> Afterwards, the QP will be transitioned to the RTR state by the
>> cma_modify_qp_rtr() function. But this function starts by performing
>> an ib_modify_qp() to the INIT state again, before another
>> ib_modify_qp() is performed to transition the QP to the RTR state.
> 
> This makes me really nervous that something depends on this since the
> API is split up??

As I commented to Mark, no ULP creates a connected QP with rdma_create_qp() and thereafter modifies it with an INIT -> INIT transition. And if it did, the values modified would be overwritten by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is called.


Thxs, Håkon
Jason Gunthorpe June 22, 2021, 2:59 p.m. UTC | #3 
On Tue, Jun 22, 2021 at 02:53:33PM +0000, Haakon Bugge wrote:
> 
> 
> > On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote:
> >> In rdma_create_qp(), a connected QP will be transitioned to the INIT
> >> state.
> >> 
> >> Afterwards, the QP will be transitioned to the RTR state by the
> >> cma_modify_qp_rtr() function. But this function starts by performing
> >> an ib_modify_qp() to the INIT state again, before another
> >> ib_modify_qp() is performed to transition the QP to the RTR state.
> > 
> > This makes me really nervous that something depends on this since the
> > API is split up??
> 
> As I commented to Mark, no ULP creates a connected QP with
> rdma_create_qp() and thereafter modifies it with an INIT -> INIT
> transition. And if it did, the values modified would be overwritten
> by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is
> called.

Does anything call query_qp?

Jason
Haakon Bugge June 22, 2021, 3:44 p.m. UTC | #4 
> On 22 Jun 2021, at 16:59, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Tue, Jun 22, 2021 at 02:53:33PM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>> 
>>> On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote:
>>>> In rdma_create_qp(), a connected QP will be transitioned to the INIT
>>>> state.
>>>> 
>>>> Afterwards, the QP will be transitioned to the RTR state by the
>>>> cma_modify_qp_rtr() function. But this function starts by performing
>>>> an ib_modify_qp() to the INIT state again, before another
>>>> ib_modify_qp() is performed to transition the QP to the RTR state.
>>> 
>>> This makes me really nervous that something depends on this since the
>>> API is split up??
>> 
>> As I commented to Mark, no ULP creates a connected QP with
>> rdma_create_qp() and thereafter modifies it with an INIT -> INIT
>> transition. And if it did, the values modified would be overwritten
>> by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is
>> called.
> 
> Does anything call query_qp?

iser_connected_handler() does, but that's when an RDMA_CM_EVENT_ESTABLISHED has been received, so the QP state is already RTS.


Håkon
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index ab148a6..e3f52c5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -926,25 +926,12 @@  static int cma_init_ud_qp(struct rdma_id_private *id_priv, struct ib_qp *qp)
 	return ret;
 }
 
-static int cma_init_conn_qp(struct rdma_id_private *id_priv, struct ib_qp *qp)
-{
-	struct ib_qp_attr qp_attr;
-	int qp_attr_mask, ret;
-
-	qp_attr.qp_state = IB_QPS_INIT;
-	ret = rdma_init_qp_attr(&id_priv->id, &qp_attr, &qp_attr_mask);
-	if (ret)
-		return ret;
-
-	return ib_modify_qp(qp, &qp_attr, qp_attr_mask);
-}
-
 int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd,
 		   struct ib_qp_init_attr *qp_init_attr)
 {
 	struct rdma_id_private *id_priv;
 	struct ib_qp *qp;
-	int ret;
+	int ret = 0;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
 	if (id->device != pd->device) {
@@ -961,8 +948,6 @@  int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd,
 
 	if (id->qp_type == IB_QPT_UD)
 		ret = cma_init_ud_qp(id_priv, qp);
-	else
-		ret = cma_init_conn_qp(id_priv, qp);
 	if (ret)
 		goto out_destroy;