[Question about DPCM] dpcm_run_new_update races again xrun

[Question about DPCM] dpcm_run_new_update races again xrun

[Qiao Zhou]

Hi Mark, Liam

I met one BE not-function issue when developing DPCM feature, and found
dpcm_run_new_update is not protected well against xrun(interrupt context).
Could you help to give some suggestions?

The scenario is like this, taking audio playback for example:
1, FE1 <-> BE1, working well for some time.
2, disconnect BE1 and connect BE2(FE1 <-> BE2)
3, during FE1 connecting BE2, FE1 is still streaming data normally. Then an
under-run happens. Below are the code sequence.
    
soc_dpcm_runtime_update() {
    ...
    dpcm_connect_be() // connect FE1 & BE2
    dpcm_run_new_update(fe, stream) {
      fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_BE
      dpcm_run_update_startup(fe, stream) {
        dpcm_be_dai_startup(fe, stream)
    _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
   |    // under-run happens (interrupt context)                       |
   |    snd_pcm_stop(substream) {                                      |
   |        dpcm_fe_dai_trigger(STOP) {                                |
   |            fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_FE       |
   |            // trigger stop FE1, BE2                               |
   |            fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO       |
   |        }                                                          |
   |    }                                                              |
   |_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _|
        dpcm_be_dai_hw_params(fe, stream)
        dpcm_be_dai_prepare(fe, stream)
        if(fe state == STOP)
            return 0;
        dpcm_be_dai_trigger(fe, ....)
      fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO
    ...
    }
}

After xrun handler finishes, the FE runtime update staus is UPDATE_NO. Then
the following dpcm_be_dai_hw_params & dpcm_be_dai_prepare skip related driver
API excuting with this FE runtime status, and return 0 to end runtime-startup.

When user APP(ALSA lib/TinyALSA) detects xrun, usually it will do the substream
prepare and write data again. Due to BE dai has not been ready for hw_params,
the BE dai can't work properly after substream prepare and trigger start. After
that system has no sound and can't be recovered, maybe due to FE1 doesn't know
what's going on inside BE2.

The under-run is random, and under-run handler does what's necessary to be done,
though trigger-stop a BE which has not been started yet is not good in my opinion.

I did an experiment to avoid be udpate checking in dpcm_be_dai_hw_params by
Commenting out "if(!snd_soc_dpcm_be_can_update()}". This change is verified OK
since APP will do the prepare & trigger start to resume the under-run. The patch
is also attached. 

>From current code, dpcm_be_dai_hw_params is only callded by fe_hw_params & this
runtime startup. This be update checking might be redundant or unnecessary in
current code context.

Could you help to give some suggestions? Please help to correct me if anything
wrong. Thanks in advance.

---
 sound/soc/soc-pcm.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 002311a..dfeb1e7 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1697,10 +1697,6 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
 		struct snd_pcm_substream *be_substream =
 			snd_soc_dpcm_get_substream(be, stream);
 
-		/* is this op for this BE ? */
-		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
-			continue;
-
 		/* only allow hw_params() if no connected FEs are running */
 		if (!snd_soc_dpcm_can_be_params(fe, be, stream))
 			continue;
-- 
1.7.9.5

Best Regards
Qiao

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Liam Girdwood]

On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> Hi Mark, Liam
> 
> I met one BE not-function issue when developing DPCM feature, and found
> dpcm_run_new_update is not protected well against xrun(interrupt context).
> Could you help to give some suggestions?
> 

I'm wondering if this would be better solved by improving the locking so
that an XRUN could not run at the same time as the runtime update. Both
functions are async, but are only protected by a mutex atm (like the
rest of PCM ops except trigger which is atomic). We maybe need to add a
spinlock to both runtime_update() and dpcm_fe_dai_trigger() 

Liam

> The scenario is like this, taking audio playback for example:
> 1, FE1 <-> BE1, working well for some time.
> 2, disconnect BE1 and connect BE2(FE1 <-> BE2)
> 3, during FE1 connecting BE2, FE1 is still streaming data normally. Then an
> under-run happens. Below are the code sequence.
>     
> soc_dpcm_runtime_update() {
>     ...
>     dpcm_connect_be() // connect FE1 & BE2
>     dpcm_run_new_update(fe, stream) {
>       fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_BE
>       dpcm_run_update_startup(fe, stream) {
>         dpcm_be_dai_startup(fe, stream)
>     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
>    |    // under-run happens (interrupt context)                       |
>    |    snd_pcm_stop(substream) {                                      |
>    |        dpcm_fe_dai_trigger(STOP) {                                |
>    |            fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_FE       |
>    |            // trigger stop FE1, BE2                               |
>    |            fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO       |
>    |        }                                                          |
>    |    }                                                              |
>    |_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _|
>         dpcm_be_dai_hw_params(fe, stream)
>         dpcm_be_dai_prepare(fe, stream)
>         if(fe state == STOP)
>             return 0;
>         dpcm_be_dai_trigger(fe, ....)
>       fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO
>     ...
>     }
> }
> 
> After xrun handler finishes, the FE runtime update staus is UPDATE_NO. Then
> the following dpcm_be_dai_hw_params & dpcm_be_dai_prepare skip related driver
> API excuting with this FE runtime status, and return 0 to end runtime-startup.
> 
> When user APP(ALSA lib/TinyALSA) detects xrun, usually it will do the substream
> prepare and write data again. Due to BE dai has not been ready for hw_params,
> the BE dai can't work properly after substream prepare and trigger start. After
> that system has no sound and can't be recovered, maybe due to FE1 doesn't know
> what's going on inside BE2.
> 
> The under-run is random, and under-run handler does what's necessary to be done,
> though trigger-stop a BE which has not been started yet is not good in my opinion.
> 
> I did an experiment to avoid be udpate checking in dpcm_be_dai_hw_params by
> Commenting out "if(!snd_soc_dpcm_be_can_update()}". This change is verified OK
> since APP will do the prepare & trigger start to resume the under-run. The patch
> is also attached. 
> 
> From current code, dpcm_be_dai_hw_params is only callded by fe_hw_params & this
> runtime startup. This be update checking might be redundant or unnecessary in
> current code context.
> 
> Could you help to give some suggestions? Please help to correct me if anything
> wrong. Thanks in advance.
> 
> ---
>  sound/soc/soc-pcm.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 002311a..dfeb1e7 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1697,10 +1697,6 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
>  		struct snd_pcm_substream *be_substream =
>  			snd_soc_dpcm_get_substream(be, stream);
>  
> -		/* is this op for this BE ? */
> -		if (!snd_soc_dpcm_be_can_update(fe, be, stream))
> -			continue;
> -
>  		/* only allow hw_params() if no connected FEs are running */
>  		if (!snd_soc_dpcm_can_be_params(fe, be, stream))
>  			continue;


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Takashi Iwai]

