Re: [net-next PATCH RFC 02/26] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

[Konrad Rzeszutek Wilk]

On Mon, Oct 24, 2016 at 08:04:37AM -0400, Alexander Duyck wrote:
> As a first step to making DMA_ATTR_SKIP_CPU_SYNC apply to architectures
> beyond just ARM I need to make it so that the swiotlb will respect the
> flag.  In order to do that I also need to update the swiotlb-xen since it
> heavily makes use of the functionality.
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@xxxxxxxxx>
> ---
>  drivers/xen/swiotlb-xen.c |   40 ++++++++++++++++++++++----------------
>  include/linux/swiotlb.h   |    6 ++++--
>  lib/swiotlb.c             |   48 +++++++++++++++++++++++++++------------------
>  3 files changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 87e6035..cf047d8 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -405,7 +405,8 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
>  	 */
>  	trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>  
> -	map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir);
> +	map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
> +				     attrs);
>  	if (map == SWIOTLB_MAP_ERROR)
>  		return DMA_ERROR_CODE;
>  
> @@ -416,11 +417,13 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
>  	/*
>  	 * Ensure that the address returned is DMA'ble
>  	 */
> -	if (!dma_capable(dev, dev_addr, size)) {
> -		swiotlb_tbl_unmap_single(dev, map, size, dir);
> -		dev_addr = 0;
> -	}
> -	return dev_addr;
> +	if (dma_capable(dev, dev_addr, size))
> +		return dev_addr;
> +
> +	swiotlb_tbl_unmap_single(dev, map, size, dir,
> +				 attrs | DMA_ATTR_SKIP_CPU_SYNC);
> +
> +	return DMA_ERROR_CODE;

Why? This change (re-ordering the code - and returning DMA_ERROR_CODE instead
of 0) does not have anything to do with the title.

If you really feel strongly about it - then please send it as a seperate patch.
>  }
>  EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
>  
> @@ -444,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
>  
>  	/* NOTE: We use dev_addr here, not paddr! */
>  	if (is_xen_swiotlb_buffer(dev_addr)) {
> -		swiotlb_tbl_unmap_single(hwdev, paddr, size, dir);
> +		swiotlb_tbl_unmap_single(hwdev, paddr, size, dir, attrs);
>  		return;
>  	}
>  
> @@ -557,16 +560,9 @@ void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
>  								 start_dma_addr,
>  								 sg_phys(sg),
>  								 sg->length,
> -								 dir);
> -			if (map == SWIOTLB_MAP_ERROR) {
> -				dev_warn(hwdev, "swiotlb buffer is full\n");
> -				/* Don't panic here, we expect map_sg users
> -				   to do proper error handling. */
> -				xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
> -							   attrs);
> -				sg_dma_len(sgl) = 0;
> -				return 0;
> -			}
> +								 dir, attrs);
> +			if (map == SWIOTLB_MAP_ERROR)
> +				goto map_error;
>  			xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT),
>  						dev_addr,
>  						map & ~PAGE_MASK,
> @@ -589,6 +585,16 @@ void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
>  		sg_dma_len(sg) = sg->length;
>  	}
>  	return nelems;
> +map_error:
> +	dev_warn(hwdev, "swiotlb buffer is full\n");
> +	/*
> +	 * Don't panic here, we expect map_sg users
> +	 * to do proper error handling.
> +	 */
> +	xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
> +				   attrs | DMA_ATTR_SKIP_CPU_SYNC);
> +	sg_dma_len(sgl) = 0;
> +	return 0;
>  }

This too. Why can't that be part of the existing code that was there?