[alsa-devel] [PATCH] ASoC: fsl: imx-ssi: omit ssi counter to avoid harm in unbalanced situation
Unbalanced calls to imx_ssi_trigger() may result in endless SSI activity and thus provoke eternal sound. While on the first glance, the switch statement looks pretty symmetric, the SUSPEND/RESUME pair is not: the suspend case comes along snd_pcm_suspend_all(), which for fsl/imx-pcm-fiq is called only at snd_soc_suspend(), but the resume case originates straight from the SNDRV_PCM_IOCTL_RESUME. This way userland may provoke an unbalanced resume, which might cause the ssi->enabled counter to increase and never return to zero again, so eventually SSI_SCR_SSIEN is never disabled.
Simply removing the ssi->enabled will solve the problem, as long as one never goes play and capture game simultaneously, but beware trying both at once, the early TRIGGER_STOP will cut off the other activity prematurely. So now playing and capturing is scrutinized separately, instead of by counting.
This is essentially the same stuff as in sound/soc/fsl/imx-pcm-fiq.c which I send a patch for three days ago. Astonishing enough this highly fragile scheme is used twice in parallel to serve the very same control function, synchronously: Once out of sync you are lost until reboot.
Note, that these fixes wont prevent state machine distortion on alsa level to cut sound or the like. It just makes sure we have a chance to synchronise again later on.
Signed-off-by: Oskar Schirmer oskar@scara.com --- sound/soc/fsl/imx-ssi.c | 19 ++++++++++++------- sound/soc/fsl/imx-ssi.h | 3 ++- 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c index f5f248c..c68499b 100644 --- a/sound/soc/fsl/imx-ssi.c +++ b/sound/soc/fsl/imx-ssi.c @@ -298,27 +298,32 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { scr |= SSI_SCR_TE; - else + ssi->playing = 1; + } else { scr |= SSI_SCR_RE; + ssi->capturing = 1; + } sier |= sier_bits;
- if (++ssi->enabled == 1) - scr |= SSI_SCR_SSIEN; + scr |= SSI_SCR_SSIEN;
break;
case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { scr &= ~SSI_SCR_TE; - else + ssi->playing = 0; + } else { scr &= ~SSI_SCR_RE; + ssi->capturing = 0; + } sier &= ~sier_bits;
- if (--ssi->enabled == 0) + if (!ssi->playing && !ssi->capturing) scr &= ~SSI_SCR_SSIEN;
break; diff --git a/sound/soc/fsl/imx-ssi.h b/sound/soc/fsl/imx-ssi.h index 560c40f..a3c90d5 100644 --- a/sound/soc/fsl/imx-ssi.h +++ b/sound/soc/fsl/imx-ssi.h @@ -213,7 +213,8 @@ struct imx_ssi {
int fiq_init; int dma_init; - int enabled; + int playing; + int capturing; };
#endif /* _IMX_SSI_H */
Unbalanced calls to imx_ssi_trigger() may result in endless SSI activity and thus provoke eternal sound. While on the first glance, the switch statement looks pretty symmetric, the SUSPEND/RESUME pair is not: the suspend case comes along snd_pcm_suspend_all(), which for fsl/imx-pcm-fiq is called only at snd_soc_suspend(), but the resume case originates straight from the SNDRV_PCM_IOCTL_RESUME. This way userland may provoke an unbalanced resume, which might cause the ssi->enabled counter to increase and never return to zero again, so eventually SSI_SCR_SSIEN is never disabled.
As the information on whether to enable the SSI or not is contained in the two bits for TE/RE, we save all the software mirroring of hardware state here and simply use the hardware register itself to keep the state of whether someone is currently playing or capturing.
This is essentially the same stuff as in sound/soc/fsl/imx-pcm-fiq.c which I send a patch for three days ago. Astonishing enough this highly fragile scheme is used twice in parallel to serve the very same control function, synchronously: Once out of sync you are lost until reboot.
Note, that these fixes wont prevent state machine distortion on alsa level to cut sound or the like. It just makes sure we have a chance to synchronise again later on.
Signed-off-by: Oskar Schirmer oskar@scara.com --- v2: drop all software flag handling in favour of hardware registers
sound/soc/fsl/imx-ssi.c | 5 ++--- sound/soc/fsl/imx-ssi.h | 1 - 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c index f5f248c..2eb2691 100644 --- a/sound/soc/fsl/imx-ssi.c +++ b/sound/soc/fsl/imx-ssi.c @@ -304,8 +304,7 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, scr |= SSI_SCR_RE; sier |= sier_bits;
- if (++ssi->enabled == 1) - scr |= SSI_SCR_SSIEN; + scr |= SSI_SCR_SSIEN;
break;
@@ -318,7 +317,7 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, scr &= ~SSI_SCR_RE; sier &= ~sier_bits;
- if (--ssi->enabled == 0) + if (!(scr & (SSI_SCR_TE | SSI_SCR_RE))) scr &= ~SSI_SCR_SSIEN;
break; diff --git a/sound/soc/fsl/imx-ssi.h b/sound/soc/fsl/imx-ssi.h index 560c40f..be65623 100644 --- a/sound/soc/fsl/imx-ssi.h +++ b/sound/soc/fsl/imx-ssi.h @@ -213,7 +213,6 @@ struct imx_ssi {
int fiq_init; int dma_init; - int enabled; };
#endif /* _IMX_SSI_H */
Someone around who would like to add his acked-by?
thanks, Oskar
On Sat, Nov 16, 2013 at 07:52:25 +0000, Oskar Schirmer wrote:
Unbalanced calls to imx_ssi_trigger() may result in endless SSI activity and thus provoke eternal sound. While on the first glance, the switch statement looks pretty symmetric, the SUSPEND/RESUME pair is not: the suspend case comes along snd_pcm_suspend_all(), which for fsl/imx-pcm-fiq is called only at snd_soc_suspend(), but the resume case originates straight from the SNDRV_PCM_IOCTL_RESUME. This way userland may provoke an unbalanced resume, which might cause the ssi->enabled counter to increase and never return to zero again, so eventually SSI_SCR_SSIEN is never disabled.
As the information on whether to enable the SSI or not is contained in the two bits for TE/RE, we save all the software mirroring of hardware state here and simply use the hardware register itself to keep the state of whether someone is currently playing or capturing.
This is essentially the same stuff as in sound/soc/fsl/imx-pcm-fiq.c which I send a patch for three days ago. Astonishing enough this highly fragile scheme is used twice in parallel to serve the very same control function, synchronously: Once out of sync you are lost until reboot.
Note, that these fixes wont prevent state machine distortion on alsa level to cut sound or the like. It just makes sure we have a chance to synchronise again later on.
Signed-off-by: Oskar Schirmer oskar@scara.com
v2: drop all software flag handling in favour of hardware registers
sound/soc/fsl/imx-ssi.c | 5 ++--- sound/soc/fsl/imx-ssi.h | 1 - 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c index f5f248c..2eb2691 100644 --- a/sound/soc/fsl/imx-ssi.c +++ b/sound/soc/fsl/imx-ssi.c @@ -304,8 +304,7 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, scr |= SSI_SCR_RE; sier |= sier_bits;
if (++ssi->enabled == 1)
scr |= SSI_SCR_SSIEN;
scr |= SSI_SCR_SSIEN;
break;
@@ -318,7 +317,7 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, scr &= ~SSI_SCR_RE; sier &= ~sier_bits;
if (--ssi->enabled == 0)
if (!(scr & (SSI_SCR_TE | SSI_SCR_RE))) scr &= ~SSI_SCR_SSIEN;
break;
diff --git a/sound/soc/fsl/imx-ssi.h b/sound/soc/fsl/imx-ssi.h index 560c40f..be65623 100644 --- a/sound/soc/fsl/imx-ssi.h +++ b/sound/soc/fsl/imx-ssi.h @@ -213,7 +213,6 @@ struct imx_ssi {
int fiq_init; int dma_init;
- int enabled;
};
#endif /* _IMX_SSI_H */
1.7.9.5
On Sat, Nov 16, 2013 at 07:52:25AM +0000, Oskar Schirmer wrote:
Unbalanced calls to imx_ssi_trigger() may result in endless SSI activity and thus provoke eternal sound. While on the first glance, the switch statement looks pretty symmetric, the SUSPEND/RESUME pair is not: the suspend case comes along snd_pcm_suspend_all(),
Applied, thanks.
participants (2)
-
Mark Brown
-
Oskar Schirmer