-----"Doug Ledford" <dledford@xxxxxxxxxx> wrote: -----
>To: "Leon Romanovsky" <leon@xxxxxxxxxx>, "Bernard Metzler"
><bmt@xxxxxxxxxxxxxx>
>From: "Doug Ledford" <dledford@xxxxxxxxxx>
>Date: 08/22/2019 09:08PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx, jgg@xxxxxxxx, geert@xxxxxxxxxxxxxx
>Subject: [EXTERNAL] Re: [PATCH v3] RDMA/siw: Fix 64/32bit pointer
>inconsistency
>
>On Thu, 2019-08-22 at 21:41 +0300, Leon Romanovsky wrote:
>> On Thu, Aug 22, 2019 at 07:37:38PM +0200, Bernard Metzler wrote:
>> > Fixes improper casting between addresses and unsigned types.
>> > Changes siw_pbl_get_buffer() function to return appropriate
>> > dma_addr_t, and not u64.
>> >
>> > Also fixes debug prints. Now any potentially kernel private
>> > pointers are printed formatted as '%pK', to allow keeping that
>> > information secret.
>> >
>> > Fixes: d941bfe500be ("RDMA/siw: Change CQ flags from 64->32
>bits")
>> > Fixes: b0fff7317bb4 ("rdma/siw: completion queue methods")
>> > Fixes: 8b6a361b8c48 ("rdma/siw: receive path")
>> > Fixes: b9be6f18cf9e ("rdma/siw: transmit path")
>> > Fixes: f29dd55b0236 ("rdma/siw: queue pair methods")
>> > Fixes: 2251334dcac9 ("rdma/siw: application buffer management")
>> > Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
>> > Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
>> > Fixes: a531975279f3 ("rdma/siw: main include file")
>> >
>> > Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>> > Reported-by: Jason Gunthorpe <jgg@xxxxxxxx>
>> > Reported-by: Leon Romanovsky <leon@xxxxxxxxxx>
>> > Signed-off-by: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
>> > ---
>> > drivers/infiniband/sw/siw/siw.h | 8 +--
>> > drivers/infiniband/sw/siw/siw_cm.c | 74
>++++++++++++----------
>> > -----
>> > drivers/infiniband/sw/siw/siw_cq.c | 5 +-
>> > drivers/infiniband/sw/siw/siw_mem.c | 14 ++---
>> > drivers/infiniband/sw/siw/siw_mem.h | 2 +-
>> > drivers/infiniband/sw/siw/siw_qp.c | 2 +-
>> > drivers/infiniband/sw/siw/siw_qp_rx.c | 26 +++++-----
>> > drivers/infiniband/sw/siw/siw_qp_tx.c | 43 ++++++++--------
>> > drivers/infiniband/sw/siw/siw_verbs.c | 40 +++++++--------
>> > 9 files changed, 106 insertions(+), 108 deletions(-)
>> >
>> > diff --git a/drivers/infiniband/sw/siw/siw.h
>> > b/drivers/infiniband/sw/siw/siw.h
>> > index 77b1aabf6ff3..dba4535494ab 100644
>> > --- a/drivers/infiniband/sw/siw/siw.h
>> > +++ b/drivers/infiniband/sw/siw/siw.h
>> > @@ -138,9 +138,9 @@ struct siw_umem {
>> > };
>> >
>> > struct siw_pble {
>> > - u64 addr; /* Address of assigned user buffer */
>> > - u64 size; /* Size of this entry */
>> > - u64 pbl_off; /* Total offset from start of PBL */
>> > + dma_addr_t addr; /* Address of assigned buffer */
>> > + unsigned int size; /* Size of this entry */
>> > + unsigned long pbl_off; /* Total offset from start of PBL */
>> > };
>> >
>> > struct siw_pbl {
>> > @@ -734,7 +734,7 @@ static inline void siw_crc_skb(struct
>> > siw_rx_stream *srx, unsigned int len)
>> > "MEM[0x%08x] %s: " fmt, mem->stag, __func__,
>> > ##__VA_ARGS__)
>> >
>> > #define siw_dbg_cep(cep, fmt,
>> > ...) \
>> > - ibdev_dbg(&cep->sdev->base_dev, "CEP[0x%p] %s: "
>> > fmt, \
>> > + ibdev_dbg(&cep->sdev->base_dev, "CEP[0x%pK] %s: "
>> > fmt, \
>> > cep, __func__, ##__VA_ARGS__)
>> >
>> > void siw_cq_flush(struct siw_cq *cq);
>> > diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>> > b/drivers/infiniband/sw/siw/siw_cm.c
>> > index 9ce8a1b925d2..ae7ea3ad7224 100644
>> > --- a/drivers/infiniband/sw/siw/siw_cm.c
>> > +++ 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 ;)
Thanks!
Bernard.
>Thanks Bernard, it's in my wip/dl-for-rc branch to get 0day testing.
>
>--
>Doug Ledford <dledford@xxxxxxxxxx>
> GPG KeyID: B826A3330E572FDD
> Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
>
[attachment "signature.asc" removed by Bernard Metzler/Zurich/IBM]
