On Fri, 2019-08-23 at 15:05 +0000, Bernard Metzler wrote:
> -----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----
>
> > To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
> > From: "Jason Gunthorpe" <jgg@xxxxxxxx>
> > Date: 08/23/2019 04:42PM
> > Cc: "Doug Ledford" <dledford@xxxxxxxxxx>, "Leon Romanovsky"
> > <leon@xxxxxxxxxx>, linux-rdma@xxxxxxxxxxxxxxx, geert@xxxxxxxxxxxxxx
> > Subject: [EXTERNAL] Re: [PATCH v3] RDMA/siw: Fix 64/32bit pointer
> > inconsistency
> >
> > On Fri, Aug 23, 2019 at 02:33:56PM +0000, Bernard Metzler wrote:
> > > > > > diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> > > > > > b/drivers/infiniband/sw/siw/siw_cm.c
> > > > > > index 9ce8a1b925d2..ae7ea3ad7224 100644
> > > > > > +++ b/drivers/infiniband/sw/siw/siw_cm.c
> > > > > > @@ -355,8 +355,8 @@ static int siw_cm_upcall(struct siw_cep
> > *cep,
> > > > > > enum iw_cm_event_type reason,
> > > > > > getname_local(cep->sock, &event.local_addr);
> > > > > > getname_peer(cep->sock, &event.remote_addr);
> > > > > > }
> > > > > > - siw_dbg_cep(cep, "[QP %u]: id 0x%p, reason=%d,
> > > > > > status=%d\n",
> > > > > > - cep->qp ? qp_id(cep->qp) : -1, id, reason,
> > > > > > status);
> > > > > > + siw_dbg_cep(cep, "[QP %u]: reason=%d, status=%d\n",
> > > > > > + cep->qp ? qp_id(cep->qp) : -1, reason,
> > > > > > status);
> > > > > ^^^^
> > > > > There is a chance that such construction (attempt to print -1
> > with
> > > > %u)
> > > > > will generate some sort of warning.
> > > > >
> > > > > Thanks
> > > >
> > > > I didn't see any warnings when I built it. And %u->-1 would be
> > the
> > > > same
> > > > error on 64bit or 32bit, so I think we're safe here.
> > > >
> > >
> > > Doug,
> > > May I ask you to amend this patch in a way which would
> > > just stop this monument of programming stupidity from
> > > prolonging into the future, while of course recognizing
> > > the impossibility of erasing it from the past?
> > > Exchanging the %u with %d would help me regaining
> > > some self-confidence ;)
> >
> > A
> > q?a:b
> >
> > Expression has only a single type. There are some tricky rules on
> > this, but since gcc does not complain on the %u it means
> > 'q?(u32):(int)' is a (u32) and the -1 is implicitly casted.
> >
> > The better thing to write would have been U32_MAX instead of -1
> >
>
> What I wanted to have though is an easy to spot invalid number
> for the QP. This is why I wanted to have it a negative number
> on the screen, which is obviously not nicely achievable. So,
> yeah, U32_MAX is a better idea. It will not very often be a
> valid QP ID...
Given that this patch was still the top of my tree, I fixed this up.
But, I think U32_MAX is wrong. It should be UINT_MAX (which is what I
used). Otherwise it will give errors on s390 where an int is 31 bits
(and anywhere else that might have a non-32 bit int).
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
