lkml.org 
[lkml]   [2009]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH V2 2/3] powerpc: Add support for swiotlb on 32-bit
Date

On May 21, 2009, at 12:43 PM, Jeremy Fitzhardinge wrote:

> Becky Bruce wrote:
>>>
>>>
>>> If we have something like in arch/{x86|ia64|powerpc}/dma-mapping.h:
>>>
>>> static inline int is_buffer_dma_capable(struct device *dev,
>>> dma_addr_t addr, size_t size)
>>>
>>> then we don't need two checking functions, address_needs_mapping and
>>> range_needs_mapping.
>>
>> It's never been clear to me *why* we had both in the first place -
>> if you can explain this, I'd be grateful :)
>
> I was about to ask the same thing. It seems that
> range_needs_mapping should be able to do both jobs.
>
> I think range_needs_mapping came from the Xen swiotlb changes, and
> address_needs_mapping came from your powerpc changes. Many of the
> changes were exact overlaps; I think this was one of the few
> instances where there was a difference.

I think address_needs_mapping was already there and I added the
ability for an arch to provide its own version. Ian added
range_needs_mapping in commit b81ea27b2329bf44b. At the time, it
took a virtual address as its argument, so we couldn't use it for
highmem. That's since been changed to phys_addr_t, so I think we
should be able to merge the two.

>
> We need a range check in Xen (rather than iterating over individual
> pages) because we want to check that the underlying pages are
> machine contiguous, but I think that's also sufficient to do
> whatever checks you need to do.

Yes.

>
> The other difference is that is_buffer_dma_capable operates on a
> dma_addr_t, which presumes that you can generate a dma address and
> then test for its validity. For Xen, it doesn't make much sense to
> talk about the dma_addr_t for memory which isn't actually dma-
> capable; we need the test to be in terms of phys_addr_t. Given that
> the two functions are always called from the same place, that
> doesn't seem to pose a problem.
>
> So I think the unified function would be something like:
>
> int range_needs_mapping(struct device *hwdev, phys_addr_t addr,
> size_t size);
>
> which would be defined somewhere under asm/*.h. Would that work for
> powerpc?

I can work with that, but it's going to be a bit inefficient, as I
actually need the dma_addr_t, not the phys_addr_t, so I'll have to
convert. In every case, this is a conversion I've already done and
that I need in the calling code as well. Can we pass in both the
phys_addr_t and the dma_addr_t? We have both in every case but one,
which is in swiotlb_map_page where we call address_needs_mapping()
without calling range_needs_mapping. It's not actually clear to me
that we need that check, though. Can someone explain what case that
was designed to catch?

Cheers,
Becky



\
 
 \ /
  Last update: 2009-05-21 20:31    [W:0.183 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site