[alsa-devel] [PATCH 0/2] ASoC: hdmi-codec: fix locking issue
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(-)
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;
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.
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?
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.
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;
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;
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
participants (4)
-
Jerome Brunet -
Mark Brown -
Russell King - ARM Linux admin -
Takashi Iwai