Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1460092 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFCv2][PATCH 2/5] arm: Implement ARCH_HAS_FORCE_CACHE Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-11 01:30 +0200
    Re: [RFCv2][PATCH 2/5] arm: Implement ARCH_HAS_FORCE_CACHE Laura Abbott <labbott@redhat.com> - 2016-08-16 22:50 +0200

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

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1464154

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web