On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey brian.starkey@arm.com wrote:
On Sat, Oct 03, 2020 at 04:02:57AM +0000, John Stultz wrote:
@@ -215,8 +236,12 @@ static void *system_heap_do_vmap(struct system_heap_buffer *buffer) struct page **pages = vmalloc(sizeof(struct page *) * npages); struct page **tmp = pages; struct sg_page_iter piter;
pgprot_t pgprot = PAGE_KERNEL; void *vaddr;if (buffer->uncached)pgprot = pgprot_writecombine(PAGE_KERNEL);I think this should go after the 'if (!pages)' check. Best to get the allocation failure check as close to the allocation as possible IMO.
Sounds good. Changed in my tree.
@@ -393,6 +424,16 @@ static int system_heap_allocate(struct dma_heap *heap, /* just return, as put will call release and that will free */ return ret; }
/** For uncached buffers, we need to initially flush cpu cache, since* the __GFP_ZERO on the allocation means the zeroing was done by the* cpu and thus it is likely cached. Map (and implicitly flush) it out* now so we don't get corruption later on.*/if (buffer->uncached)dma_map_sgtable(dma_heap_get_dev(heap), table, DMA_BIDIRECTIONAL, 0);Do we have to keep this mapping around for the entire lifetime of the buffer?
Yea, I guess we can just map and unmap it right there. It will look a little absurd, but that sort of aligns with your next point.
Also, this problem (and solution) keeps lingering around. It really feels like there should be a better way to solve "clean the linear mapping all the way to DRAM", but I don't know what that should be.
Yea, something better here would be nice...
@@ -426,6 +487,16 @@ static int system_heap_create(void) if (IS_ERR(sys_heap)) return PTR_ERR(sys_heap);
exp_info.name = "system-uncached";exp_info.ops = &system_uncached_heap_ops;exp_info.priv = NULL;sys_uncached_heap = dma_heap_add(&exp_info);if (IS_ERR(sys_uncached_heap))return PTR_ERR(sys_heap);In principle, there's a race here between the heap getting registered to sysfs and the dma_mask getting updated.
I don't think it would cause a problem in practice, but with the API as it is, there's no way to avoid it - so I wonder if the dma_heap_get_dev() accessor isn't the right API design.
Hrm. I guess to address your concern we would need split dma_heap_add() into something like: dma_heap_create() dma_heap_add()
Which breaks the creation of the heap with the registering it to the subsystem, so some attributes can be tweaked inbetween?
I'll see about taking a stab at this, but I'll probably submit my updated series sooner with this un-addressed so I can get some further review.
thanks -john