Date:   Mon, 24 Oct 2016 12:16:16 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Cc:     Alexander Duyck <alexander.h.duyck@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH RFC 02/26] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

On Mon, Oct 24, 2016 at 11:09 AM, Konrad Rzeszutek Wilk
<konrad.wilk@...cle.com> 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 <konrad.wilk@...cle.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
>> ---
>>  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

Powered by blists - more mailing lists