2019-11-06 07:32:36

by Shengjiu Wang

Subject: [PATCH] ASoC: fsl_audmix: Add spin lock to protect tdms

Audmix support two substream, When two substream start
to run, the trigger function may be called by two substream
in same time, that the priv->tdms may be updated wrongly.

The expected priv->tdms is 0x3, but sometimes the
result is 0x2, or 0x1.

Signed-off-by: Shengjiu Wang <[email protected]>
---
sound/soc/fsl/fsl_audmix.c | 6 ++++++
sound/soc/fsl/fsl_audmix.h | 1 +
2 files changed, 7 insertions(+)

diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
index c7e4e9757dce..a1db1bce330f 100644
--- a/sound/soc/fsl/fsl_audmix.c
+++ b/sound/soc/fsl/fsl_audmix.c
@@ -286,6 +286,7 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
struct snd_soc_dai *dai)
{
struct fsl_audmix *priv = snd_soc_dai_get_drvdata(dai);
+ unsigned long lock_flags;

/* Capture stream shall not be handled */
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
@@ -295,12 +296,16 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ spin_lock_irqsave(&priv->lock, lock_flags);
priv->tdms |= BIT(dai->driver->id);
+ spin_unlock_irqrestore(&priv->lock, lock_flags);
break;
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ spin_lock_irqsave(&priv->lock, lock_flags);
priv->tdms &= ~BIT(dai->driver->id);
+ spin_unlock_irqrestore(&priv->lock, lock_flags);
break;
default:
return -EINVAL;
@@ -491,6 +496,7 @@ static int fsl_audmix_probe(struct platform_device *pdev)
return PTR_ERR(priv->ipg_clk);
}

+ spin_lock_init(&priv->lock);
platform_set_drvdata(pdev, priv);
pm_runtime_enable(dev);

diff --git a/sound/soc/fsl/fsl_audmix.h b/sound/soc/fsl/fsl_audmix.h
index 7812ffec45c5..479f05695d53 100644
--- a/sound/soc/fsl/fsl_audmix.h
+++ b/sound/soc/fsl/fsl_audmix.h
@@ -96,6 +96,7 @@ struct fsl_audmix {
struct platform_device *pdev;
struct regmap *regmap;
struct clk *ipg_clk;
+ spinlock_t lock; /* Protect tdms */
u8 tdms;
};

--
2.21.0

2019-11-06 11:38:26

by Daniel Baluta

Subject: Re: [alsa-devel] [PATCH] ASoC: fsl_audmix: Add spin lock to protect tdms

Hi Shengjiu,

Comments inline.

On Wed, Nov 6, 2019 at 9:30 AM Shengjiu Wang <[email protected]> wrote:
>
> Audmix support two substream, When two substream start
> to run, the trigger function may be called by two substream
> in same time, that the priv->tdms may be updated wrongly.
>
> The expected priv->tdms is 0x3, but sometimes the
> result is 0x2, or 0x1.
>
> Signed-off-by: Shengjiu Wang <[email protected]>
> ---
> sound/soc/fsl/fsl_audmix.c | 6 ++++++
> sound/soc/fsl/fsl_audmix.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
> index c7e4e9757dce..a1db1bce330f 100644
> --- a/sound/soc/fsl/fsl_audmix.c
> +++ b/sound/soc/fsl/fsl_audmix.c
> @@ -286,6 +286,7 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> struct snd_soc_dai *dai)
> {
> struct fsl_audmix *priv = snd_soc_dai_get_drvdata(dai);
> + unsigned long lock_flags;
>
> /* Capture stream shall not be handled */
> if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> @@ -295,12 +296,16 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> case SNDRV_PCM_TRIGGER_START:
> case SNDRV_PCM_TRIGGER_RESUME:
> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + spin_lock_irqsave(&priv->lock, lock_flags);

Why do we need to disable interrupts here? I assume that lock is only
used in process context.

thanks,
Daniel.

2019-11-09 02:48:02

by Nicolin Chen

Subject: Re: [PATCH] ASoC: fsl_audmix: Add spin lock to protect tdms

On Wed, Nov 06, 2019 at 03:27:45PM +0800, Shengjiu Wang wrote:
> Audmix support two substream, When two substream start
> to run, the trigger function may be called by two substream
> in same time, that the priv->tdms may be updated wrongly.
>
> The expected priv->tdms is 0x3, but sometimes the
> result is 0x2, or 0x1.

Feels like a bug fix to me, so might be better to have a "Fixes"
line and CC stable tree?

Anyway, change looks good to me:

Acked-by: Nicolin Chen <[email protected]>

Thanks

> Signed-off-by: Shengjiu Wang <[email protected]>
> ---
> sound/soc/fsl/fsl_audmix.c | 6 ++++++
> sound/soc/fsl/fsl_audmix.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
> index c7e4e9757dce..a1db1bce330f 100644
> --- a/sound/soc/fsl/fsl_audmix.c
> +++ b/sound/soc/fsl/fsl_audmix.c
> @@ -286,6 +286,7 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> struct snd_soc_dai *dai)
> {
> struct fsl_audmix *priv = snd_soc_dai_get_drvdata(dai);
> + unsigned long lock_flags;
>
> /* Capture stream shall not be handled */
> if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> @@ -295,12 +296,16 @@ static int fsl_audmix_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> case SNDRV_PCM_TRIGGER_START:
> case SNDRV_PCM_TRIGGER_RESUME:
> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + spin_lock_irqsave(&priv->lock, lock_flags);
> priv->tdms |= BIT(dai->driver->id);
> + spin_unlock_irqrestore(&priv->lock, lock_flags);
> break;
> case SNDRV_PCM_TRIGGER_STOP:
> case SNDRV_PCM_TRIGGER_SUSPEND:
> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + spin_lock_irqsave(&priv->lock, lock_flags);
> priv->tdms &= ~BIT(dai->driver->id);
> + spin_unlock_irqrestore(&priv->lock, lock_flags);
> break;
> default:
> return -EINVAL;
> @@ -491,6 +496,7 @@ static int fsl_audmix_probe(struct platform_device *pdev)
> return PTR_ERR(priv->ipg_clk);
> }
>
> + spin_lock_init(&priv->lock);
> platform_set_drvdata(pdev, priv);
> pm_runtime_enable(dev);
>
> diff --git a/sound/soc/fsl/fsl_audmix.h b/sound/soc/fsl/fsl_audmix.h
> index 7812ffec45c5..479f05695d53 100644
> --- a/sound/soc/fsl/fsl_audmix.h
> +++ b/sound/soc/fsl/fsl_audmix.h
> @@ -96,6 +96,7 @@ struct fsl_audmix {
> struct platform_device *pdev;
> struct regmap *regmap;
> struct clk *ipg_clk;
> + spinlock_t lock; /* Protect tdms */
> u8 tdms;
> };
>
> --
> 2.21.0
>

2019-11-11 07:39:15

by Shengjiu Wang

Subject: Re: [alsa-devel] [PATCH] ASoC: fsl_audmix: Add spin lock to protect tdms

Hi

On Sat, Nov 9, 2019 at 10:48 AM Nicolin Chen <[email protected]> wrote:
>
> On Wed, Nov 06, 2019 at 03:27:45PM +0800, Shengjiu Wang wrote:
> > Audmix support two substream, When two substream start
> > to run, the trigger function may be called by two substream
> > in same time, that the priv->tdms may be updated wrongly.
> >
> > The expected priv->tdms is 0x3, but sometimes the
> > result is 0x2, or 0x1.
>
> Feels like a bug fix to me, so might be better to have a "Fixes"
> line and CC stable tree?
>
> Anyway, change looks good to me:
>
> Acked-by: Nicolin Chen <[email protected]>
>

Ok, will send v2.

best regards
wang shengjiu