| Message ID | 20191023161203.28955-1-jbrunet@baylibre.com |
|---|---|
| Headers | show |
| Series | ASoC: hdmi-codec: fix locking issue | expand |
On Wed, 23 Oct 2019 18:12:01 +0200, Jerome Brunet wrote: > > This patchset fixes the locking issue reported by Russell. > > As explained a mutex was used as flag and held while returning to > userspace. > > Patch 2 is entirely optional and switches from bit atomic operation > to mutex again. I tend to prefer bit atomic operation in this > particular case but either way should be fine. I fail to see why the mutex is needed there. Could you elaborate about the background? IIUC, the protection with the atomic bitmap should guarantee the exclusive access. The mutex would allow the possible concurrent calls of multiple startup of a single instance, but is this the thing to be solved? thanks, Takashi > > Jerome Brunet (2): > Revert "ASoC: hdmi-codec: re-introduce mutex locking" > ASoC: hdmi-codec: re-introduce mutex locking again > > sound/soc/codecs/hdmi-codec.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > -- > 2.21.0 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Wed 23 Oct 2019 at 18:23, Takashi Iwai <tiwai@suse.de> wrote: > On Wed, 23 Oct 2019 18:12:01 +0200, > Jerome Brunet wrote: >> >> This patchset fixes the locking issue reported by Russell. >> >> As explained a mutex was used as flag and held while returning to >> userspace. >> >> Patch 2 is entirely optional and switches from bit atomic operation >> to mutex again. I tend to prefer bit atomic operation in this >> particular case but either way should be fine. > > I fail to see why the mutex is needed there. Could you elaborate > about the background? You are right, It is not required. Just a bit of history: A while ago the hdmi-codec was keeping track of the substream pointer. It was not used for anything useful, other than knowing if device was busy or not, and it was causing problem with codec-to-codec links I removed the saved substream pointer and replaced with a simple bit to track the busy state. Protecting a single bit with a mutex seemed a bit overkill to me, so I thought it was a good idea to replace the whole thing with atomic bit ops. [0] Mark told me he preferred the mutex method as it simpler to understand. As long as as it works it's fine for me :) I proposed something that uses the mutex as a busy flag but it turned out to be a bad idea. With the revert, we are back to the bit ops. Even if it works, Mark's original comment on the bit ops still stands I think. This is why I'm proposing patch 2 but I don't really mind if it is applied or not. [0] https://lkml.kernel.org/r/20190506095815.24578-3-jbrunet@baylibre.com > > IIUC, the protection with the atomic bitmap should guarantee the > exclusive access. The mutex would allow the possible concurrent calls > of multiple startup of a single instance, but is this the thing to be > solved? > > > thanks, > > Takashi > >> >> Jerome Brunet (2): >> Revert "ASoC: hdmi-codec: re-introduce mutex locking" >> ASoC: hdmi-codec: re-introduce mutex locking again >> >> sound/soc/codecs/hdmi-codec.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> -- >> 2.21.0 >> >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >>
On Wed, Oct 23, 2019 at 07:53:31PM +0200, Jerome Brunet wrote: > With the revert, we are back to the bit ops. Even if it works, Mark's > original comment on the bit ops still stands I think. This is why I'm > proposing patch 2 but I don't really mind if it is applied or not. Yeah, it's not *required* but the atomic operations have lots of spiky edges so a simpler locking construct would have less chance of running into trouble later when someone's updating the code.
On Thu, 24 Oct 2019 13:34:44 +0200, Mark Brown wrote: > > On Wed, Oct 23, 2019 at 07:53:31PM +0200, Jerome Brunet wrote: > > > With the revert, we are back to the bit ops. Even if it works, Mark's > > original comment on the bit ops still stands I think. This is why I'm > > proposing patch 2 but I don't really mind if it is applied or not. > > Yeah, it's not *required* but the atomic operations have lots of spiky > edges so a simpler locking construct would have less chance of running > into trouble later when someone's updating the code. If that's the reason, it should be mentioned specifically in the commit. That is, it's not about functionality or efficiency but just about the code readability. thanks, Takashi