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

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

[Jerome Brunet]

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.

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

[PATCH 1/2] Revert "ASoC: hdmi-codec: re-introduce mutex locking"

[Jerome Brunet]

This reverts commit eb1ecadb7f67dde94ef0efd3ddaed5cb6c9a65ed.

This fixes the following warning reported by lockdep and a potential
issue with hibernation

====================================
WARNING: pulseaudio/1297 still has locks held!
5.3.0+ #1826 Not tainted
------------------------------------
1 lock held by pulseaudio/1297:
 #0: ee815308 (&hcp->lock){....}, at: hdmi_codec_startup+0x20/0x130

stack backtrace:
CPU: 0 PID: 1297 Comm: pulseaudio Not tainted 5.3.0+ #1826
Hardware name: Marvell Dove (Cubox)
[<c0017b4c>] (unwind_backtrace) from [<c0014d10>] (show_stack+0x10/0x14)
[<c0014d10>] (show_stack) from [<c00a2d18>] (futex_wait_queue_me+0x13c/0x19c)
[<c00a2d18>] (futex_wait_queue_me) from [<c00a3630>] (futex_wait+0x184/0x24c)
[<c00a3630>] (futex_wait) from [<c00a5e1c>] (do_futex+0x334/0x598)
[<c00a5e1c>] (do_futex) from [<c00a62e8>] (sys_futex_time32+0x118/0x180)
[<c00a62e8>] (sys_futex_time32) from [<c0009000>] (ret_fast_syscall+0x0/0x54)
Exception stack(0xebd31fa8 to 0xebd31ff0)
1fa0:                   00000000 ffffffff 000c8748 00000189 00000001 00000000
1fc0: 00000000 ffffffff 00000000 000000f0 00000000 00000000 00000000 00056200
1fe0: 000000f0 beac03a8 b6d6c835 b6d6f456

Fixes: eb1ecadb7f67 ("ASoC: hdmi-codec: re-introduce mutex locking")
Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/hdmi-codec.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index b5fd8f08726e..f8b5b960e597 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -274,7 +274,7 @@ struct hdmi_codec_priv {
 	uint8_t eld[MAX_ELD_BYTES];
 	struct snd_pcm_chmap *chmap_info;
 	unsigned int chmap_idx;
-	struct mutex lock;
+	unsigned long busy;
 	struct snd_soc_jack *jack;
 	unsigned int jack_status;
 };
@@ -390,8 +390,8 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 	int ret = 0;
 
-	ret = mutex_trylock(&hcp->lock);
-	if (!ret) {
+	ret = test_and_set_bit(0, &hcp->busy);
+	if (ret) {
 		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
 		return -EINVAL;
 	}
@@ -419,7 +419,7 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 
 err:
 	/* Release the exclusive lock on error */
-	mutex_unlock(&hcp->lock);
+	clear_bit(0, &hcp->busy);
 	return ret;
 }
 
@@ -431,7 +431,7 @@ static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
 	hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN;
 	hcp->hcd.ops->audio_shutdown(dai->dev->parent, hcp->hcd.data);
 
-	mutex_unlock(&hcp->lock);
+	clear_bit(0, &hcp->busy);
 }
 
 static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
@@ -811,8 +811,6 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hcp->hcd = *hcd;
-	mutex_init(&hcp->lock);
-
 	daidrv = devm_kcalloc(dev, dai_count, sizeof(*daidrv), GFP_KERNEL);
 	if (!daidrv)
 		return -ENOMEM;
-- 
2.21.0

Re: [PATCH 1/2] Revert "ASoC: hdmi-codec: re-introduce mutex locking"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Wed, Oct 23, 2019 at 06:12:02PM +0200, Jerome Brunet wrote:
> This reverts commit eb1ecadb7f67dde94ef0efd3ddaed5cb6c9a65ed.
> 
> This fixes the following warning reported by lockdep and a potential
> issue with hibernation

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] Revert "ASoC: hdmi-codec: re-introduce mutex locking"

[Russell King - ARM Linux admin]

