Re: [PATCH] ASoC: rsnd: stop all working stream when .remove

Takashi Iwai
2017-10-06T13:19:34Z

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

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