[openib-general] [PATCH v2 1/7] IB/core - Add DMA mapping functions to allow device drivers to interpose

[Ralph Campbell]

> On 11/30/06, Ralph Campbell <ralph.campbell at qlogic.com> wrote:
>> On Thu, 2006-11-30 at 12:10 -0800, Roland Dreier wrote:
>> > So what did you change since v1?  How do you deal with fitting 64-bit
>> > addresses into an sg list entry that has a 32-bit dma_addr_t?
>
>> The ipath_map_sg() handler for ib_dma_map_sg() doesn't store
>> anything in the struct scatterlist.  The translation is
>> done when ipath_sg_dma_address() is called which now
>> returns u64 instead of dma_addr_t thus avoiding the truncation
>> problem.
>
> And there is this open/TODO of calling kmap(page) on dma mapping time
> (or when ipath_sg_dma_address is called) and kunmap(page) on dma
> unmapping time, where you must store the kvaddr between the two calls
> and the sg does not have a room for it where dma_addr_t is u32 and
> kvaddr is u64 ....

Although the driver compiles on 32-bit kernels, it is unsupported
and never been tested. All known 64-bit systems don't define
CONFIG_HIGHMEM.  In spite of previous emails suggesting that
page_address() can return NULL without CONFIG_HIGHMEM defined,
the code in include/linux/mm.h doesn't allow it (assuming the
page pointer is valid and not some random address).
I verified this with Andrew Morton.

I don't see value in adding code which will be unsupported
and untested.

>> All of the callers to ib_dma_map_single(), ib_dma_map_page(),
>> and ib_sg_dma_address() have been modifed to save the address
>> in a u64 instead of a dma_addr_t.  This actually wasn't much
>> of a change since the address was being cast to u64 anway
>> when assigned to struct sge.addr.
>
> Its fixes a bug, so it actually somehow much of a change. Without it
> on arch as mentioned above, ipath_dma_map_single would return only a
> u32 portion of the kvaddr and later the ulp code would place this
> chopped address in sge.addr and the ipath driver would use the wrong
> address.
>
> Or.

I only meant that the change was minor compared to the previous
patches sent.  Of course, fixing a bug is important and not minor.