On Wed, Oct 23, 2019 at 05:37:16PM +0100, Mark Brown wrote:
> On Wed, Oct 23, 2019 at 06:12:02PM +0200, Jerome Brunet wrote:
> > This reverts commit eb1ecadb7f67dde94ef0efd3ddaed5cb6c9a65ed.
> > 
> > This fixes the following warning reported by lockdep and a potential
> > issue with hibernation
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.

Hi Mark,

If you look at the git log for reverted commits, the vast majority
of them follow _this_ style.  From 5.3 back to the start of current
git history, there are 3665 commits with "Revert" in their subject
line, 3050 of those start with "Revert" with no subsystem prefix.

It seems that there are a small number of subsystems that want
something different, ASoC included.  That will be an ongoing problem,
people won't remember which want it when the majority don't.

Maybe the revert format should be standardised in some manner?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 1/2] Revert "ASoC: hdmi-codec: re-introduce mutex locking"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On Wed, Oct 23, 2019 at 07:26:18PM +0100, Russell King - ARM Linux admin wrote:

> If you look at the git log for reverted commits, the vast majority
> of them follow _this_ style.  From 5.3 back to the start of current
> git history, there are 3665 commits with "Revert" in their subject
> line, 3050 of those start with "Revert" with no subsystem prefix.

That's assuming that all reverts have Revert in their subject line of
course!

> It seems that there are a small number of subsystems that want
> something different, ASoC included.  That will be an ongoing problem,
> people won't remember which want it when the majority don't.

> Maybe the revert format should be standardised in some manner?

My general thought on this is that reverts are commits like any other
and that the documentation for how you're supposed to do commit messages
also applies to them, might be worth a note though as I do see people
not writing a commit log at all for them sometimes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Applied "ASoC: hdmi-codec: drop mutex locking again" to the asoc tree

[Mark Brown]

The patch

   ASoC: hdmi-codec: drop mutex locking again

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 901af18b6baade6a327e532427cbb233f4945f5d Mon Sep 17 00:00:00 2001
From: Jerome Brunet <jbrunet@baylibre.com>
Date: Wed, 23 Oct 2019 18:12:02 +0200
Subject: [PATCH] ASoC: hdmi-codec: drop mutex locking again

This reverts commit eb1ecadb7f67dde94ef0efd3ddaed5cb6c9a65ed.

This fixes the following warning reported by lockdep and a potential
issue with hibernation

====================================
WARNING: pulseaudio/1297 still has locks held!
5.3.0+ #1826 Not tainted
------------------------------------
1 lock held by pulseaudio/1297:
 #0: ee815308 (&hcp->lock){....}, at: hdmi_codec_startup+0x20/0x130

stack backtrace:
CPU: 0 PID: 1297 Comm: pulseaudio Not tainted 5.3.0+ #1826
Hardware name: Marvell Dove (Cubox)
[<c0017b4c>] (unwind_backtrace) from [<c0014d10>] (show_stack+0x10/0x14)
[<c0014d10>] (show_stack) from [<c00a2d18>] (futex_wait_queue_me+0x13c/0x19c)
[<c00a2d18>] (futex_wait_queue_me) from [<c00a3630>] (futex_wait+0x184/0x24c)
[<c00a3630>] (futex_wait) from [<c00a5e1c>] (do_futex+0x334/0x598)
[<c00a5e1c>] (do_futex) from [<c00a62e8>] (sys_futex_time32+0x118/0x180)
[<c00a62e8>] (sys_futex_time32) from [<c0009000>] (ret_fast_syscall+0x0/0x54)
Exception stack(0xebd31fa8 to 0xebd31ff0)
1fa0:                   00000000 ffffffff 000c8748 00000189 00000001 00000000
1fc0: 00000000 ffffffff 00000000 000000f0 00000000 00000000 00000000 00056200
1fe0: 000000f0 beac03a8 b6d6c835 b6d6f456