At Mon, 03 Nov 2014 13:32:04 +0000,
Liam Girdwood wrote:
> 
> On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > Hi Mark, Liam
> > 
> > I met one BE not-function issue when developing DPCM feature, and found
> > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > Could you help to give some suggestions?
> > 
> 
> I'm wondering if this would be better solved by improving the locking so
> that an XRUN could not run at the same time as the runtime update. Both
> functions are async, but are only protected by a mutex atm (like the
> rest of PCM ops except trigger which is atomic). We maybe need to add a
> spinlock to both runtime_update() and dpcm_fe_dai_trigger() 

Be careful, you cannot take spinlock while hw_params, for example.

Why do you need to set runtime_update to NO in the trigger callback in
the first place?  The trigger may influence on the FE streaming state,
but not on the FE/BE connection state, right?


Takashi

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Takashi Iwai]

At Mon, 03 Nov 2014 15:42:16 +0100,
Takashi Iwai wrote:
> 
> At Mon, 03 Nov 2014 13:32:04 +0000,
> Liam Girdwood wrote:
> > 
> > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > Hi Mark, Liam
> > > 
> > > I met one BE not-function issue when developing DPCM feature, and found
> > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > Could you help to give some suggestions?
> > > 
> > 
> > I'm wondering if this would be better solved by improving the locking so
> > that an XRUN could not run at the same time as the runtime update. Both
> > functions are async, but are only protected by a mutex atm (like the
> > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > spinlock to both runtime_update() and dpcm_fe_dai_trigger() 
> 
> Be careful, you cannot take spinlock while hw_params, for example.
> 
> Why do you need to set runtime_update to NO in the trigger callback in
> the first place?  The trigger may influence on the FE streaming state,
> but not on the FE/BE connection state, right?

Thinking of this again, this doesn't look like a good suggestion,
either.

As mentioned, we can't take a lock for the whole lengthy operation
like prepare or hw_params.  So, instead of taking a lock, the trigger
should be delayed when some operation is being performed.  (I thought
at first just performing FE's trigger and delaying BE later, but FE/BE
trigger can be more complex than that, so it ends up with the whole
delayed trigger.)

The patch below is a quick hack (only compile tested!).  It uses PCM's
stream lock for protecting against the race for the state transition,
and the trigger callback checks the state, aborts immediately if there
is a concurrent operation.  At the exit of state change, it invokes
the delayed trigger.

Let me know if this really works as imagined.  Then I'll cook up a
proper patch.


Takashi

---
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 2883a7a6f9f3..98f2ade0266e 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -102,6 +102,8 @@ struct snd_soc_dpcm_runtime {
 	/* state and update */
 	enum snd_soc_dpcm_update runtime_update;
 	enum snd_soc_dpcm_state state;
+
+	int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
 };
 
 /* can this BE stop and free */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 002311afdeaa..fbd570c55a67 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1522,13 +1522,36 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
 		dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture);
 }
 
+static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd);
+
+/* Set FE's runtime_update state; the state is protected via PCM stream lock
+ * for avoiding the race with trigger callback.
+ * If the state is unset and a trigger is pending while the previous operation,
+ * process the pending trigger action here.
+ */
+static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
+				     int stream, enum snd_soc_dpcm_update state)
+{
+	struct snd_pcm_substream *substream =
+		snd_soc_dpcm_get_substream(fe, stream);
+
+	snd_pcm_stream_lock_irq(substream);
+	fe->dpcm[stream].runtime_update = state;
+	if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) {
+		dpcm_fe_dai_do_trigger(substream,
+				       fe->dpcm[stream].trigger_pending - 1);
+		fe->dpcm[stream].trigger_pending = 0;
+	}
+	snd_pcm_stream_unlock_irq(substream);
+}
+
 static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 {
 	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
 	struct snd_pcm_runtime *runtime = fe_substream->runtime;
 	int stream = fe_substream->stream, ret = 0;
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	ret = dpcm_be_dai_startup(fe, fe_substream->stream);
 	if (ret < 0) {
@@ -1550,13 +1573,13 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
 	dpcm_set_fe_runtime(fe_substream);
 	snd_pcm_limit_hw_rates(runtime);
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	return 0;
 
 unwind:
 	dpcm_be_dai_startup_unwind(fe, fe_substream->stream);
 be_err:
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	return ret;
 }
 
@@ -1603,7 +1626,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *fe = substream->private_data;
 	int stream = substream->stream;
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	/* shutdown the BEs */
 	dpcm_be_dai_shutdown(fe, substream->stream);
@@ -1617,7 +1640,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
 	dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
 
 	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	return 0;
 }
 
