Re: [PATCH] ion: scatterlist offset not used for buffer map
> I don't think this is right. A compound_page still has a page struct > for every page, you should be passing the page struct where your data > starts. Using an offset > PAGE_SIZE is going to break lots of places, > for example anywhere that uses kmap(sg_page(sg)). Was kind-of expecting that response. Will update, thanks.
Re: [PATCH] ion: scatterlist offset not used for buffer map
On Thu, Apr 7, 2016 at 11:56 PM, John Einar Reitan wrote: > On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote: >> On 04/07/2016 04:29 AM, John Einar Reitan wrote: >> > ion's default user/kernel page mapping code don't honor the offset >> > option for scatterlists. It uses sg_page and expect the whole page to be >> > mapped, while the offset could dictate an offset within a large page. >> > >> > sg_phys correctly accounts for the offset, so should be used instead. >> > >> >> Can you be more specific about which heap and which allocation pattern >> is exposing this bug? > > The heap that exposed the bug is one I'm developing and will be posting > as a RFC soon. It uses compound pages and an sub-divides it into surface > buffers. The ion buffers are configured to hold sgl's with the compound > page and the correct offset of the buffer, via > sg_set_page(.., compound_page, .., offset_of_logical_buffer); I don't think this is right. A compound_page still has a page struct for every page, you should be passing the page struct where your data starts. Using an offset > PAGE_SIZE is going to break lots of places, for example anywhere that uses kmap(sg_page(sg)). > sg_phys/sg_virt includes this offset, but if you poke the sg and extract > the page with sg_page yourself you must include this offset in your > calculations too.
Re: [PATCH] ion: scatterlist offset not used for buffer map
On 04/07/2016 11:56 PM, John Einar Reitan wrote: On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote: On 04/07/2016 04:29 AM, John Einar Reitan wrote: ion's default user/kernel page mapping code don't honor the offset option for scatterlists. It uses sg_page and expect the whole page to be mapped, while the offset could dictate an offset within a large page. sg_phys correctly accounts for the offset, so should be used instead. Can you be more specific about which heap and which allocation pattern is exposing this bug? The heap that exposed the bug is one I'm developing and will be posting as a RFC soon. It uses compound pages and an sub-divides it into surface buffers. The ion buffers are configured to hold sgl's with the compound page and the correct offset of the buffer, via sg_set_page(.., compound_page, .., offset_of_logical_buffer); sg_phys/sg_virt includes this offset, but if you poke the sg and extract the page with sg_page yourself you must include this offset in your calculations too. This patch should be re-sent when you have the RFC for the heap. Unless there is a heap available in tree we don't really need this patch. Thanks, Laura
Re: [PATCH] ion: scatterlist offset not used for buffer map
On Thu, Apr 07, 2016 at 12:37:50PM -0700, Laura Abbott wrote: > On 04/07/2016 04:29 AM, John Einar Reitan wrote: > > ion's default user/kernel page mapping code don't honor the offset > > option for scatterlists. It uses sg_page and expect the whole page to be > > mapped, while the offset could dictate an offset within a large page. > > > > sg_phys correctly accounts for the offset, so should be used instead. > > > > Can you be more specific about which heap and which allocation pattern > is exposing this bug? The heap that exposed the bug is one I'm developing and will be posting as a RFC soon. It uses compound pages and an sub-divides it into surface buffers. The ion buffers are configured to hold sgl's with the compound page and the correct offset of the buffer, via sg_set_page(.., compound_page, .., offset_of_logical_buffer); sg_phys/sg_virt includes this offset, but if you poke the sg and extract the page with sg_page yourself you must include this offset in your calculations too.
Re: [PATCH] ion: scatterlist offset not used for buffer map
On 04/07/2016 04:29 AM, John Einar Reitan wrote:
ion's default user/kernel page mapping code don't honor the offset
option for scatterlists. It uses sg_page and expect the whole page to be
mapped, while the offset could dictate an offset within a large page.
sg_phys correctly accounts for the offset, so should be used instead.
Can you be more specific about which heap and which allocation pattern
is exposing this bug?
Signed-off-by: John Einar Reitan
---
drivers/staging/android/ion/ion_heap.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/ion/ion_heap.c
b/drivers/staging/android/ion/ion_heap.c
index ca15a87..e83002dc 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -47,7 +47,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
for_each_sg(table->sgl, sg, table->nents, i) {
int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
- struct page *page = sg_page(sg);
+ struct page *page = pfn_to_page(PFN_DOWN(sg_phys(sg)));
BUG_ON(i >= npages);
for (j = 0; j < npages_this_entry; j++)
@@ -79,7 +79,6 @@ int ion_heap_map_user(struct ion_heap *heap, struct
ion_buffer *buffer,
int ret;
for_each_sg(table->sgl, sg, table->nents, i) {
- struct page *page = sg_page(sg);
unsigned long remainder = vma->vm_end - addr;
unsigned long len = sg->length;
@@ -87,18 +86,18 @@ int ion_heap_map_user(struct ion_heap *heap, struct
ion_buffer *buffer,
offset -= sg->length;
continue;
} else if (offset) {
- page += offset / PAGE_SIZE;
len = sg->length - offset;
- offset = 0;
}
len = min(len, remainder);
- ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
- vma->vm_page_prot);
+ ret = remap_pfn_range(vma, addr, PFN_DOWN(sg_phys(sg) + offset),
+ len, vma->vm_page_prot);
if (ret)
return ret;
addr += len;
if (addr >= vma->vm_end)
return 0;
+
+ offset = 0;
}
return 0;
}
[PATCH] ion: scatterlist offset not used for buffer map
ion's default user/kernel page mapping code don't honor the offset
option for scatterlists. It uses sg_page and expect the whole page to be
mapped, while the offset could dictate an offset within a large page.
sg_phys correctly accounts for the offset, so should be used instead.
Signed-off-by: John Einar Reitan
---
drivers/staging/android/ion/ion_heap.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/android/ion/ion_heap.c
b/drivers/staging/android/ion/ion_heap.c
index ca15a87..e83002dc 100644
--- a/drivers/staging/android/ion/ion_heap.c
+++ b/drivers/staging/android/ion/ion_heap.c
@@ -47,7 +47,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
for_each_sg(table->sgl, sg, table->nents, i) {
int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
- struct page *page = sg_page(sg);
+ struct page *page = pfn_to_page(PFN_DOWN(sg_phys(sg)));
BUG_ON(i >= npages);
for (j = 0; j < npages_this_entry; j++)
@@ -79,7 +79,6 @@ int ion_heap_map_user(struct ion_heap *heap, struct
ion_buffer *buffer,
int ret;
for_each_sg(table->sgl, sg, table->nents, i) {
- struct page *page = sg_page(sg);
unsigned long remainder = vma->vm_end - addr;
unsigned long len = sg->length;
@@ -87,18 +86,18 @@ int ion_heap_map_user(struct ion_heap *heap, struct
ion_buffer *buffer,
offset -= sg->length;
continue;
} else if (offset) {
- page += offset / PAGE_SIZE;
len = sg->length - offset;
- offset = 0;
}
len = min(len, remainder);
- ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
- vma->vm_page_prot);
+ ret = remap_pfn_range(vma, addr, PFN_DOWN(sg_phys(sg) + offset),
+ len, vma->vm_page_prot);
if (ret)
return ret;
addr += len;
if (addr >= vma->vm_end)
return 0;
+
+ offset = 0;
}
return 0;
}
--
2.7.3
