On Fri, Apr 08, 2022 at 03:24:40PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> > -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> > {
> > struct ib_sa_mcmember_rec rec;
> > int ret = 0;
> >
> > - if (id_priv->qkey) {
> > - if (qkey && id_priv->qkey != qkey)
> > - return -EINVAL;
> > - return 0;
> > - }
> > -
> > - if (qkey) {
> > - id_priv->qkey = qkey;
> > - return 0;
> > - }
> > -
> > switch (id_priv->id.ps) {
> > case RDMA_PS_UDP:
> > case RDMA_PS_IB:
> > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > default:
> > break;
> > }
> > +
> > return ret;
> > }
> >
> > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> > +{
> > + if (!qkey)
> > + return cma_set_default_qkey(id_priv);
>
> This should be called in the couple of places that are actually
> allowed to set a default qkey. We have some confusion about when that
> is supposed to happen and when a 0 qkey can be presented.
>
> But isn't this not the same? The original behavior was to make the
> set_default a NOP if the id_priv already had a qkey:
>
> - if (id_priv->qkey) {
> - if (qkey && id_priv->qkey != qkey)
>
> But that is gone now?
When I reviewed, I got an impression what once we create id_priv and set
qkey to default values, we won't hit this if (..).
>
> I got this:
>
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 3e315fc0ac16cb..ef980ea7153e51 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -1102,7 +1102,7 @@ static int cma_ib_init_qp_attr(struct rdma_id_private *id_priv,
> *qp_attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_PORT;
>
> if (id_priv->id.qp_type == IB_QPT_UD) {
> - ret = cma_set_qkey(id_priv, 0);
> + ret = cma_set_default_qkey(id_priv);
This is ok
> if (ret)
> return ret;
>
> @@ -4430,14 +4430,10 @@ int rdma_accept(struct rdma_cm_id *id, struct rdma_conn_param *conn_param)
>
> if (rdma_cap_ib_cm(id->device, id->port_num)) {
> if (id->qp_type == IB_QPT_UD) {
> - if (conn_param)
> - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> - conn_param->qkey,
> - conn_param->private_data,
> - conn_param->private_data_len);
> - else
> - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> - 0, NULL, 0);
> + ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS,
> + conn_param->qkey,
> + conn_param->private_data,
> + conn_param->private_data_len);
It is ok too and we have many other places with not-possible "if (conn_param)".
> } else {
> if (conn_param)
> ret = cma_accept_ib(id_priv, conn_param);
> @@ -4685,7 +4681,7 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> if (ret)
> return ret;
>
> - ret = cma_set_qkey(id_priv, 0);
> + ret = cma_set_default_qkey(id_priv);
> if (ret)
> return ret;
>
>
> > static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> > {
> > dev_addr->dev_type = ARPHRD_INFINIBAND;
> > @@ -4762,8 +4764,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
> >
> > ib.rec.pkey = cpu_to_be16(0xffff);
> > - if (id_priv->id.ps == RDMA_PS_UDP)
> > - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > + ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
>
> Why isn't this symetrical with the IB side:
>
> ret = cma_set_default_qkey(id_priv);
> if (ret)
> return ret;
> rec.qkey = cpu_to_be32(id_priv->qkey);
>
>
> ??
The original code didn't touch id_priv.
>
> It fells like set_default_qkey() is the right thing to do incase the
> qkey was already set by something, just as IB does it.
>
> > @@ -4815,6 +4816,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
> > READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED))
> > return -EINVAL;
> >
> > + if (id_priv->id.qp_type != IB_QPT_UD)
> > + return -EINVAL;
> > +
>
> This makes sense
>
> Jason
