Re: [alsa-devel] [PATCH 0/2] ASoC: hdmi-codec: fix locking issue

From: Jerome Brunet <jbrunet@baylibre.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	alsa-devel@alsa-project.org,
	Russell King <rmk+kernel@armlinux.org.uk>,
	linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel] [PATCH 0/2] ASoC: hdmi-codec: fix locking issue
Date: Wed, 23 Oct 2019 19:53:31 +0200	[thread overview]
Message-ID: <1jv9sfcpr8.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <s5ha79rph1j.wl-tiwai@suse.de>

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
>> 