All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] fix race condition in rsnd_ssi_pointer_update
@ 2017-12-07  8:22 ` jiada_wang
  0 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: alsa-devel, linux-kernel, Jiada Wang

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

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v1 0/2] fix race condition in rsnd_ssi_pointer_update
@ 2017-12-07  8:22 ` jiada_wang
  0 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: Jiada Wang, alsa-devel, linux-kernel

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

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
  2017-12-07  8:22 ` jiada_wang
@ 2017-12-07  8:22   ` jiada_wang
  -1 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: alsa-devel, linux-kernel, Jiada Wang

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;
 }
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
@ 2017-12-07  8:22   ` jiada_wang
  0 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: alsa-devel, linux-kernel, Jiada Wang

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;
 }
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
  2017-12-07  8:22 ` jiada_wang
@ 2017-12-07  8:22   ` jiada_wang
  -1 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: alsa-devel, linux-kernel, Jiada Wang

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;
 		}
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
@ 2017-12-07  8:22   ` jiada_wang
  0 siblings, 0 replies; 31+ messages in thread
From: jiada_wang @ 2017-12-07  8:22 UTC (permalink / raw)
  To: lgirdwood, broonie, perex, tiwai, kuninori.morimoto.gx
  Cc: alsa-devel, linux-kernel, Jiada Wang

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;
 		}
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
  2017-12-07  8:22   ` jiada_wang
@ 2017-12-07  9:45     ` Kuninori Morimoto
  -1 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-07  9:45 UTC (permalink / raw)
  To: jiada_wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
@ 2017-12-07  9:45     ` Kuninori Morimoto
  0 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-07  9:45 UTC (permalink / raw)
  To: jiada_wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
  2017-12-07  8:22   ` jiada_wang
@ 2017-12-07  9:58     ` Kuninori Morimoto
  -1 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-07  9:58 UTC (permalink / raw)
  To: jiada_wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
@ 2017-12-07  9:58     ` Kuninori Morimoto
  0 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-07  9:58 UTC (permalink / raw)
  To: jiada_wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
  2017-12-07  9:45     ` Kuninori Morimoto
@ 2017-12-08  5:35       ` Jiada Wang
  -1 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2017-12-08  5:35 UTC (permalink / raw)
  To: Kuninori Morimoto
  Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
@ 2017-12-08  5:35       ` Jiada Wang
  0 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2017-12-08  5:35 UTC (permalink / raw)
  To: Kuninori Morimoto
  Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
  2017-12-07  9:58     ` Kuninori Morimoto
@ 2017-12-08  5:43       ` Jiada Wang
  -1 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2017-12-08  5:43 UTC (permalink / raw)
  To: Kuninori Morimoto
  Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
@ 2017-12-08  5:43       ` Jiada Wang
  0 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2017-12-08  5:43 UTC (permalink / raw)
  To: Kuninori Morimoto; +Cc: alsa-devel, lgirdwood, linux-kernel, tiwai, broonie

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
  2017-12-08  5:35       ` Jiada Wang
@ 2017-12-08  6:00         ` Kuninori Morimoto
  -1 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-08  6:00 UTC (permalink / raw)
  To: Jiada Wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v1 1/2] ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update
@ 2017-12-08  6:00         ` Kuninori Morimoto
  0 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-08  6:00 UTC (permalink / raw)
  To: Jiada Wang; +Cc: alsa-devel, lgirdwood, linux-kernel, tiwai, broonie


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
  2017-12-08  5:43       ` Jiada Wang
@ 2017-12-08  6:07         ` Kuninori Morimoto
  -1 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-08  6:07 UTC (permalink / raw)
  To: Jiada Wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos
@ 2017-12-08  6:07         ` Kuninori Morimoto
  0 siblings, 0 replies; 31+ messages in thread
