On Thu, Oct 8, 2020 at 4:51 AM Brian Starkey <brian.starkey@xxxxxxx> 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
