coherent: tighten up the API

This patch adds documentation to the coherent API header and cleans
up several potentially harmful operations. Specifically, it isn't a
good idea to modify memory via uncached addresses and then write back
caches, because the latter can overwrite uncached writes. It works
because there is no data in cache at that point, which also makes
that call redundant. Similarly, we are sure not to have any data in
caches when obtaining access to the memory, because otherwise our
spinlock-acquisition operation could be overwritten by a speculative
write back.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

src/include/sof/coherent.h

@@ -20,11 +20,37 @@
 #define __coherent __attribute__((packed, aligned(DCACHE_LINE_SIZE)))
 
 /*
+ * The coherent API allows optimized access to memory by multiple cores, using
+ * cache, taking care about coherence. The intended use is to let cores acquire
+ * ownership of such shared objects, use them, and then release them, possibly
+ * to be re-acquired by other cores. Such shared objects must only be accessed
+ * via this API. It's designed to be primarily used with dynamically allocated
+ * objects because of their well-defined life span. It can also be used with
+ * objects from .data or .bss sections but greater care must be takenwith them
+ * to strictly follow the API flow.
+ *
+ * The API assumes, that in the beginning no core has cache lines associated
+ * with the memory area, used with it. That is true for dynamically allocated
+ * memory, because when such memory is freed, its cache is invalidated - as long
+ * as that memory was never accessed by other cores, except by using this API.
+ * The first call must be coherent_init(), which initializes the header. If the
+ * object will be used by multiple cores, next coherent_shared() must be called.
+ * After that to use that memory, coherent_acquire_irq() or coherent_acquire()
+ * must be called, which acquires ownership of the object and returns a cached
+ * address of the memory. After that the user can perform cached access to the
+ * memory. To release the memory, one of coherent_release_irq() or
+ * coherent_release() must be called. The only time when the memory is accessed
+ * using cache is between those two calls, so only when releasing the memory we
+ * have to write back and invalidate caches to make sure, that next time we
+ * acquire this memory, our uncached header access will not be overwritten! When
+ * memory is not needed any more, typically before freeing the memory,
+ * coherent_free() should be called.
+ *
  * This structure needs to be embedded at the start of any container to ensure
  * container object cache alignment and to minimise non cache access when
  * acquiring ownership.
  *
- * This structure should not be accessed outside of these APIs.
+ * This structure must not be accessed outside of these APIs.
  * The shared flag is only set at coherent init and thereafter it's RO.
  */
 struct coherent {
@@ -58,12 +84,8 @@ struct coherent {
 #define CHECK_COHERENT_IRQ(_c)
 #endif
 
-/*
- * Incoherent devices require manual cache invalidation and writeback as
- * well as locking to manage shared access.
- */
-
 #if CONFIG_INCOHERENT
+/* When coherent_acquire() is called, we are sure not to have cache for this memory */
 __must_check static inline struct coherent *coherent_acquire(struct coherent *c,
 							     const size_t size)
 {
@@ -158,8 +180,8 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si
 		object->member.shared = false;				\
 		object->member.core = cpu_get_id();				\
 		list_init(&object->member.list);				\
-		/* wtb and inv local data to coherent object */			\
-		dcache_writeback_invalidate_region(uncache_to_cache(object), sizeof(*object)); \
+		/* inv local data to coherent object */				\
+		dcache_invalidate_region(uncache_to_cache(object), sizeof(*object)); \
 	} while (0)
 
 #define coherent_free(object, member)			\
@@ -177,7 +199,7 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si
 		ADDR_IS_COHERENT(object);					\
 		spin_lock(&(object)->member.lock);				\
 		(object)->member.shared = true;					\
-		dcache_writeback_invalidate_region(object, sizeof(*object));	\
+		dcache_invalidate_region(object, sizeof(*object));		\
 		spin_unlock(&(object)->member.lock); \
 	} while (0)
 
@@ -188,7 +210,7 @@ static inline struct coherent *coherent_release_irq(struct coherent *c, const si
 		ADDR_IS_COHERENT(object);					\
 		spin_lock_irq(&(object)->member.lock, &(object)->member.flags);	\
 		(object)->member.shared = true;					\
-		dcache_writeback_invalidate_region(object, sizeof(*object));	\
+		dcache_invalidate_region(object, sizeof(*object));		\
 		spin_unlock_irq(&(object)->member.lock, &(object)->member.flags); \
 	} while (0)
 #else