From: Kuninori Morimoto @ 2017-12-08  6:07 UTC (permalink / raw)
  To: Jiada Wang; +Cc: lgirdwood, broonie, perex, tiwai, alsa-devel, linux-kernel


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2017-12-07  8:22   ` jiada_wang
@ 2017-12-08 18:52     ` Mark Brown
  -1 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-08 18:52 UTC (permalink / raw)
  To: Jiada Wang
  Cc: Kuninori Morimoto, Mark Brown, lgirdwood, broonie, perex, tiwai,
	kuninori.morimoto.gx, alsa-devel, linux-kernel, alsa-devel

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;
 }
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
@ 2017-12-08 18:52     ` Mark Brown
  0 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-08 18:52 UTC (permalink / raw)
  To: Jiada Wang
  Cc: alsa-devel, kuninori.morimoto.gx, linux-kernel, tiwai, lgirdwood,
	broonie

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;
 }
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Applied "ASoC: rsnd: ssi: remove unnesessary period_pos" to the asoc tree
  2017-12-07  8:22   ` jiada_wang
@ 2017-12-08 18:55     ` Mark Brown
  -1 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-08 18:55 UTC (permalink / raw)
  To: Jiada Wang
  Cc: Kuninori Morimoto, Mark Brown, lgirdwood, broonie, perex, tiwai,
	kuninori.morimoto.gx, alsa-devel, linux-kernel, alsa-devel

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;
 		}
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Applied "ASoC: rsnd: ssi: remove unnesessary period_pos" to the asoc tree
@ 2017-12-08 18:55     ` Mark Brown
  0 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-08 18:55 UTC (permalink / raw)
  To: Jiada Wang
  Cc: alsa-devel, kuninori.morimoto.gx, linux-kernel, tiwai, lgirdwood,
	broonie

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;
 		}
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2017-12-08 18:52     ` Mark Brown
@ 2017-12-09  5:22       ` Takashi Sakamoto
  -1 siblings, 0 replies; 31+ messages in thread
From: Takashi Sakamoto @ 2017-12-09  5:22 UTC (permalink / raw)
  To: Mark Brown, Jiada Wang
  Cc: alsa-devel, kuninori.morimoto.gx, linux-kernel, tiwai, lgirdwood

Hi Mark,

On Dec 9 2017 03:52, Mark Brown wrote:
> 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

The applied patches are in v1 patchset, but we have v2 patchset already:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.html

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
@ 2017-12-09  5:22       ` Takashi Sakamoto
  0 siblings, 0 replies; 31+ messages in thread
From: Takashi Sakamoto @ 2017-12-09  5:22 UTC (permalink / raw)
  To: Mark Brown, Jiada Wang
  Cc: lgirdwood, alsa-devel, linux-kernel, kuninori.morimoto.gx, tiwai

Hi Mark,

On Dec 9 2017 03:52, Mark Brown wrote:
> 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

The applied patches are in v1 patchset, but we have v2 patchset already:
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-December/128970.html

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2017-12-09  5:22       ` Takashi Sakamoto
@ 2017-12-11 11:38         ` Mark Brown
  -1 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-11 11:38 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: Jiada Wang, alsa-devel, kuninori.morimoto.gx, linux-kernel,
	tiwai, lgirdwood

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

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

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

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
@ 2017-12-11 11:38         ` Mark Brown
  0 siblings, 0 replies; 31+ messages in thread
From: Mark Brown @ 2017-12-11 11:38 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: alsa-devel, kuninori.morimoto.gx, Jiada Wang, tiwai, lgirdwood,
	linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 423 bytes --]

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

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

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2017-12-11 11:38         ` Mark Brown
@ 2018-01-09  5:42           ` Jiada Wang
  -1 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2018-01-09  5:42 UTC (permalink / raw)
  To: Mark Brown
  Cc: Takashi Sakamoto, alsa-devel, kuninori.morimoto.gx, linux-kernel,
	tiwai, lgirdwood

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.html
>> 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?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
@ 2018-01-09  5:42           ` Jiada Wang
  0 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2018-01-09  5:42 UTC (permalink / raw)
  To: Mark Brown
  Cc: Takashi Sakamoto, alsa-devel, kuninori.morimoto.gx, linux-kernel,
	tiwai, lgirdwood

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.html
>> 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?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2018-01-09  5:42           ` Jiada Wang
  (?)
