Tom Talpey wrote:
> At 02:57 PM 4/26/2009, Steve Wise wrote:
>
>> Hey Trond,
>>
>> Turns out the server side is fine. So this patch is good to go.
>>
>
> I'll ACK the patch, but I do wonder if the code will compile cleanly on all
> platforms. The iova_start is a u64, whereas the mr_dma is a dma_addr_t,
> which is variable sized, depending on platform. Would a (u64) cast be a
> safer patch, warning-wise?
>
>
Well, if dma_addr_t is smaller, I don't think you'll get a warning, will
you? And if its larger than 64b, then you _want_ the warning, because
nothing will work. :)
Its up to you and/or Trond.
Steve.
> Tom.
>
>
>> Thanks,
>>
>> Steve.
>>
>> Steve Wise wrote:
>>
>>> Trond,
>>>
>>> There is a similar bug in the server side too. I'll repost a single
>>> patch that fixes both.
>>>
>>> Steve.
>>>
>>> Steve Wise wrote:
>>>
>>>> A bad cast causes the iova_start, which in this case is a DMA bus
>>>> address,
>>>> to be truncated on 32b systems. No cast is needed.
>>>>
>>>> Signed-off-by: Steve Wise <[email protected]>
>>>> ---
>>>>
>>>> net/sunrpc/xprtrdma/verbs.c | 2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
>>>> index 3b21e0c..3a374f5 100644
>>>> --- a/net/sunrpc/xprtrdma/verbs.c
>>>> +++ b/net/sunrpc/xprtrdma/verbs.c
>>>> @@ -1489,7 +1489,7 @@ rpcrdma_register_frmr_external(struct
>>>> rpcrdma_mr_seg *seg,
>>>> memset(&frmr_wr, 0, sizeof frmr_wr);
>>>> frmr_wr.opcode = IB_WR_FAST_REG_MR;
>>>> frmr_wr.send_flags = 0; /* unsignaled */
>>>> - frmr_wr.wr.fast_reg.iova_start = (unsigned long)seg1->mr_dma;
>>>> + frmr_wr.wr.fast_reg.iova_start = seg1->mr_dma;
>>>> frmr_wr.wr.fast_reg.page_list =
>>>> seg1->mr_chunk.rl_mw->r.frmr.fr_pgl;
>>>> frmr_wr.wr.fast_reg.page_list_len = i;
>>>> frmr_wr.wr.fast_reg.page_shift = PAGE_SHIFT;
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
On Mon, 2009-04-27 at 12:37 -0500, Steve Wise wrote:
> Tom Talpey wrote:
> > At 02:57 PM 4/26/2009, Steve Wise wrote:
> >
> >> Hey Trond,
> >>
> >> Turns out the server side is fine. So this patch is good to go.
> >>
> >
> > I'll ACK the patch, but I do wonder if the code will compile cleanly on all
> > platforms. The iova_start is a u64, whereas the mr_dma is a dma_addr_t,
> > which is variable sized, depending on platform. Would a (u64) cast be a
> > safer patch, warning-wise?
> >
> >
>
> Well, if dma_addr_t is smaller, I don't think you'll get a warning, will
> you? And if its larger than 64b, then you _want_ the warning, because
> nothing will work. :)
>
> Its up to you and/or Trond.
It looks looks as though the bug is really that the IB code is using a
u64 to store dma handles. As an external user of the IB api, we really
shouldn't have to perform this sort of transformation. If it is
absolutely necessary, then it should be done by means of specialised
accessor functions to initialise/read iova_start value when given a
dma_addr_t.
I'd therefore prefer the no-cast version (with eventual compiler
warnings), in the hope that eventually the IB folks will fix their
interface.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com