Fixes: eb1ecadb7f67 ("ASoC: hdmi-codec: re-introduce mutex locking")
Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
Link: https://lore.kernel.org/r/20191023161203.28955-2-jbrunet@baylibre.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/hdmi-codec.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index b5fd8f08726e..f8b5b960e597 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -274,7 +274,7 @@ struct hdmi_codec_priv {
 	uint8_t eld[MAX_ELD_BYTES];
 	struct snd_pcm_chmap *chmap_info;
 	unsigned int chmap_idx;
-	struct mutex lock;
+	unsigned long busy;
 	struct snd_soc_jack *jack;
 	unsigned int jack_status;
 };
@@ -390,8 +390,8 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 	int ret = 0;
 
-	ret = mutex_trylock(&hcp->lock);
-	if (!ret) {
+	ret = test_and_set_bit(0, &hcp->busy);
+	if (ret) {
 		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
 		return -EINVAL;
 	}
@@ -419,7 +419,7 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 
 err:
 	/* Release the exclusive lock on error */
-	mutex_unlock(&hcp->lock);
+	clear_bit(0, &hcp->busy);
 	return ret;
 }
 
@@ -431,7 +431,7 @@ static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
 	hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN;
 	hcp->hcd.ops->audio_shutdown(dai->dev->parent, hcp->hcd.data);
 
-	mutex_unlock(&hcp->lock);
+	clear_bit(0, &hcp->busy);
 }
 
 static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
@@ -811,8 +811,6 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hcp->hcd = *hcd;
-	mutex_init(&hcp->lock);
-
 	daidrv = devm_kcalloc(dev, dai_count, sizeof(*daidrv), GFP_KERNEL);
 	if (!daidrv)
 		return -ENOMEM;
-- 
2.20.1

[PATCH 2/2] ASoC: hdmi-codec: re-introduce mutex locking again

[Jerome Brunet]

The dai codec needs to ensure that on one dai is used at any time.
This is currently protected by bit atomic operation. With this change,
it done with a mutex instead.

Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/hdmi-codec.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index f8b5b960e597..56f6373d7927 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -274,7 +274,8 @@ struct hdmi_codec_priv {
 	uint8_t eld[MAX_ELD_BYTES];
 	struct snd_pcm_chmap *chmap_info;
 	unsigned int chmap_idx;
-	unsigned long busy;
+	struct mutex lock;
+	bool busy;
 	struct snd_soc_jack *jack;
 	unsigned int jack_status;
 };
@@ -390,12 +391,15 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 	int ret = 0;
 
-	ret = test_and_set_bit(0, &hcp->busy);
-	if (ret) {
+	mutex_lock(&hcp->lock);
+	if (hcp->busy) {
 		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
+		mutex_unlock(&hcp->lock);
 		return -EINVAL;
 	}
 
+	hcp->busy = true;
+
 	if (hcp->hcd.ops->audio_startup) {
 		ret = hcp->hcd.ops->audio_startup(dai->dev->parent, hcp->hcd.data);
 		if (ret)
@@ -415,11 +419,12 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 		/* Select chmap supported */
 		hdmi_codec_eld_chmap(hcp);
 	}
-	return 0;
 
 err:
-	/* Release the exclusive lock on error */
-	clear_bit(0, &hcp->busy);
+	if (ret)
+		hcp->busy = false;
+
+	mutex_unlock(&hcp->lock);
 	return ret;
 }
 
@@ -431,7 +436,9 @@ static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
 	hcp->chmap_idx = HDMI_CODEC_CHMAP_IDX_UNKNOWN;
 	hcp->hcd.ops->audio_shutdown(dai->dev->parent, hcp->hcd.data);
 
-	clear_bit(0, &hcp->busy);
+	mutex_lock(&hcp->lock);
+	hcp->busy = false;
+	mutex_unlock(&hcp->lock);
 }
 
 static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
@@ -811,6 +818,8 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hcp->hcd = *hcd;
+	mutex_init(&hcp->lock);
+
 	daidrv = devm_kcalloc(dev, dai_count, sizeof(*daidrv), GFP_KERNEL);
 	if (!daidrv)
 		return -ENOMEM;
-- 
2.21.0

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

[Takashi Iwai]

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
> 

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

[Jerome Brunet]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

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