Re: [PATCH 3/5] swiotlb: swiotlb_bus_to_virt/phys prototype changes

Ingo Molnar
2009-03-25T12:44:55Z

* Becky Bruce <beckyb@kernel.crashing.org> wrote:

>
> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>
>> On Tue, 24 Mar 2009 16:28:44 -0500
>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>
>>> Make these functions take the hwdev as an argument because on some
>>> platforms it contains a per-device offset that is used to convert
>>> from bus addresses to/from other types of addresses.
>>>
>>> Also, make these weak so architectures can override the default
>>> behavior (for example, by adding an offset in the hwdev).
>>>
>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>> ---
>>> arch/x86/kernel/pci-swiotlb.c | 2 +-
>>> include/linux/swiotlb.h | 4 +++-
>>> lib/swiotlb.c | 31 +++++++++++++++++++------------
>>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>
>>> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev,
>>> struct scatterlist *sgl,
>>>
>>> for_each_sg(sgl, sg, nelems, i) {
>>> if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>>> - unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
>>> - sg->dma_length, dir);
>>> + unmap_single(
>>> + hwdev,
>>> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
>>> + sg->dma_length, dir);
>>> else if (dir == DMA_FROM_DEVICE)
>>> - dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg-
>>> >dma_length);
>>> + dma_mark_clean(
>>> + swiotlb_bus_to_virt(hwdev, sg->dma_address),
>>> + sg->dma_length);
>>> }
>>
>> The coding style looks a bit odd to me. How about something like this?
>
>>
>>
>> for_each_sg(sgl, sg, nelems, i) {
>> void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>> unmap_single(hwdev, virt, sg->dma_length, dir);
>> else if (dir == DMA_FROM_DEVICE)
>> dma_mark_clean(virt, sg->dma_length);

that looks odd too.

> Heh, I was trying to avoid > 80 character lines there, and it
> ended up looking a bit gross. I originally had *exactly* what you
> suggest in my tree, but changed it, so I'm more than happy to
> change this back.

The proper solution is to split out the loop body into a helper
function, __swiotlb_unmap_sg_attrs() or so. Something like:

__swiotlb_unmap_sg_attrs()
{
if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
unmap_single(hwdev,
swiotlb_bus_to_virt(hwdev, sg->dma_address),
sg->dma_length, dir);
return;
}

if (dir != DMA_FROM_DEVICE)
return;

dma_mark_clean(swiotlb_bus_to_virt(hwdev, sg->dma_address),
sg->dma_length);
}

Ingo