@ 2018-01-09  6:53           ` Takashi Sakamoto
  2018-01-09  8:59               ` Jiada Wang
  -1 siblings, 1 reply; 31+ messages in thread
From: Takashi Sakamoto @ 2018-01-09  6:53 UTC (permalink / raw)
  To: Jiada Wang, Mark Brown
  Cc: alsa-devel, kuninori.morimoto.gx, linux-kernel, tiwai, lgirdwood

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-next
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=33f801366bdf3f8b67dfe325b84f4051a090d01e
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=2e2d53da81af6b2222c6b4e025a5d01b37b4449b


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
  2018-01-09  6:53           ` Takashi Sakamoto
@ 2018-01-09  8:59               ` Jiada Wang
  0 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2018-01-09  8:59 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: Mark Brown, alsa-devel, kuninori.morimoto.gx, linux-kernel,
	tiwai, lgirdwood

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-next
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=33f801366bdf3f8b67dfe325b84f4051a090d01e
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=2e2d53da81af6b2222c6b4e025a5d01b37b4449b
>
>
> Regards
>
> Takashi Sakamoto

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree
@ 2018-01-09  8:59               ` Jiada Wang
  0 siblings, 0 replies; 31+ messages in thread
From: Jiada Wang @ 2018-01-09  8:59 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: Mark Brown, alsa-devel, kuninori.morimoto.gx, linux-kernel,
	tiwai, lgirdwood

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-next
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=33f801366bdf3f8b67dfe325b84f4051a090d01e
> [3] 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/sh?h=for-next&id=2e2d53da81af6b2222c6b4e025a5d01b37b4449b
>
>
> Regards
>
> Takashi Sakamoto

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2018-01-09  8:59 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-07  8:22 [PATCH v1 0/2] fix race condition in rsnd_ssi_pointer_update jiada_wang
2017-12-07  8:22 ` jiada_wang
2017-12-07  8:22 ` [PATCH v1 1/2] ASoC: rsnd: ssi: " jiada_wang
2017-12-07  8:22   ` jiada_wang
2017-12-07  9:45   ` Kuninori Morimoto
2017-12-07  9:45     ` Kuninori Morimoto
2017-12-08  5:35     ` Jiada Wang
2017-12-08  5:35       ` Jiada Wang
2017-12-08  6:00       ` Kuninori Morimoto
2017-12-08  6:00         ` Kuninori Morimoto
2017-12-08 18:52   ` Applied "ASoC: rsnd: ssi: fix race condition in rsnd_ssi_pointer_update" to the asoc tree Mark Brown
2017-12-08 18:52     ` Mark Brown
2017-12-09  5:22     ` Takashi Sakamoto
2017-12-09  5:22       ` Takashi Sakamoto
2017-12-11 11:38       ` Mark Brown
2017-12-11 11:38         ` Mark Brown
2018-01-09  5:42         ` Jiada Wang
2018-01-09  5:42           ` Jiada Wang
2018-01-09  6:53           ` Takashi Sakamoto
2018-01-09  8:59             ` Jiada Wang
2018-01-09  8:59               ` Jiada Wang
2017-12-07  8:22 ` [PATCH v2 2/2] ASoC: rsnd: ssi: remove unnesessary period_pos jiada_wang
2017-12-07  8:22   ` jiada_wang
2017-12-07  9:58   ` Kuninori Morimoto
2017-12-07  9:58     ` Kuninori Morimoto
2017-12-08  5:43     ` Jiada Wang
2017-12-08  5:43       ` Jiada Wang
2017-12-08  6:07       ` Kuninori Morimoto
2017-12-08  6:07         ` Kuninori Morimoto
2017-12-08 18:55   ` Applied "ASoC: rsnd: ssi: remove unnesessary period_pos" to the asoc tree Mark Brown
2017-12-08 18:55     ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.