Hi Catalin,
On 14/08/2015 16:30, Catalin Marinas wrote:
>>>> -static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
>>>> - dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)
>>>> +static void *__arm_iommu_alloc_attrs(struct device *dev, size_t size,
>>>> + dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs,
>>>> + bool is_coherent)
>>>> {
>>>> pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
>>>> struct page **pages;
>>>> @@ -1381,7 +1395,7 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
>>>> */
>>>> gfp &= ~(__GFP_COMP);
>>>>
>>>> - pages = __iommu_alloc_buffer(dev, size, gfp, attrs);
>>>> + pages = __iommu_alloc_buffer(dev, size, gfp, attrs, is_coherent);
>>>> if (!pages)
>>>> return NULL;
>>>>
>>>
>>> I can see you are trying to fix the iommu_alloc code to cope with
>>> coherent devices. I think it's currently broken and if you want to fix
>>
>> Could you explain what is broken exactly?
>> The way I tried to deal with coherency or something more low level?
>
> The problem is that __iommu_alloc_atomic() allocates from a
> non-cacheable pool, which is fine when the device is not cache coherent
> but won't work as expected in the is_coherent case (the CPU and device
> must access the memory using the same cacheability attributes).
Thanks for the explanation.
>
>>> In arm_iommu_alloc_attrs(), there is an __iommu_alloc_atomic() call
>>> which goes to the non-cacheable pool, it should use
>>> __alloc_simple_buffer() instead if is_coherent.
>>
>> __iommu_alloc_atomic is called when GFP_WAIT is not set meaning that
>> we can't sleep, but _alloc_simple_buffer can sleep. I follow the calls
>> inside this function and at a point we found a call to the might_sleep_if
>> macro:
>> http://lxr.free-electrons.com/source/mm/page_alloc.c#L2859
>
> This might sleep thing is:
>
> might_sleep_if(gfp_mask & __GFP_WAIT);
>
> So it only sleeps if the __GFP_WAIT is set, but we would only call
> __alloc_simple_buffer() in the !__GFP_WAIT case, so there is no
> sleeping.
Yes my bad I inverted the logic at a point!
>
> The reason to call __alloc_simple_buffer() (in the atomic alloc case) is
> to get cacheable memory, unlike __alloc_from_pool() which only returns
> memory from the non-cacheable pool.
>
>> The _alloc_simple_buffer function ended by calling gen_pool_alloc
>> which won't sleep so from my point of view it is still the right
>> function to call. Did I miss something?
>
> It's not just about sleep vs atomic but about cacheable vs non-cacheable
> as well.
>
> As a quick fix, below is what I meant. It needs extra snippets from your
> patch to add arm_iommu_alloc_attrs_(coherent|noncoherent) and it's not
> tested, but just to give you an idea:
I am fine to make it a proper patch following my 1st patch. However I don't have
a platform with IOMMU so the test would have to be done by people ahving such
platforms.
Gregory
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 9f509b264346..e20a31d45ff3 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1335,13 +1335,16 @@ static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
> return NULL;
> }
>
> -static void *__iommu_alloc_atomic(struct device *dev, size_t size,
> - dma_addr_t *handle)
> +static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
> + dma_addr_t *handle, bool is_coherent)
> {
> struct page *page;
> void *addr;
>
> - addr = __alloc_from_pool(size, &page);
> + if (is_coherent)
> + addr = __alloc_simple_buffer(dev, size, gfp, &page);
> + else
> + addr = __alloc_from_pool(size, &page);
> if (!addr)
> return NULL;
>
> @@ -1356,15 +1359,18 @@ err_mapping:
> return NULL;
> }
>
> -static void __iommu_free_atomic(struct device *dev, void *cpu_addr,
> - dma_addr_t handle, size_t size)
> +static void __iommu_free_simple(struct device *dev, void *cpu_addr,
> + dma_addr_t handle, size_t size, bool is_coherent)
> {
> __iommu_remove_mapping(dev, handle, size);
> - __free_from_pool(cpu_addr, size);
> + if (is_coherent)
> + __dma_free_buffer(virt_to_page(cpu_addr), size);
> + else
> + __free_from_pool(cpu_addr, size);
> }
>
> -static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
> - dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)
> +static void *__arm_iommu_alloc_attrs(struct device *dev, size_t size,
> + dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs, bool is_coherent)
> {
> pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
> struct page **pages;
> @@ -1373,8 +1379,8 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
> *handle = DMA_ERROR_CODE;
> size = PAGE_ALIGN(size);
>
> - if (!(gfp & __GFP_WAIT))
> - return __iommu_alloc_atomic(dev, size, handle);
> + if (is_coherent || !(gfp & __GFP_WAIT))
> + return __iommu_alloc_simple(dev, size, gfp, handle, is_coherent);
>
> /*
> * Following is a work-around (a.k.a. hack) to prevent pages
> @@ -1440,14 +1446,14 @@ static int arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
> * free a page as defined by the above mapping.
> * Must not be called with IRQs disabled.
> */
> -void arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> - dma_addr_t handle, struct dma_attrs *attrs)
> +void __arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> + dma_addr_t handle, struct dma_attrs *attrs, bool is_coherent)
> {
> struct page **pages;
> size = PAGE_ALIGN(size);
>
> - if (__in_atomic_pool(cpu_addr, size)) {
> - __iommu_free_atomic(dev, cpu_addr, handle, size);
> + if (is_coherent || __in_atomic_pool(cpu_addr, size)) {
> + __iommu_free_simple(dev, cpu_addr, handle, size, is_coherent);
> return;
> }
>
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html