On Sun, Apr 10, 2022 at 03:03:24PM +0300, Leon Romanovsky wrote:
> 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 (..).
We don't set qkey during create, so I'm not so sure..
The only places setting non-default qkeys are SIDR, maybe nobody uses
SIDR with multicast.
> > > 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.
I know, but I think that is a mistake, we should make it symmetric
Jason
