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

Bernard Metzler
2019-08-23T15:05:24Z

-----"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...

Thanks
Bernard.