> On 20 Jun 2021, at 14:19, Mark Zhang <markzhang@xxxxxxxxxx> wrote:
>
> On 6/17/2021 11:46 PM, Håkon Bugge wrote:
>> External email: Use caution opening links or attachments
>> 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().
>
> The comment in cma_modify_qp_rtr() says:
> /* Need to update QP attributes from default values. */
>
> So maybe both are needed? E.g., qp_attr->qp_access_flags maybe updated in cm_init_qp_init_attr()?
I'll give you two reasons why that is not the case :-)
1. In cm_init_qp_init_attr(), which sets the mask both places, the mask is set hard to
IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PKEY_INDEX | IB_QP_PORT
If we do the old RESET -> INIT -> INIT, it is the last modify_qp to INIT which will persist. And therefore, the values will be the same if we skip RESET -> INIT.
2. I think the rationale behind modifying the QP to INIT in rdma_create_qp() was to enable ULPs to tweak some of the values. But no ULP calls modify_qp on a QP created by rdma_create_qp(). And if one did, the values would be overwritten when the state transitions to RTR.
Thxs, Håkon
>
>> Signed-off-by: Håkon Bugge <haakon.bugge@xxxxxxxxxx>
>> ---
>> drivers/infiniband/core/cma.c | 15 ---------------
>> 1 file changed, 15 deletions(-)
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 2b9ffc2..937e44e 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -925,19 +925,6 @@ 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)
>> {
>> @@ -960,8 +947,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;
>> --
>> 1.8.3.1
>
