On Fri, 15 Oct 2021 18:22:58 +0200, Pierre-Louis Bossart wrote:
The FE stream locks are necessary only two points: at adding and deleting the BE in the link. We used dpcm_lock in other places, but those are superfluous or would make problem if converted to a FE stream lock.
I must be missing a fundamental concept here - possibly a set of concepts...
It is my understanding that the FE-BE connection can be updated dynamically without any relationship to the usual ALSA steps, e.g. as a result of a control being changed by a user.
So if you only protect the addition/removal, isn't there a case where the for_each_dpcm_be() loop would either miss a BE or point to an invalid one?
No, for sleepable context, pcm_mutex is *always* taken when adding/deleting a BE, and that's the main protection for the link. The BE stream lock is taken additionally over it at adding/deleting a BE, just for the code path via FE and BE trigger.
In other words, don't we need the *same* lock to be used a) before changing and b) walking through the list?
I also don't get what would happen if the dpcm_lock was converted to an FE stream lock. It works fine in my tests, so if there's limitation I didn't see it.
dpcm_lock was put in the places that could be recursively taken. So this caused some deadlock, I suppose.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
What happens if we show the state while a trigger happens? That's my main concern with using two separate locks (pcm_mutex and FE stream lock) to protect the same list, there are still windows of time where the list is not protected.
With the proper use of mutex, the list itself is protected. If we need to protect the concurrent access to each BE in the show method, an additional BE lock is needed in that part. But that's a subtle issue, as the link traversal itself is protected by the mutex.
If I look at your patch2, dpcm_be_disconnect() protects the list removal with the fe stream lock, but the show state is protected by both the pcm_mutex and the fe stream lock.
No, show_state() itself doesn't take any lock, but its caller dpcm_state_read_file() takes the pcm_mutex. That protects the list addition / deletion.
I have not been able to figure out when you need a) the pcm_mutex only b) the fe stream lock only c) both pcm_mutex and fe stream lock
The pcm_mutex is needed for every sleepable function that treat DPCM FE link, but the mutex is taken only at the upper level, i.e. the top-most caller like PCM ops FE itself or the DAPM calls.
That said, pcm_mutex is the top-most protection of BE links in FE. But, there is a code path where a mutex can't be used, and that's the FE and BE trigger. For protecting against this, the FE stream lock is taken only at the placing both adding and deleting a BE *in addition*. At those places, both pcm_mutex and FE stream lock are taken.
BE stream lock is taken in addition below the above mutex and FE locks.
Takashi