ASoC: rsnd: stop all working stream when .remove

Commit Message

Takashi Iwai Oct. 6, 2017, 1:19 p.m. UTC

Morimoto-san,

sorry for the late reply.  It took time until I digest all pending
stuff after vacation.

On Wed, 27 Sep 2017 07:14:21 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi
> 
> > snd_pcm_drop() has this check
> > 
> > 	if (runtime->status->state == SNDRV_PCM_STATE_OPEN ||
> > 	    runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED)
> > 		return -EBADFD;
> (snip)
> > Then, 2nd issue happen on snd_pcm_stop(). it will call snd_pcm_do_stop(),
> > and it has
> > 
> > 	if (substream->runtime->trigger_master == substream &&
> > 	    snd_pcm_running(substream))
> > 		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
> 
> I used your patch, and modified above, then, hot-unplug
> during playback stops correctly without kernel panic.
> snd_pcm_drop() and snd_pcm_do_stop() care about
> SNDRV_PCM_STATE_DISCONNECTED on this patch.
> I think it means, "it should be stopped immediately
> if it was disconnected".
> But, I don't know this is OK or Not.
> 
> I added my local patch on this mail.
> Maybe we want to separate this patch into few small patches.
> but can you review this ?
> It is including
>  - your patch
>  - snd_pcm_stop() snd_pcm_do_stop() care DISCONNECTED

That needs a bit more investigation.
When the device is disconnected, not all drivers expect that further
PCM operations are done for non-existing devices.  We might need
either some flag to allow/prefer the stop-after-disconnection, or
rethink whether we should actually stop at snd_pcm_dev_disconnect()
like below.

>  - ASoC version of snd_card_disconnect_sync()
>  - user driver (= rsnd) uses snd_soc_card_disconnect_sync()

Yes, these should be split.


thanks,

Takashi

---

Comments

Kuninori Morimoto Oct. 10, 2017, 8 a.m. UTC | #1

Hi Takashi-san

> sorry for the late reply.  It took time until I digest all pending
> stuff after vacation.

no problem

> > I added my local patch on this mail.
> > Maybe we want to separate this patch into few small patches.
> > but can you review this ?
> > It is including
> >  - your patch
> >  - snd_pcm_stop() snd_pcm_do_stop() care DISCONNECTED
> 
> That needs a bit more investigation.
> When the device is disconnected, not all drivers expect that further
> PCM operations are done for non-existing devices.  We might need
> either some flag to allow/prefer the stop-after-disconnection, or
> rethink whether we should actually stop at snd_pcm_dev_disconnect()
> like below.

Thank you for below patch.
I will check/test it

> ---
> diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> index 7eadb7fd8074..054e47ad23ed 100644
> --- a/sound/core/pcm.c
> +++ b/sound/core/pcm.c
> @@ -1152,6 +1152,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
>  		for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) {
>  			snd_pcm_stream_lock_irq(substream);
>  			if (substream->runtime) {
> +				if (snd_pcm_running(substream))
> +					snd_pcm_stop(substream,
> +						     SNDRV_PCM_STATE_DISCONNECTED);
> +				/* to be sure, set the state unconditionally */
>  				substream->runtime->status->state = SNDRV_PCM_STATE_DISCONNECTED;
>  				wake_up(&substream->runtime->sleep);
>  				wake_up(&substream->runtime->tsleep);

Kuninori Morimoto Oct. 11, 2017, 6:52 a.m. UTC | #2

Hi Takashi-san, again

> > > I added my local patch on this mail.
> > > Maybe we want to separate this patch into few small patches.
> > > but can you review this ?
> > > It is including
> > >  - your patch
> > >  - snd_pcm_stop() snd_pcm_do_stop() care DISCONNECTED
> > 
> > That needs a bit more investigation.
> > When the device is disconnected, not all drivers expect that further
> > PCM operations are done for non-existing devices.  We might need
> > either some flag to allow/prefer the stop-after-disconnection, or
> > rethink whether we should actually stop at snd_pcm_dev_disconnect()
> > like below.
> 
> Thank you for below patch.
> I will check/test it

It solved my issue (without strange status magic :).
I posted this patch-set. [1/3] is your patch but I posted.
Please check (and fix) it

Best regards
---
Kuninori Morimoto

Patch

diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 7eadb7fd8074..054e47ad23ed 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -1152,6 +1152,10 @@  static int snd_pcm_dev_disconnect(struct snd_device *device)
 		for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) {
 			snd_pcm_stream_lock_irq(substream);
 			if (substream->runtime) {
+				if (snd_pcm_running(substream))
+					snd_pcm_stop(substream,
+						     SNDRV_PCM_STATE_DISCONNECTED);
+				/* to be sure, set the state unconditionally */
 				substream->runtime->status->state = SNDRV_PCM_STATE_DISCONNECTED;
 				wake_up(&substream->runtime->sleep);
 				wake_up(&substream->runtime->tsleep);