@@ -1665,7 +1688,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
 	int err, stream = substream->stream;
 
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
 
@@ -1680,7 +1703,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
 	err = dpcm_be_dai_hw_free(fe, stream);
 
 	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 
 	mutex_unlock(&fe->card->mutex);
 	return 0;
@@ -1773,7 +1796,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 	int ret, stream = substream->stream;
 
 	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	memcpy(&fe->dpcm[substream->stream].hw_params, params,
 			sizeof(struct snd_pcm_hw_params));
@@ -1796,7 +1819,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 		fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
 
 out:
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	mutex_unlock(&fe->card->mutex);
 	return ret;
 }
@@ -1910,14 +1933,12 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 }
 EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
 
-static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
+static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_soc_pcm_runtime *fe = substream->private_data;
 	int stream = substream->stream, ret;
 	enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
-
 	switch (trigger) {
 	case SND_SOC_DPCM_TRIGGER_PRE:
 		/* call trigger on the frontend before the backend. */
@@ -1928,7 +1949,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
 		ret = soc_pcm_trigger(substream, cmd);
 		if (ret < 0) {
 			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
-			goto out;
+			return ret;
 		}
 
 		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
@@ -1939,7 +1960,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
 		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
 		if (ret < 0) {
 			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
-			goto out;
+			return ret;
 		}
 
 		dev_dbg(fe->dev, "ASoC: post trigger FE %s cmd %d\n",
@@ -1956,14 +1977,13 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
 		ret = soc_pcm_bespoke_trigger(substream, cmd);
 		if (ret < 0) {
 			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
-			goto out;
+			return ret;
 		}
 		break;
 	default:
 		dev_err(fe->dev, "ASoC: invalid trigger cmd %d for %s\n", cmd,
 				fe->dai_link->name);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	switch (cmd) {
@@ -1979,7 +1999,25 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
 		break;
 	}
 
-out:
+	return 0;
+}
+
+static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+	struct snd_soc_pcm_runtime *fe = substream->private_data;
+	int stream = substream->stream, ret;
+
+	/* if FE's runtime_update is already set, we're in race;
+	 * process this trigger later at exit
+	 */
+	if (fe->dpcm[stream].runtime_update != SND_SOC_DPCM_UPDATE_NO) {
+		fe->dpcm[stream].trigger_pending = cmd + 1;
+		return 0; /* delayed, assuming it's successful */
+	}
+
+	/* we're alone, let's trigger */
+	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	ret = dpcm_fe_dai_do_trigger(substream, cmd);
 	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
 	return ret;
 }
@@ -2027,7 +2065,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 
 	dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
 
 	/* there is no point preparing this FE if there are no BEs */
 	if (list_empty(&fe->dpcm[stream].be_clients)) {
@@ -2054,7 +2092,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE;
 
 out:
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 	mutex_unlock(&fe->card->mutex);
 
 	return ret;
@@ -2201,11 +2239,11 @@ static int dpcm_run_new_update(struct snd_soc_pcm_runtime *fe, int stream)
 {
 	int ret;
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
 	ret = dpcm_run_update_startup(fe, stream);
 	if (ret < 0)
 		dev_err(fe->dev, "ASoC: failed to startup some BEs\n");
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 
 	return ret;
 }
@@ -2214,11 +2252,11 @@ static int dpcm_run_old_update(struct snd_soc_pcm_runtime *fe, int stream)
 {
 	int ret;
 
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
 	ret = dpcm_run_update_shutdown(fe, stream);
 	if (ret < 0)
 		dev_err(fe->dev, "ASoC: failed to shutdown some BEs\n");
-	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
 
 	return ret;
 }

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Liam Girdwood]

On Mon, 2014-11-03 at 15:42 +0100, Takashi Iwai wrote:
> At Mon, 03 Nov 2014 13:32:04 +0000,
> Liam Girdwood wrote:
> > 
> > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > Hi Mark, Liam
> > > 
> > > I met one BE not-function issue when developing DPCM feature, and found
> > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > Could you help to give some suggestions?
> > > 
> > 
> > I'm wondering if this would be better solved by improving the locking so
> > that an XRUN could not run at the same time as the runtime update. Both
> > functions are async, but are only protected by a mutex atm (like the
> > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > spinlock to both runtime_update() and dpcm_fe_dai_trigger() 
> 
> Be careful, you cannot take spinlock while hw_params, for example.
> 
> Why do you need to set runtime_update to NO in the trigger callback in
> the first place?  The trigger may influence on the FE streaming state,
> but not on the FE/BE connection state, right?
> 

The XRUN trigger will propagate to the BE atm, but the BE DSP DAIs
should never really XRUN (since the DSP does the IO) meaning the XRUN
will have no influence on the BE.

I guess we could ignore the XRUN on the BE if that's what you are
meaning ?

Liam
> 
> Takashi


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Liam Girdwood]

