Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941483AbcJXTSC (ORCPT ); Mon, 24 Oct 2016 15:18:02 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:32883 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941158AbcJXTQS (ORCPT ); Mon, 24 Oct 2016 15:16:18 -0400 MIME-Version: 1.0 In-Reply-To: <20161024180934.GA24840@char.us.oracle.com> References: <20161024115737.16276.71059.stgit@ahduyck-blue-test.jf.intel.com> <20161024120437.16276.68349.stgit@ahduyck-blue-test.jf.intel.com> <20161024180934.GA24840@char.us.oracle.com> From: Alexander Duyck Date: Mon, 24 Oct 2016 12:16:16 -0700 Message-ID: Subject: Re: [net-next PATCH RFC 02/26] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC To: Konrad Rzeszutek Wilk Cc: Alexander Duyck , Netdev , "linux-kernel@vger.kernel.org" , linux-mm , Jesper Dangaard Brouer , David Miller Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5139 Lines: 113 On Mon, Oct 24, 2016 at 11:09 AM, Konrad Rzeszutek Wilk wrote: > 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 >> Signed-off-by: Alexander Duyck >> --- >> 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. Okay I can do that. This was mostly just to clean up the formatting because I was over 80 characters when I added the attribute. Changing the return value to DMA_ERROR_CODE from 0 was based on the fact that earlier in the function that is the value you return if there is a mapping error. >> } >> 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? Once again it was a formatting thing. I was indented too far and adding the attribute pushed me over 80 characters so I broke it out to a label to avoid the problem. - Alex