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

[Ingo Molnar <mingo@elte.hu>]

* 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