On Mon, 2014-11-03 at 17:16 +0000, Liam Girdwood wrote:
> On Mon, 2014-11-03 at 15:42 +0100, Takashi Iwai wrote:
> > At Mon, 03 Nov 2014 13:32:04 +0000,
> > Liam Girdwood wrote:
> > > 
> > > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > > Hi Mark, Liam
> > > > 
> > > > I met one BE not-function issue when developing DPCM feature, and found
> > > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > > Could you help to give some suggestions?
> > > > 
> > > 
> > > I'm wondering if this would be better solved by improving the locking so
> > > that an XRUN could not run at the same time as the runtime update. Both
> > > functions are async, but are only protected by a mutex atm (like the
> > > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > > spinlock to both runtime_update() and dpcm_fe_dai_trigger() 
> > 
> > Be careful, you cannot take spinlock while hw_params, for example.
> > 
> > Why do you need to set runtime_update to NO in the trigger callback in
> > the first place?  The trigger may influence on the FE streaming state,
> > but not on the FE/BE connection state, right?
> > 
> 
> The XRUN trigger will propagate to the BE atm, but the BE DSP DAIs
> should never really XRUN (since the DSP does the IO) meaning the XRUN
> will have no influence on the BE.
> 
> I guess we could ignore the XRUN on the BE if that's what you are
> meaning ?
> 

Oh, hit send before seeing your last reply. Ignore.

Liam

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Liam Girdwood]

