Re: [RFCv2][PATCH 2/5] arm: Implement ARCH_HAS_FORCE_CACHE

[Laura Abbott <labbott@redhat.com>]

On 08/10/2016 04:22 PM, Russell King - ARM Linux wrote:
> On Mon, Aug 08, 2016 at 10:49:34AM -0700, Laura Abbott wrote:
>> +/*
>> + * Make an area consistent for devices.
>> + * Note: Drivers should NOT use this function directly, as it will break
>> + * platforms with CONFIG_DMABOUNCE.
>> + * Use the driver DMA support - see dma-mapping.h (dma_sync_*)
>> + */
>> +void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
>> +	size_t size, enum dma_data_direction dir)
>> +{
>> +	phys_addr_t paddr;
>> +
>> +	dma_cache_maint_page(page, off, size, dir, dmac_map_area);
>> +
>> +	paddr = page_to_phys(page) + off;
>> +	if (dir == DMA_FROM_DEVICE) {
>> +		outer_inv_range(paddr, paddr + size);
>> +	} else {
>> +		outer_clean_range(paddr, paddr + size);
>> +	}
>> +	/* FIXME: non-speculating: flush on bidirectional mappings? */
>> +}
>> +
>> +void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
>> +	size_t size, enum dma_data_direction dir)
>> +{
>> +	phys_addr_t paddr = page_to_phys(page) + off;
>> +
>> +	/* FIXME: non-speculating: not required */
>> +	/* in any case, don't bother invalidating if DMA to device */
>> +	if (dir != DMA_TO_DEVICE) {
>> +		outer_inv_range(paddr, paddr + size);
>> +
>> +		dma_cache_maint_page(page, off, size, dir, dmac_unmap_area);
>> +	}
>> +
>> +	/*
>> +	 * Mark the D-cache clean for these pages to avoid extra flushing.
>> +	 */
>> +	if (dir != DMA_TO_DEVICE && size >= PAGE_SIZE) {
>> +		unsigned long pfn;
>> +		size_t left = size;
>> +
>> +		pfn = page_to_pfn(page) + off / PAGE_SIZE;
>> +		off %= PAGE_SIZE;
>> +		if (off) {
>> +			pfn++;
>> +			left -= PAGE_SIZE - off;
>> +		}
>> +		while (left >= PAGE_SIZE) {
>> +			page = pfn_to_page(pfn++);
>> +			set_bit(PG_dcache_clean, &page->flags);
>> +			left -= PAGE_SIZE;
>> +		}
>> +	}
>> +}
>
> I _really_ don't want these exposed in any shape or form to driver
> code.  I've seen too many hacks out there where people have gone
> under the cover of the APIs they should be using, and headed straight
> for the low-level functionality - adding function prototypes to get
> at stuff they have no business doing.  Moving this here is just
> asking for it to be abused.
>
>> +
>> +void kernel_force_cache_clean(struct page *page, size_t size)
>> +{
>> +	__dma_page_cpu_to_dev(page, 0, size, DMA_BIDIRECTIONAL);
>> +}
>> +
>> +void kernel_force_cache_invalidate(struct page *page, size_t size)
>> +{
>> +	__dma_page_dev_to_cpu(page, 0, size, DMA_BIDIRECTIONAL);
>> +}
>
> Nothing in our implementation of these DMA operations guarantees
> that those mean "clean" and "invalidate".  The DMA operations are
> there so that CPUs can implement whatever they need at the map and
> unmap times - and I've been very careful not to specify which cache
> operations are involved.
>
> For example, on older CPUs, __dma_page_dev_to_cpu() is almost
> always a no-op.
>
> If you want something that does something specific, then we need
> something designed to do something specific.  Please don't re-use
> what you think will fit.
>

I see what you are saying. What I really wanted was to re-use some of
the code that dma_cache_maint_page was doing for highmem handling
but it looks like I picked the wrong layer to make common. I'll give
this some thought.

Thanks,
Laura