Re: Re: [PATCH v3] RDMA/siw: Fix 64/32bit pointer inconsistency

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

On Fri, 2019-08-23 at 15:05 +0000, Bernard Metzler wrote:
> -----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
> 
> > To: "Bernard Metzler" <BMT@zurich.ibm.com>
> > From: "Jason Gunthorpe" <jgg@ziepe.ca>
> > Date: 08/23/2019 04:42PM
> > Cc: "Doug Ledford" <dledford@redhat.com>, "Leon Romanovsky"
> > <leon@kernel.org>, linux-rdma@vger.kernel.org, geert@linux-m68k.org
> > 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@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]