locks should be taken and released symmetrically. buffer_acquire() can potentially take a lock if the buffer is shared, so buffer_unlock() should come after sa_unlock(). Also below you acquire the buffer inside sa_lock() blocks. I know, that this is just a test, so probably it cannot race, right? Still it would be better to have that consistent to not even look like we can get an AB-BA deadlock

good catch! thanks! refined it.

AB-BA is still here: in this function buffer-acquisition is outside of sa_lock(), and in smart_amp_trigger() it's the other way round. Maybe you could just take the sa_lock() outside of the loop here?

yes, it is a potential risk

right, let's get it correct then. It should be ok to have sa_lock() outside of buffer locking since with Zephyr they're both taking mutexes and with XTOS - spinlocks

@RanderWang can you do this in prepare() instead of bind?

@ranj063 we can't do it in prepare for playback stream since the feedback buffer is shared by capture feedback stream, so it can be available only when the capture feedback stream is started.

I will submit a patch later that remove bind/unbind interface and use notifier to process bind/unbind event. please check #6751 (comment)

Can we revisit this in a subsequent PR (as there are others doing the same) and use k_mutex_ API directly (where we will define this for xtos as a spinlock).

sure, thanks

ok, since we are updating the PR due to other issues, let fix this one too. Please add the k_mutex() calls to xtos/include/rtos/spinlock.h (and map them to use spinlocks).

@lgirdwood are we sure that this is safe? XTOS also has a real thread context, IIRC it's used for EDF tasks. Are we sure that we don't have code in SOF, that maps to an EDF task in XTOS and uses a mutex for locking? Or would it be safe to take a spinlock and potentially go to sleep in such a task?

do we need to maintain XTOS ? or switch to zephyr completely ? if so we can do nothing for k_mutex for XTOS

@lgirdwood When I try to support k_mutex with spinlock, I find that the parameters of mutex and spinlock is different. How to save spinlock and its key ? I can't figure out a method

Spinlock;
struct k_spinlock lock;
k_spinlock_key_t key;

Mutex:
struct k_mutex lock;

@RanderWang Now I get the problem. Maybe define "struct k_mutex" so that there's both struct k_spinlock and k_spinlock_key_t as members of it when you build for XTOS. So then you can implement k_mutex_lock() with k_spin_lock() and store the key in XTOS version of "struct k_mutex". Would this work?

@RanderWang pls consider doing something like (not even build tested)

diff --git a/xtos/include/rtos/spinlock.h b/xtos/include/rtos/spinlock.h
index 410c66bb9..5a02406bb 100644
--- a/xtos/include/rtos/spinlock.h
+++ b/xtos/include/rtos/spinlock.h
@@ -160,4 +160,27 @@ k_spinlock_key_t _k_spin_lock_irq(struct k_spinlock *lock);
 void _k_spin_unlock_irq(struct k_spinlock *lock, k_spinlock_key_t key, int line);
 #define k_spin_unlock(lock, key) _k_spin_unlock_irq(lock, key, __LINE__)
 
+struct k_mutex {
+       struct k_spinlock lock;
+       k_spinlock_key_t key;
+};
+
+static inline int k_mutex_init(struct k_mutex *mutex)
+{
+       k_spinlock_init(mutex->lock);
+       return 0;
+}
+
+static int k_mutex_lock(struct k_mutex *mutex, k_timeout_t timeout)
+{
+       mutex->key = k_spin_lock(mutex->lock);
+       return 0;
+}
+
+static inline int k_mutex_unlock(struct k_mutex *mutex)
+{
+       k_spin_unlock(mutex->lock, mutex->key);
+       return 0;
+}
+
 #endif /* __XTOS_RTOS_SPINLOCK_H__ */

@kv2019i @lgirdwood thanks very much ! completely address my concern.

We're already doing this in the coherent API - storing the spinlock and the key in the same persistent structure. But that made me rethink that a bit - is that actually safe, because normally you have those keys as local variables - you store your returned spinlock status (CPU interrupt state) and then you restore it from the same variable, making sure nobody else overwrites that variable in the meantime. So having that variable as persistent and available to all might sound risky - you don't want to have another context overwrite that key and then restore a wrong context. But I think it should be safe in this context because another context will only overwrite the key after it has taken the spinlock, so the spinlock protects the key too. Just something to be aware of. Alternatively we could wrap k_mutex_lock() to return a status, but I don't think we want to do that, we want to use Zephyr APIs directly.

I think this is ok as a bugfix, but this kind of hints at need for reference counting type approach to be used. Otherwise this will get very complicated when we might have connections across the cores and just disabling the irq is not enough to make checks like the above atomic.

sure, I add it to my shopping list

@RanderWang why is this needed? ipc_comp_disconnect() will only be called when 2 connected components from separate pipelines are disconnected. You should disconnecte and unbind here.

consider different streams not different pipelines

A buffer is shared between playback stream and capture stream. I meant playback stream is still working, but capture stream is stopped, and one component in capture stream will be disconnected from a component in playback stream and the shared buffer will be free. if the buffer is using by playback stream, the disconnection will result to null pointer issue

@RanderWang this can never happen, right? During comp_disconnect, we have disabled interrupts. So it is not possible that the buffer is dsconnected from the sink dev but it hasn't been deleted from the list and freed.

in different stream case, not in the same stream.

I meant the buffer is not disconnected for the state check in comp_disconnect. Ignore it since I remove the logic