On Mon, 2014-11-03 at 17:54 +0100, Takashi Iwai wrote:
> At Mon, 03 Nov 2014 15:42:16 +0100,
> Takashi Iwai wrote:
> > 
> > At Mon, 03 Nov 2014 13:32:04 +0000,
> > Liam Girdwood wrote:
> > > 
> > > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > > Hi Mark, Liam
> > > > 
> > > > I met one BE not-function issue when developing DPCM feature, and found
> > > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > > Could you help to give some suggestions?
> > > > 
> > > 
> > > I'm wondering if this would be better solved by improving the locking so
> > > that an XRUN could not run at the same time as the runtime update. Both
> > > functions are async, but are only protected by a mutex atm (like the
> > > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > > spinlock to both runtime_update() and dpcm_fe_dai_trigger() 
> > 
> > Be careful, you cannot take spinlock while hw_params, for example.
> > 
> > Why do you need to set runtime_update to NO in the trigger callback in
> > the first place?  The trigger may influence on the FE streaming state,
> > but not on the FE/BE connection state, right?
> 
> Thinking of this again, this doesn't look like a good suggestion,
> either.
> 
> As mentioned, we can't take a lock for the whole lengthy operation
> like prepare or hw_params.  So, instead of taking a lock, the trigger
> should be delayed when some operation is being performed.  (I thought
> at first just performing FE's trigger and delaying BE later, but FE/BE
> trigger can be more complex than that, so it ends up with the whole
> delayed trigger.)
> 
> The patch below is a quick hack (only compile tested!).  It uses PCM's
> stream lock for protecting against the race for the state transition,
> and the trigger callback checks the state, aborts immediately if there
> is a concurrent operation.  At the exit of state change, it invokes
> the delayed trigger.
> 
> Let me know if this really works as imagined.  Then I'll cook up a
> proper patch.
> 

I'll give the patch a try tomorrow and it would be good for Marvell to
test tomorrow too (as my system does not XRUN).

Thanks

Liam

> 
> Takashi
> 
> ---
> diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
> index 2883a7a6f9f3..98f2ade0266e 100644
> --- a/include/sound/soc-dpcm.h
> +++ b/include/sound/soc-dpcm.h
> @@ -102,6 +102,8 @@ struct snd_soc_dpcm_runtime {
>  	/* state and update */
>  	enum snd_soc_dpcm_update runtime_update;
>  	enum snd_soc_dpcm_state state;
> +
> +	int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
>  };
>  
>  /* can this BE stop and free */
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 002311afdeaa..fbd570c55a67 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1522,13 +1522,36 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
>  		dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture);
>  }
>  
> +static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd);
> +
> +/* Set FE's runtime_update state; the state is protected via PCM stream lock
> + * for avoiding the race with trigger callback.
> + * If the state is unset and a trigger is pending while the previous operation,
> + * process the pending trigger action here.
> + */
> +static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
> +				     int stream, enum snd_soc_dpcm_update state)
> +{
> +	struct snd_pcm_substream *substream =
> +		snd_soc_dpcm_get_substream(fe, stream);
> +
> +	snd_pcm_stream_lock_irq(substream);
> +	fe->dpcm[stream].runtime_update = state;
> +	if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) {
> +		dpcm_fe_dai_do_trigger(substream,
> +				       fe->dpcm[stream].trigger_pending - 1);
> +		fe->dpcm[stream].trigger_pending = 0;
> +	}
> +	snd_pcm_stream_unlock_irq(substream);
> +}
> +
>  static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
>  {
>  	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
>  	struct snd_pcm_runtime *runtime = fe_substream->runtime;
>  	int stream = fe_substream->stream, ret = 0;
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>  
>  	ret = dpcm_be_dai_startup(fe, fe_substream->stream);
>  	if (ret < 0) {
> @@ -1550,13 +1573,13 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
>  	dpcm_set_fe_runtime(fe_substream);
>  	snd_pcm_limit_hw_rates(runtime);
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  	return 0;
>  
>  unwind:
>  	dpcm_be_dai_startup_unwind(fe, fe_substream->stream);
>  be_err:
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  	return ret;
>  }
>  
> @@ -1603,7 +1626,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
>  	struct snd_soc_pcm_runtime *fe = substream->private_data;
>  	int stream = substream->stream;
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>  
>  	/* shutdown the BEs */
>  	dpcm_be_dai_shutdown(fe, substream->stream);
> @@ -1617,7 +1640,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
>  	dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
>  
>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  	return 0;
>  }
>  
> @@ -1665,7 +1688,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
>  	int err, stream = substream->stream;
>  
>  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>  
>  	dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
>  
> @@ -1680,7 +1703,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
>  	err = dpcm_be_dai_hw_free(fe, stream);
>  
>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  
>  	mutex_unlock(&fe->card->mutex);
>  	return 0;
> @@ -1773,7 +1796,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>  	int ret, stream = substream->stream;
>  
>  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>  
>  	memcpy(&fe->dpcm[substream->stream].hw_params, params,
>  			sizeof(struct snd_pcm_hw_params));
> @@ -1796,7 +1819,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>  		fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
>  
>  out:
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  	mutex_unlock(&fe->card->mutex);
>  	return ret;
>  }
> @@ -1910,14 +1933,12 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  }
>  EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
>  
> -static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
> +static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd)
>  {
>  	struct snd_soc_pcm_runtime *fe = substream->private_data;
>  	int stream = substream->stream, ret;
>  	enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> -
>  	switch (trigger) {
>  	case SND_SOC_DPCM_TRIGGER_PRE:
>  		/* call trigger on the frontend before the backend. */
> @@ -1928,7 +1949,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
>  		ret = soc_pcm_trigger(substream, cmd);
>  		if (ret < 0) {
>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
> -			goto out;
> +			return ret;
>  		}
>  
>  		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
> @@ -1939,7 +1960,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
>  		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
>  		if (ret < 0) {
>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
> -			goto out;
> +			return ret;
>  		}
>  
>  		dev_dbg(fe->dev, "ASoC: post trigger FE %s cmd %d\n",
> @@ -1956,14 +1977,13 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
>  		ret = soc_pcm_bespoke_trigger(substream, cmd);
>  		if (ret < 0) {
>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
> -			goto out;
> +			return ret;
>  		}
>  		break;
>  	default:
>  		dev_err(fe->dev, "ASoC: invalid trigger cmd %d for %s\n", cmd,
>  				fe->dai_link->name);
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
>  
>  	switch (cmd) {
> @@ -1979,7 +1999,25 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
>  		break;
>  	}
>  
> -out:
> +	return 0;
> +}
> +
> +static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *fe = substream->private_data;
> +	int stream = substream->stream, ret;
> +
> +	/* if FE's runtime_update is already set, we're in race;
> +	 * process this trigger later at exit
> +	 */
> +	if (fe->dpcm[stream].runtime_update != SND_SOC_DPCM_UPDATE_NO) {
> +		fe->dpcm[stream].trigger_pending = cmd + 1;
> +		return 0; /* delayed, assuming it's successful */
> +	}
> +
> +	/* we're alone, let's trigger */
> +	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	ret = dpcm_fe_dai_do_trigger(substream, cmd);
>  	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>  	return ret;
>  }
> @@ -2027,7 +2065,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>  
>  	dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>  
>  	/* there is no point preparing this FE if there are no BEs */
>  	if (list_empty(&fe->dpcm[stream].be_clients)) {
> @@ -2054,7 +2092,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE;
>  
>  out:
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  	mutex_unlock(&fe->card->mutex);
>  
>  	return ret;
> @@ -2201,11 +2239,11 @@ static int dpcm_run_new_update(struct snd_soc_pcm_runtime *fe, int stream)
>  {
>  	int ret;
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
>  	ret = dpcm_run_update_startup(fe, stream);
>  	if (ret < 0)
>  		dev_err(fe->dev, "ASoC: failed to startup some BEs\n");
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  
>  	return ret;
>  }
> @@ -2214,11 +2252,11 @@ static int dpcm_run_old_update(struct snd_soc_pcm_runtime *fe, int stream)
>  {
>  	int ret;
>  
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
>  	ret = dpcm_run_update_shutdown(fe, stream);
>  	if (ret < 0)
>  		dev_err(fe->dev, "ASoC: failed to shutdown some BEs\n");
> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>  
>  	return ret;
>  }

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Takashi Iwai]

At Mon, 03 Nov 2014 17:20:12 +0000,
Liam Girdwood wrote:
> 
> On Mon, 2014-11-03 at 17:54 +0100, Takashi Iwai wrote:
> > At Mon, 03 Nov 2014 15:42:16 +0100,
> > Takashi Iwai wrote:
> > > 
> > > At Mon, 03 Nov 2014 13:32:04 +0000,
> > > Liam Girdwood wrote:
> > > > 
> > > > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > > > Hi Mark, Liam
> > > > > 
> > > > > I met one BE not-function issue when developing DPCM feature, and found
> > > > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > > > Could you help to give some suggestions?
> > > > > 
> > > > 
> > > > I'm wondering if this would be better solved by improving the locking so
> > > > that an XRUN could not run at the same time as the runtime update. Both
> > > > functions are async, but are only protected by a mutex atm (like the
> > > > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > > > spinlock to both runtime_update() and dpcm_fe_dai_trigger() 
> > > 
> > > Be careful, you cannot take spinlock while hw_params, for example.
> > > 
> > > Why do you need to set runtime_update to NO in the trigger callback in
> > > the first place?  The trigger may influence on the FE streaming state,
> > > but not on the FE/BE connection state, right?
> > 
> > Thinking of this again, this doesn't look like a good suggestion,
> > either.
> > 
> > As mentioned, we can't take a lock for the whole lengthy operation
> > like prepare or hw_params.  So, instead of taking a lock, the trigger
> > should be delayed when some operation is being performed.  (I thought
> > at first just performing FE's trigger and delaying BE later, but FE/BE
> > trigger can be more complex than that, so it ends up with the whole
> > delayed trigger.)
> > 
> > The patch below is a quick hack (only compile tested!).  It uses PCM's
> > stream lock for protecting against the race for the state transition,
> > and the trigger callback checks the state, aborts immediately if there
> > is a concurrent operation.  At the exit of state change, it invokes
> > the delayed trigger.
> > 
> > Let me know if this really works as imagined.  Then I'll cook up a
> > proper patch.
> > 
> 
> I'll give the patch a try tomorrow and it would be good for Marvell to
> test tomorrow too (as my system does not XRUN).

FWIW, below is a simple hack to simulate an XRUN via proc file.
Just write any value xrun_injection proc file and it triggers XRUN,
e.g.
   # echo > /proc/asound/card0/pcm0p/sub0/xrun_injection

For simulating the race, you should impose some artificial delay in
the function you'll try to conflict, then trigger xrun.


Takashi

---
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index e862497f7556..9e46132529c4 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -416,6 +416,7 @@ struct snd_pcm_substream {
 	struct snd_info_entry *proc_status_entry;
 	struct snd_info_entry *proc_prealloc_entry;
 	struct snd_info_entry *proc_prealloc_max_entry;
+	struct snd_info_entry *proc_xrun_injection;
 #endif
 	/* misc flags */
 	unsigned int hw_opened: 1;
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 42ded997b223..fa442d0504f0 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -478,6 +478,19 @@ static void snd_pcm_substream_proc_status_read(struct snd_info_entry *entry,
 	mutex_unlock(&substream->pcm->open_mutex);
 }
 
+static void snd_pcm_xrun_injection_write(struct snd_info_entry *entry,
+					 struct snd_info_buffer *buffer)
+{
+	struct snd_pcm_substream *substream = entry->private_data;
+	struct snd_pcm_runtime *runtime;
+
+	snd_pcm_stream_lock_irq(substream);
+	runtime = substream->runtime;
+	if (runtime && runtime->status->state == SNDRV_PCM_STATE_RUNNING)
+		snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
+	snd_pcm_stream_unlock_irq(substream);
+}
+
 #ifdef CONFIG_SND_PCM_XRUN_DEBUG
 static void snd_pcm_xrun_debug_read(struct snd_info_entry *entry,
 				    struct snd_info_buffer *buffer)
@@ -610,6 +623,20 @@ static int snd_pcm_substream_proc_init(struct snd_pcm_substream *substream)
 	}
 	substream->proc_status_entry = entry;
 
+	entry = snd_info_create_card_entry(card, "xrun_injection",
+					   substream->proc_root);
+	if (entry) {
+		entry->private_data = substream;
+		entry->c.text.read = NULL;
+		entry->c.text.write = snd_pcm_xrun_injection_write;
+		entry->mode = S_IFREG | S_IWUSR;
+		if (snd_info_register(entry) < 0) {
+			snd_info_free_entry(entry);
+			entry = NULL;
+		}
+	}
+	substream->proc_status_entry = entry;
+
 	return 0;
 }
 
@@ -623,6 +650,8 @@ static int snd_pcm_substream_proc_done(struct snd_pcm_substream *substream)
 	substream->proc_sw_params_entry = NULL;
 	snd_info_free_entry(substream->proc_status_entry);
 	substream->proc_status_entry = NULL;
+	snd_info_free_entry(substream->proc_xrun_injection);
+	substream->proc_xrun_injection = NULL;
 	snd_info_free_entry(substream->proc_root);
 	substream->proc_root = NULL;
 	return 0;

Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Qiao Zhou]

