[alsa-devel] [PATCH v1 0/2] fix race condition in rsnd_ssi_pointer_update
From: Jiada Wang jiada_wang@mentor.com
This patch set aims to fix the race condition in rsnd_ssi_pointer_update, between set of byte_pos and wrap it around when new buffer starts.
Jiada Wang (2): ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update ASoC: rsnd: ssi: remove unnesessary period_pos
sound/soc/sh/rcar/ssi.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
From: Jiada Wang jiada_wang@mentor.com
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Reviewed-by: Takashi Sakamoto takashi.sakamoto@miraclelinux.com --- sound/soc/sh/rcar/ssi.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index fece1e5f..cbf3bf3 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -446,25 +446,29 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod, int byte) { struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); + bool ret = false; + int byte_pos;
- ssi->byte_pos += byte; + byte_pos = ssi->byte_pos + byte;
- if (ssi->byte_pos >= ssi->next_period_byte) { + if (byte_pos >= ssi->next_period_byte) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->period_pos++; ssi->next_period_byte += ssi->byte_per_period;
if (ssi->period_pos >= runtime->periods) { - ssi->byte_pos = 0; + byte_pos = 0; ssi->period_pos = 0; ssi->next_period_byte = ssi->byte_per_period; }
- return true; + ret = true; }
- return false; + WRITE_ONCE(ssi->byte_pos, byte_pos); + + return ret; }
/* @@ -838,7 +842,7 @@ static int rsnd_ssi_pointer(struct rsnd_mod *mod, struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
- *pointer = bytes_to_frames(runtime, ssi->byte_pos); + *pointer = bytes_to_frames(runtime, READ_ONCE(ssi->byte_pos));
return 0; }
Hi Jiada
Thank you for your patch
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Reviewed-by: Takashi Sakamoto takashi.sakamoto@miraclelinux.com
You using playback with PIO mode ? Because this function is no longer used on DMA mode
Best regards --- Kuninori Morimoto
Hello Morimoto-san
On 12/07/2017 01:45 AM, Kuninori Morimoto wrote:
Hi Jiada
Thank you for your patch
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wangjiada_wang@mentor.com Reviewed-by: Takashi Sakamototakashi.sakamoto@miraclelinux.com
You using playback with PIO mode ? Because this function is no longer used on DMA mode
No, we are using rcar sound in DMA mode, our original patch resolves the issue in core.c for both PIO & DMA mode.
but with your commit a97a06c ("ASoC: rsnd: cleanup pointer related code"), DMA mode no longer has the race condition issue, so I ported our fix patch to only address the issue in PIO mode
Thanks, Jiada
Best regards
Kuninori Morimoto
Hi Jiada
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wangjiada_wang@mentor.com Reviewed-by: Takashi Sakamototakashi.sakamoto@miraclelinux.com
You using playback with PIO mode ? Because this function is no longer used on DMA mode
No, we are using rcar sound in DMA mode, our original patch resolves the issue in core.c for both PIO & DMA mode.
but with your commit a97a06c ("ASoC: rsnd: cleanup pointer related code"), DMA mode no longer has the race condition issue, so I ported our fix patch to only address the issue in PIO mode
Thanks. Nice to know.
Best regards --- Kuninori Morimoto
The patch
ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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 33f801366bdf3f8b67dfe325b84f4051a090d01e Mon Sep 17 00:00:00 2001
From: Jiada Wang jiada_wang@mentor.com Date: Thu, 7 Dec 2017 22:15:38 -0800 Subject: [PATCH] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Reviewed-by: Takashi Sakamoto takashi.sakamoto@miraclelinux.com Acked-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/ssi.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index fece1e5f582f..cbf3bf312d23 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -446,25 +446,29 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod, int byte) { struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); + bool ret = false; + int byte_pos;
- ssi->byte_pos += byte; + byte_pos = ssi->byte_pos + byte;
- if (ssi->byte_pos >= ssi->next_period_byte) { + if (byte_pos >= ssi->next_period_byte) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->period_pos++; ssi->next_period_byte += ssi->byte_per_period;
if (ssi->period_pos >= runtime->periods) { - ssi->byte_pos = 0; + byte_pos = 0; ssi->period_pos = 0; ssi->next_period_byte = ssi->byte_per_period; }
- return true; + ret = true; }
- return false; + WRITE_ONCE(ssi->byte_pos, byte_pos); + + return ret; }
/* @@ -838,7 +842,7 @@ static int rsnd_ssi_pointer(struct rsnd_mod *mod, struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
- *pointer = bytes_to_frames(runtime, ssi->byte_pos); + *pointer = bytes_to_frames(runtime, READ_ONCE(ssi->byte_pos));
return 0; }
Hi Mark,
On Dec 9 2017 03:52, Mark Brown wrote:
The patch
ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_updatehas been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
The applied patches are in v1 patchset, but we have v2 patchset already: http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.ht...
Would you please replace the applied patches with the renewed ones? There're slight differences between them.
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 33f801366bdf3f8b67dfe325b84f4051a090d01e Mon Sep 17 00:00:00 2001
From: Jiada Wang jiada_wang@mentor.com Date: Thu, 7 Dec 2017 22:15:38 -0800 Subject: [PATCH] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
Currently there is race condition between set of byte_pos and wrap it around when new buffer starts. If .pointer is called in-between it will result in inconsistent pointer position be returned from .pointer callback.
This patch increments buffer pointer atomically to avoid this issue.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Reviewed-by: Takashi Sakamoto takashi.sakamoto@miraclelinux.com Acked-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org
sound/soc/sh/rcar/ssi.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index fece1e5f582f..cbf3bf312d23 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -446,25 +446,29 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod, int byte) { struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
- bool ret = false;
- int byte_pos;
- ssi->byte_pos += byte;
- byte_pos = ssi->byte_pos + byte;
- if (ssi->byte_pos >= ssi->next_period_byte) {
if (byte_pos >= ssi->next_period_byte) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->period_pos++; ssi->next_period_byte += ssi->byte_per_period;
if (ssi->period_pos >= runtime->periods) {
ssi->byte_pos = 0;
byte_pos = 0; ssi->period_pos = 0; ssi->next_period_byte = ssi->byte_per_period;
return true;
ret = true;
- return false;
WRITE_ONCE(ssi->byte_pos, byte_pos);
return ret; }
/*
@@ -838,7 +842,7 @@ static int rsnd_ssi_pointer(struct rsnd_mod *mod, struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
- *pointer = bytes_to_frames(runtime, ssi->byte_pos);
*pointer = bytes_to_frames(runtime, READ_ONCE(ssi->byte_pos));
return 0; }
Thanks
Takashi Sakamoto
On Sat, Dec 09, 2017 at 02:22:50PM +0900, Takashi Sakamoto wrote:
On Dec 9 2017 03:52, Mark Brown wrote:
The applied patches are in v1 patchset, but we have v2 patchset already: http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.ht...
Would you please replace the applied patches with the renewed ones? There're slight differences between them.
As far as I can tell what's applied is v2?
Hi Mark
Only the first patch in https://www.spinics.net/lists/alsa-devel/msg71021.html was applied, but the second patch "ASoC: rsnd: ssi: remove unnesessary period_pos" was missed
Could you please have a look
Thanks, Jiada
On 12/11/2017 03:38 AM, Mark Brown wrote:
On Sat, Dec 09, 2017 at 02:22:50PM +0900, Takashi Sakamoto wrote:
On Dec 9 2017 03:52, Mark Brown wrote: The applied patches are in v1 patchset, but we have v2 patchset already: http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.ht... Would you please replace the applied patches with the renewed ones? There're slight differences between them.
As far as I can tell what's applied is v2?
Hi Jiada,
On Jan 9 2018 14:42, Jiada Wang wrote:
Only the first patch in https://www.spinics.net/lists/alsa-devel/msg71021.html was applied, but the second patch "ASoC: rsnd: ssi: remove unnesessary period_pos" was missed
Could you please have a look
I can see all of them were applied to Mark's for-next branch[1].
- 33f801366bdf ('ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update')[2] - 2e2d53da81af ('ASoC: rsnd: ssi: remove unnesessary period_pos')[3]
No worries to us.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for... [2] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sou... [3] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sou...
Regards
Takashi Sakamoto
Hi Sakamoto-san
On 01/08/2018 10:53 PM, Takashi Sakamoto wrote:
Hi Jiada,
On Jan 9 2018 14:42, Jiada Wang wrote:
Only the first patch in https://www.spinics.net/lists/alsa-devel/msg71021.html was applied, but the second patch "ASoC: rsnd: ssi: remove unnesessary period_pos" was missed
Could you please have a look
I can see all of them were applied to Mark's for-next branch[1].
- 33f801366bdf ('ASoC: rsnd: ssi: fix race condition in
rsnd_ssi_pointer_update')[2]
- 2e2d53da81af ('ASoC: rsnd: ssi: remove unnesessary period_pos')[3]
No worries to us.
Thanks,
Jiada
[1] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for... [2] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sou... [3] https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sou...
Regards
Takashi Sakamoto
From: Jiada Wang jiada_wang@mentor.com
period_pos can always be calculated by byte_pos and byte_per_period, there is no reason to maintain this variable in rsnd_dai_stream. Further more, if the passed 'byte' amount to rsnd_ssi_pointer_update() is more than byte_per_period. the calculation of next_period_byte isn't correct.
This patch removes period_pos from rsnd_ssi and calculates next_period_byte with consideration of actual byte_pos value.
Signed-off-by: Jiada Wang jiada_wang@mentor.com --- sound/soc/sh/rcar/ssi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index cbf3bf3..f212024 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -80,7 +80,6 @@ struct rsnd_ssi { unsigned int usrcnt;
int byte_pos; - int period_pos; int byte_per_period; int next_period_byte; }; @@ -421,7 +420,6 @@ static void rsnd_ssi_pointer_init(struct rsnd_mod *mod, struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->byte_pos = 0; - ssi->period_pos = 0; ssi->byte_per_period = runtime->period_size * runtime->channels * samples_to_bytes(runtime, 1); @@ -453,13 +451,12 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod,
if (byte_pos >= ssi->next_period_byte) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io); + int period_pos = byte_pos / ssi->byte_per_period;
- ssi->period_pos++; - ssi->next_period_byte += ssi->byte_per_period; + ssi->next_period_byte = (period_pos + 1) * ssi->byte_per_period;
- if (ssi->period_pos >= runtime->periods) { + if (period_pos >= runtime->periods) { byte_pos = 0; - ssi->period_pos = 0; ssi->next_period_byte = ssi->byte_per_period; }
Hi Jiada
Further more, if the passed 'byte' amount to rsnd_ssi_pointer_update() is more than byte_per_period. the calculation of next_period_byte isn't correct.
Is it really happen ??
Basically, I have no objection about this patch, but this explanation is very strange for me...
Best regards --- Kuninori Morimoto
Hi Morimoto-san
On 12/07/2017 01:58 AM, Kuninori Morimoto wrote:
Hi Jiada
Further more, if the passed 'byte' amount to rsnd_ssi_pointer_update() is more than byte_per_period. the calculation of next_period_byte isn't correct.
Is it really happen ??
Basically, I have no objection about this patch, but this explanation is very strange for me...
No, I didn't see the issue, but the implementation of rsnd_ssi_pointer_update(), behaves like it knows all caller will always pass 'byte' no larger than byte_per_period, without any check internally.
I am ok to remove this explanation from commit message, what do you think?
Thanks, Jiada
Best regards
Kuninori Morimoto
Hi Jiada
Thank you for your feedback
Further more, if the passed 'byte' amount to rsnd_ssi_pointer_update() is more than byte_per_period. the calculation of next_period_byte isn't correct.
Is it really happen ??
Basically, I have no objection about this patch, but this explanation is very strange for me...
No, I didn't see the issue, but the implementation of rsnd_ssi_pointer_update(), behaves like it knows all caller will always pass 'byte' no larger than byte_per_period, without any check internally.
I am ok to remove this explanation from commit message, what do you think?
This function is used from PIO mode only now, and "byte" is sizeof(u32) (Its size was "byte_per_period" when DMA mode used). This "Further more" case never happen. Removing from commit message is better for reader, IMO.
Best regards --- Kuninori Morimoto
The patch
ASoC: rsnd: ssi: remove unnesessary period_pos
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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 2e2d53da81af6b2222c6b4e025a5d01b37b4449b Mon Sep 17 00:00:00 2001
From: Jiada Wang jiada_wang@mentor.com Date: Thu, 7 Dec 2017 22:15:39 -0800 Subject: [PATCH] ASoC: rsnd: ssi: remove unnesessary period_pos
period_pos can always be calculated by byte_pos and byte_per_period, there is no reason to maintain this variable in rsnd_dai_stream.
This patch removes period_pos from rsnd_ssi and calculates next_period_byte with consideration of actual byte_pos value.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Acked-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/ssi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index cbf3bf312d23..f21202429000 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -80,7 +80,6 @@ struct rsnd_ssi { unsigned int usrcnt;
int byte_pos; - int period_pos; int byte_per_period; int next_period_byte; }; @@ -421,7 +420,6 @@ static void rsnd_ssi_pointer_init(struct rsnd_mod *mod, struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
ssi->byte_pos = 0; - ssi->period_pos = 0; ssi->byte_per_period = runtime->period_size * runtime->channels * samples_to_bytes(runtime, 1); @@ -453,13 +451,12 @@ static bool rsnd_ssi_pointer_update(struct rsnd_mod *mod,
if (byte_pos >= ssi->next_period_byte) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io); + int period_pos = byte_pos / ssi->byte_per_period;
- ssi->period_pos++; - ssi->next_period_byte += ssi->byte_per_period; + ssi->next_period_byte = (period_pos + 1) * ssi->byte_per_period;
- if (ssi->period_pos >= runtime->periods) { + if (period_pos >= runtime->periods) { byte_pos = 0; - ssi->period_pos = 0; ssi->next_period_byte = ssi->byte_per_period; }
participants (5)
-
Jiada Wang -
jiada_wang＠mentor.com
-
Kuninori Morimoto -
Mark Brown -
Takashi Sakamoto