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