>-----Original Message-----
>From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com]
>Sent: 2014年11月4日 1:20
>To: Takashi Iwai
>Cc: Qiao Zhou; Mark Brown; perex@perex.cz; alsa-devel@alsa-project.org;
>Wilbur Wang; Chao Xie
>Subject: Re: [Question about DPCM] dpcm_run_new_update races again xrun
>
>On Mon, 2014-11-03 at 17:54 +0100, Takashi Iwai wrote:
>> At Mon, 03 Nov 2014 15:42:16 +0100,
>> Takashi Iwai wrote:
>> >
>> > At Mon, 03 Nov 2014 13:32:04 +0000,
>> > Liam Girdwood wrote:
>> > >
>> > > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
>> > > > Hi Mark, Liam
>> > > >
>> > > > I met one BE not-function issue when developing DPCM feature,
>> > > > and found dpcm_run_new_update is not protected well against
>xrun(interrupt context).
>> > > > Could you help to give some suggestions?
>> > > >
>> > >
>> > > I'm wondering if this would be better solved by improving the
>> > > locking so that an XRUN could not run at the same time as the
>> > > runtime update. Both functions are async, but are only protected
>> > > by a mutex atm (like the rest of PCM ops except trigger which is
>> > > atomic). We maybe need to add a spinlock to both runtime_update()
>> > > and dpcm_fe_dai_trigger()
>> >
>> > Be careful, you cannot take spinlock while hw_params, for example.
>> >
>> > Why do you need to set runtime_update to NO in the trigger callback
>> > in the first place?  The trigger may influence on the FE streaming
>> > state, but not on the FE/BE connection state, right?
>>
>> Thinking of this again, this doesn't look like a good suggestion,
>> either.
>>
>> As mentioned, we can't take a lock for the whole lengthy operation
>> like prepare or hw_params.  So, instead of taking a lock, the trigger
>> should be delayed when some operation is being performed.  (I thought
>> at first just performing FE's trigger and delaying BE later, but FE/BE
>> trigger can be more complex than that, so it ends up with the whole
>> delayed trigger.)
>>
>> The patch below is a quick hack (only compile tested!).  It uses PCM's
>> stream lock for protecting against the race for the state transition,
>> and the trigger callback checks the state, aborts immediately if there
>> is a concurrent operation.  At the exit of state change, it invokes
>> the delayed trigger.
>>
>> Let me know if this really works as imagined.  Then I'll cook up a
>> proper patch.
>>
>
>I'll give the patch a try tomorrow and it would be good for Marvell to
>test tomorrow too (as my system does not XRUN).
>
>Thanks
>
>Liam
>
>>
>> Takashi
>>
>> ---
>> diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h index
>> 2883a7a6f9f3..98f2ade0266e 100644
>> --- a/include/sound/soc-dpcm.h
>> +++ b/include/sound/soc-dpcm.h
>> @@ -102,6 +102,8 @@ struct snd_soc_dpcm_runtime {
>>  	/* state and update */
>>  	enum snd_soc_dpcm_update runtime_update;
>>  	enum snd_soc_dpcm_state state;
>> +
>> +	int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
>>  };
>>
>>  /* can this BE stop and free */
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index
>> 002311afdeaa..fbd570c55a67 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -1522,13 +1522,36 @@ static void dpcm_set_fe_runtime(struct
>snd_pcm_substream *substream)
>>  		dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture);  }
>>
>> +static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream
>> +*substream, int cmd);
>> +
>> +/* Set FE's runtime_update state; the state is protected via PCM
>> +stream lock
>> + * for avoiding the race with trigger callback.
>> + * If the state is unset and a trigger is pending while the previous
>> +operation,
>> + * process the pending trigger action here.
>> + */
>> +static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
>> +				     int stream, enum snd_soc_dpcm_update state)
>{
>> +	struct snd_pcm_substream *substream =
>> +		snd_soc_dpcm_get_substream(fe, stream);
>> +
>> +	snd_pcm_stream_lock_irq(substream);
>> +	fe->dpcm[stream].runtime_update = state;
>> +	if (state == SND_SOC_DPCM_UPDATE_NO && fe-
>>dpcm[stream].trigger_pending) {
>> +		dpcm_fe_dai_do_trigger(substream,
>> +				       fe->dpcm[stream].trigger_pending - 1);
>> +		fe->dpcm[stream].trigger_pending = 0;
>> +	}
>> +	snd_pcm_stream_unlock_irq(substream);
>> +}
>> +
>>  static int dpcm_fe_dai_startup(struct snd_pcm_substream
>> *fe_substream)  {
>>  	struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
>>  	struct snd_pcm_runtime *runtime = fe_substream->runtime;
>>  	int stream = fe_substream->stream, ret = 0;
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>>
>>  	ret = dpcm_be_dai_startup(fe, fe_substream->stream);
>>  	if (ret < 0) {
>> @@ -1550,13 +1573,13 @@ static int dpcm_fe_dai_startup(struct
>snd_pcm_substream *fe_substream)
>>  	dpcm_set_fe_runtime(fe_substream);
>>  	snd_pcm_limit_hw_rates(runtime);
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>  	return 0;
>>
>>  unwind:
>>  	dpcm_be_dai_startup_unwind(fe, fe_substream->stream);
>>  be_err:
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>  	return ret;
>>  }
>>
>> @@ -1603,7 +1626,7 @@ static int dpcm_fe_dai_shutdown(struct
>snd_pcm_substream *substream)
>>  	struct snd_soc_pcm_runtime *fe = substream->private_data;
>>  	int stream = substream->stream;
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>>
>>  	/* shutdown the BEs */
>>  	dpcm_be_dai_shutdown(fe, substream->stream); @@ -1617,7 +1640,7 @@
>> static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
>>  	dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
>>
>>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>  	return 0;
>>  }
>>
>> @@ -1665,7 +1688,7 @@ static int dpcm_fe_dai_hw_free(struct
>snd_pcm_substream *substream)
>>  	int err, stream = substream->stream;
>>
>>  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>>
>>  	dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
>>
>> @@ -1680,7 +1703,7 @@ static int dpcm_fe_dai_hw_free(struct
>snd_pcm_substream *substream)
>>  	err = dpcm_be_dai_hw_free(fe, stream);
>>
>>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>
>>  	mutex_unlock(&fe->card->mutex);
>>  	return 0;
>> @@ -1773,7 +1796,7 @@ static int dpcm_fe_dai_hw_params(struct
>snd_pcm_substream *substream,
>>  	int ret, stream = substream->stream;
>>
>>  	mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>>
>>  	memcpy(&fe->dpcm[substream->stream].hw_params, params,
>>  			sizeof(struct snd_pcm_hw_params)); @@ -1796,7 +1819,7
>@@ static
>> int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>>  		fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
>>
>>  out:
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>  	mutex_unlock(&fe->card->mutex);
>>  	return ret;
>>  }
>> @@ -1910,14 +1933,12 @@ int dpcm_be_dai_trigger(struct
>> snd_soc_pcm_runtime *fe, int stream,  }
>> EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
>>
>> -static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream,
>> int cmd)
>> +static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream
>> +*substream, int cmd)
>>  {
>>  	struct snd_soc_pcm_runtime *fe = substream->private_data;
>>  	int stream = substream->stream, ret;
>>  	enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> -
>>  	switch (trigger) {
>>  	case SND_SOC_DPCM_TRIGGER_PRE:
>>  		/* call trigger on the frontend before the backend. */ @@ -
>1928,7
>> +1949,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream
>*substream, int cmd)
>>  		ret = soc_pcm_trigger(substream, cmd);
>>  		if (ret < 0) {
>>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
>> -			goto out;
>> +			return ret;
>>  		}
>>
>>  		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd); @@ -
>1939,7
>> +1960,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream
>*substream, int cmd)
>>  		ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
>>  		if (ret < 0) {
>>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
>> -			goto out;
>> +			return ret;
>>  		}
>>
>>  		dev_dbg(fe->dev, "ASoC: post trigger FE %s cmd %d\n", @@ -
>1956,14
>> +1977,13 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream
>*substream, int cmd)
>>  		ret = soc_pcm_bespoke_trigger(substream, cmd);
>>  		if (ret < 0) {
>>  			dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
>> -			goto out;
>> +			return ret;
>>  		}
>>  		break;
>>  	default:
>>  		dev_err(fe->dev, "ASoC: invalid trigger cmd %d for %s\n",
>cmd,
>>  				fe->dai_link->name);
>> -		ret = -EINVAL;
>> -		goto out;
>> +		return -EINVAL;
>>  	}
>>
>>  	switch (cmd) {
>> @@ -1979,7 +1999,25 @@ static int dpcm_fe_dai_trigger(struct
>snd_pcm_substream *substream, int cmd)
>>  		break;
>>  	}
>>
>> -out:
>> +	return 0;
>> +}
>> +
>> +static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream,
>> +int cmd) {
>> +	struct snd_soc_pcm_runtime *fe = substream->private_data;
>> +	int stream = substream->stream, ret;
>> +
>> +	/* if FE's runtime_update is already set, we're in race;
>> +	 * process this trigger later at exit
>> +	 */
>> +	if (fe->dpcm[stream].runtime_update != SND_SOC_DPCM_UPDATE_NO) {
>> +		fe->dpcm[stream].trigger_pending = cmd + 1;
>> +		return 0; /* delayed, assuming it's successful */
>> +	}
>> +
>> +	/* we're alone, let's trigger */
>> +	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	ret = dpcm_fe_dai_do_trigger(substream, cmd);
>>  	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>>  	return ret;
>>  }
>> @@ -2027,7 +2065,7 @@ static int dpcm_fe_dai_prepare(struct
>> snd_pcm_substream *substream)
>>
>>  	dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>>
>>  	/* there is no point preparing this FE if there are no BEs */
>>  	if (list_empty(&fe->dpcm[stream].be_clients)) { @@ -2054,7 +2092,7
>> @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>>  	fe->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE;
>>
>>  out:
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>  	mutex_unlock(&fe->card->mutex);
>>
>>  	return ret;
>> @@ -2201,11 +2239,11 @@ static int dpcm_run_new_update(struct
>> snd_soc_pcm_runtime *fe, int stream)  {
>>  	int ret;
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
>>  	ret = dpcm_run_update_startup(fe, stream);
>>  	if (ret < 0)
>>  		dev_err(fe->dev, "ASoC: failed to startup some BEs\n");
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>
>>  	return ret;
>>  }
>> @@ -2214,11 +2252,11 @@ static int dpcm_run_old_update(struct
>> snd_soc_pcm_runtime *fe, int stream)  {
>>  	int ret;
>>
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
>>  	ret = dpcm_run_update_shutdown(fe, stream);
>>  	if (ret < 0)
>>  		dev_err(fe->dev, "ASoC: failed to shutdown some BEs\n");
>> -	fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
>> +	dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>>
>>  	return ret;
>>  }
>
Takashi, Liam

We have verified again and This patch fixes the race issue. Thanks a 
lot for your patch and suggestions.

Best Regards
Qiao
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel