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

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

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Driver should stop all working stream when .remove timing.
Current Renesas sound driver is assuming that all stream was
stopped when .remove but it was wrong.
This patch stops all working stream when .remove, otherwise
kernel will get damage for example in below case.
Special thanks to Truong, Hiep

	> cd /sys/bus/platform/drivers/rcar_sound
	> aplay xxx.wav &
	> echo ec500000.sound > unbind

Reported-by: Hiep Cao Minh <cm-hiep@jinso.co.jp>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/core.c | 68 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 1071332..1bf472f 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -197,12 +197,6 @@ void rsnd_mod_interrupt(struct rsnd_mod *mod,
 	}
 }
 
-int rsnd_io_is_working(struct rsnd_dai_stream *io)
-{
-	/* see rsnd_dai_stream_init/quit() */
-	return !!io->substream;
-}
-
 int rsnd_runtime_channel_original(struct rsnd_dai_stream *io)
 {
 	struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io);
@@ -610,20 +604,24 @@ struct rsnd_dai_stream *rsnd_rdai_to_io(struct rsnd_dai *rdai,
 		return &rdai->capture;
 }
 
-static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
-			    struct snd_soc_dai *dai)
+int rsnd_io_is_working(struct rsnd_dai_stream *io)
+{
+	/* see rsnd_dai_stream_init/quit() */
+	return !!io->substream;
+}
+
+#define rsnd_io_start(priv, io, sub)	rsnd_io_operation(priv, io, sub)
+#define rsnd_io_stop(priv, io)		rsnd_io_operation(priv, io, NULL)
+static int rsnd_io_operation(struct rsnd_priv *priv,
+			     struct rsnd_dai_stream *io,
+			     struct snd_pcm_substream *substream)
 {
-	struct rsnd_priv *priv = rsnd_dai_to_priv(dai);
-	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
-	struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
 	int ret;
 	unsigned long flags;
 
 	spin_lock_irqsave(&priv->lock, flags);
 
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
+	if (substream) {
 		rsnd_dai_stream_init(io, substream);
 
 		ret = rsnd_dai_call(init, io, priv);
@@ -638,9 +636,7 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 		if (ret < 0)
 			goto dai_trigger_end;
 
-		break;
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
+	} else {
 		ret = rsnd_dai_call(irq, io, priv, 0);
 
 		ret |= rsnd_dai_call(stop, io, priv);
@@ -648,9 +644,6 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 		ret |= rsnd_dai_call(quit, io, priv);
 
 		rsnd_dai_stream_quit(io);
-		break;
-	default:
-		ret = -EINVAL;
 	}
 
 dai_trigger_end:
@@ -659,6 +652,30 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 	return ret;
 }
 
+static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
+			    struct snd_soc_dai *dai)
+{
+	struct rsnd_priv *priv = rsnd_dai_to_priv(dai);
+	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
+	struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
+	int ret;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		ret = rsnd_io_start(priv, io, substream);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+		ret = rsnd_io_stop(priv, io);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int rsnd_soc_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
@@ -1477,6 +1494,17 @@ static int rsnd_remove(struct platform_device *pdev)
 	};
 	int ret = 0, i;
 
+	/*
+	 * Stop all working io
+	 */
+	for_each_rsnd_dai(rdai, priv, i) {
+		if (rsnd_io_is_working(&rdai->playback))
+			rsnd_io_stop(priv, &rdai->playback);
+
+		if (rsnd_io_is_working(&rdai->capture))
+			rsnd_io_stop(priv, &rdai->capture);
+	}
+
 	pm_runtime_disable(&pdev->dev);
 
 	for_each_rsnd_dai(rdai, priv, i) {
-- 
1.9.1

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

[Takashi Iwai]

On Fri, 01 Sep 2017 06:34:48 +0200,
Kuninori Morimoto wrote:
> 
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Driver should stop all working stream when .remove timing.
> Current Renesas sound driver is assuming that all stream was
> stopped when .remove but it was wrong.
> This patch stops all working stream when .remove, otherwise
> kernel will get damage for example in below case.
> Special thanks to Truong, Hiep
> 
> 	> cd /sys/bus/platform/drivers/rcar_sound
> 	> aplay xxx.wav &
> 	> echo ec500000.sound > unbind
> 
> Reported-by: Hiep Cao Minh <cm-hiep@jinso.co.jp>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

The lack of stop sync is a known problem in the ALSA PCM
infrastructure.  The standard idiom is to do sync at both prepare and
hw_free (or close) callbacks.


Takashi

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

[Kuninori Morimoto]

Hi Takashi

> > Driver should stop all working stream when .remove timing.
> > Current Renesas sound driver is assuming that all stream was
> > stopped when .remove but it was wrong.
> > This patch stops all working stream when .remove, otherwise
> > kernel will get damage for example in below case.
> > Special thanks to Truong, Hiep
> > 
> > 	> cd /sys/bus/platform/drivers/rcar_sound
> > 	> aplay xxx.wav &
> > 	> echo ec500000.sound > unbind
> > 
> > Reported-by: Hiep Cao Minh <cm-hiep@jinso.co.jp>
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> The lack of stop sync is a known problem in the ALSA PCM
> infrastructure.  The standard idiom is to do sync at both prepare and
> hw_free (or close) callbacks.

Thanks.
This path main sync is for clk ON/OFF

Best regards
---
Kuninori Morimoto

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

[Takashi Iwai]

On Fri, 01 Sep 2017 09:48:52 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi
> 
> > > Driver should stop all working stream when .remove timing.
> > > Current Renesas sound driver is assuming that all stream was
> > > stopped when .remove but it was wrong.
> > > This patch stops all working stream when .remove, otherwise
> > > kernel will get damage for example in below case.
> > > Special thanks to Truong, Hiep
> > > 
> > > 	> cd /sys/bus/platform/drivers/rcar_sound
> > > 	> aplay xxx.wav &
> > > 	> echo ec500000.sound > unbind
> > > 
> > > Reported-by: Hiep Cao Minh <cm-hiep@jinso.co.jp>
> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > The lack of stop sync is a known problem in the ALSA PCM
> > infrastructure.  The standard idiom is to do sync at both prepare and
> > hw_free (or close) callbacks.
> 
> Thanks.
> This path main sync is for clk ON/OFF

Hm, but it's managed as PCM trigger, no?
How can the rsnd_io_is_working() return true after PCM streams are
stopped?


Takashi

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

[Kuninori Morimoto]

Hi Takasi-san

> > The lack of stop sync is a known problem in the ALSA PCM 
> > infrastructure.  The standard idiom is to do sync at both prepare 
> > and hw_free (or close) callbacks.
> 
> Thanks.
> This path main sync is for clk ON/OFF
>
> Hm, but it's managed as PCM trigger, no?
> How can the rsnd_io_is_working() return true after PCM streams are stopped?

It is based on PCM trigger, thus, it returns false if PCM streams are stopped.

This driver calls clk_get() when PCM started, and clk_put() when stopped.
And it calles clk_enable() on .probe, and clk_disable() on .remove.

My problem is that user unbinds driver during Sound playing, this means clk_get() is called, but clk_put() is not called. Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed.
Is this clear answer for you ?

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

[Takashi Iwai]

On Mon, 04 Sep 2017 19:44:36 +0200,
Kuninori Morimoto wrote:
> 
> Hi Takasi-san
> 
> > > The lack of stop sync is a known problem in the ALSA PCM 
> > > infrastructure.  The standard idiom is to do sync at both prepare 
> > > and hw_free (or close) callbacks.
> > 
> > Thanks.
> > This path main sync is for clk ON/OFF
> >
> > Hm, but it's managed as PCM trigger, no?
> > How can the rsnd_io_is_working() return true after PCM streams are stopped?
> 
> It is based on PCM trigger, thus, it returns false if PCM streams are stopped.
> 
> This driver calls clk_get() when PCM started, and clk_put() when stopped.
> And it calles clk_enable() on .probe, and clk_disable() on .remove.
> 
> My problem is that user unbinds driver during Sound playing, this
> means clk_get() is called, but clk_put() is not called.
>
>Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed.
> Is this clear answer for you ?

This isn't something you shouldn't fiddle with the codec layer.
If the driver gets removed during the operation, you have to cancel
the operation and sync with it in a proper way, then proceed the rest
of the remove, not only a codec-specific resource management.

Admittedly, there is no common infrastructure for that.  But it
doesn't mean that each codec driver should do its own hack.

I can imagine a way like calling the card disconnect/free at codec
remove, so that it can sync with the whole stop operation before doing
the rest.  This should be ignored when the code path is from the card
removal -- e.g. checking card->shutdown flag.


thanks,

Takashi

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

[Takashi Iwai]

On Mon, 04 Sep 2017 20:43:19 +0200,
Takashi Iwai wrote:
> 
> On Mon, 04 Sep 2017 19:44:36 +0200,
> Kuninori Morimoto wrote:
> > 
> > Hi Takasi-san
> > 
> > > > The lack of stop sync is a known problem in the ALSA PCM 
> > > > infrastructure.  The standard idiom is to do sync at both prepare 
> > > > and hw_free (or close) callbacks.
> > > 
> > > Thanks.
> > > This path main sync is for clk ON/OFF
> > >
> > > Hm, but it's managed as PCM trigger, no?
> > > How can the rsnd_io_is_working() return true after PCM streams are stopped?
> > 
> > It is based on PCM trigger, thus, it returns false if PCM streams are stopped.
> > 
> > This driver calls clk_get() when PCM started, and clk_put() when stopped.
> > And it calles clk_enable() on .probe, and clk_disable() on .remove.
> > 
> > My problem is that user unbinds driver during Sound playing, this
> > means clk_get() is called, but clk_put() is not called.
> >
> >Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed.
> > Is this clear answer for you ?
> 
> This isn't something you shouldn't fiddle with the codec layer.
> If the driver gets removed during the operation, you have to cancel
> the operation and sync with it in a proper way, then proceed the rest
> of the remove, not only a codec-specific resource management.
> 
> Admittedly, there is no common infrastructure for that.  But it
> doesn't mean that each codec driver should do its own hack.
> 
> I can imagine a way like calling the card disconnect/free at codec
> remove, so that it can sync with the whole stop operation before doing
> the rest.  This should be ignored when the code path is from the card
> removal -- e.g. checking card->shutdown flag.

Here I mentioned the codec driver, but it's applied to each lower-level
component.  It'd need some graceful way to communicate with the
top-level card to assure the removal of the component.


Takashi

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

[Kuninori Morimoto]

Hi Takashi-san

Thank you for your feedback

> This isn't something you shouldn't fiddle with the codec layer.
> If the driver gets removed during the operation, you have to cancel 
> the operation and sync with it in a proper way, then proceed the rest 
> of the remove, not only a codec-specific resource management.
(snip)
> Here I mentioned the codec driver, but it's applied to each lower-level
> component.  It'd need some graceful way to communicate with the
> top-level card to assure the removal of the component.

I agree.
I can't access to source code now (I'm in business-trip), but my head-acke is that kernel doesn't check return value from .remove when unbind case.
Thus, we can't "cancel" remove operation.
I'm happy if you can confirm it.

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

[Takashi Iwai]

On Tue, 05 Sep 2017 09:40:11 +0200,
Kuninori Morimoto wrote:
> 
> Hi Takashi-san
> 
> Thank you for your feedback
> 
> > This isn't something you shouldn't fiddle with the codec layer.
> > If the driver gets removed during the operation, you have to cancel 
> > the operation and sync with it in a proper way, then proceed the rest 
> > of the remove, not only a codec-specific resource management.
> (snip)
> > Here I mentioned the codec driver, but it's applied to each lower-level
> > component.  It'd need some graceful way to communicate with the
> > top-level card to assure the removal of the component.
> 
> I agree.
> I can't access to source code now (I'm in business-trip), but my head-acke is that kernel doesn't check return value from .remove when unbind case.
> Thus, we can't "cancel" remove operation.
> I'm happy if you can confirm it.

Right, you can't cancel or return an error at that point.  That is,
you'd need to sync (wait) until the all top-level operations are
canceled at remove callback.

For example, snd_card_free() processes the disconnection procedure at
first, then waits for the completion.  That's how the hot-unplug works
safely.  It's implemented, at least, in the top-level driver removal.

Now for the lower level driver, you'd need a similar strategy; notify
to the toplevel for hot-unplug (disconnect in ALSA), and sync with the
stop operation, then continue the rest of its own remove procedure.


thanks,

Takashi

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

[Kuninori Morimoto]

Hi Takashi-san

Thank you for your feedback

> Right, you can't cancel or return an error at that point.
> That is, you'd need to sync (wait) until the all top-level operations are canceled at remove callback.
>
> For example, snd_card_free() processes the disconnection procedure at first,
> then waits for the completion.  That's how the hot-unplug works safely.
> It's implemented, at least, in the top-level driver removal.
>
> Now for the lower level driver, you'd need a similar strategy; notify to the toplevel for
> hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.

OK, it needs ALSA SoC framework side new feature.
But can I confirm current situation ?

In ALSA SoC, it has Card/CPU/Codec/Platform drivers, and we can unbind these randomly.
Now, if I unbind CPU first, it checks connected Card, and will disconnect it if needed (Then, other drivers are as-is).
Because of this, Card will be disconnected automatically, and we can't use it again if user didn't remove all other
remaining drivers and re-bind all drivers again. This is current ALSA SoC I think.

If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
And then, Card want to wait all drivers are removed. Correct ?

I'm happy to work for it.
But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
Can we separate these ?

Morimoto

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

[Takashi Iwai]

On Tue, 05 Sep 2017 10:58:37 +0200,
Kuninori Morimoto wrote:
> 
> Hi Takashi-san
> 
> Thank you for your feedback
> 
> > Right, you can't cancel or return an error at that point.
> > That is, you'd need to sync (wait) until the all top-level operations are canceled at remove callback.
> >
> > For example, snd_card_free() processes the disconnection procedure at first,
> > then waits for the completion.  That's how the hot-unplug works safely.
> > It's implemented, at least, in the top-level driver removal.
> >
> > Now for the lower level driver, you'd need a similar strategy; notify to the toplevel for
> > hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
> 
> OK, it needs ALSA SoC framework side new feature.

Not only ASoC but also in all ALSA component generally.
The component-level hot unplug isn't implemented yet properly.

> But can I confirm current situation ?
> 
> In ALSA SoC, it has Card/CPU/Codec/Platform drivers, and we can unbind these randomly.
> Now, if I unbind CPU first, it checks connected Card, and will disconnect it if needed (Then, other drivers are as-is).
> Because of this, Card will be disconnected automatically, and we can't use it again if user didn't remove all other
> remaining drivers and re-bind all drivers again. This is current ALSA SoC I think.
> 
> If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
> And then, Card want to wait all drivers are removed. Correct ?

Right.  Unless we really want to support the hog-plug/unplug of each
component, it'd be more straightforward to do the full hot-unplug upon
every component unbind action.

> I'm happy to work for it.
> But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
> Can we separate these ?

It belongs with the same thing.  Basically you're tweaking clk per PCM
stream status.  By handling the full hot-plug properly, the PCM stream
is assured to be stopped, thus you don't have to fiddle with clk in
the remove callback at all.


Takashi

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

[Kuninori Morimoto]

Hi Takashi-san

> I'm happy to work for it.
> But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
> Can we separate these ?
>
> It belongs with the same thing. Basically you're tweaking clk per PCM stream status.
> By handling the full hot-plug properly, the PCM stream is assured to be stopped,
> thus you don't have to fiddle with clk in the remove callback at all.

Oh, yes, indeed.
Thanks.

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 622 bytes --]

On Tue, Sep 05, 2017 at 11:33:42AM +0200, Takashi Iwai wrote:
> Kuninori Morimoto wrote:

> > If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
> > And then, Card want to wait all drivers are removed. Correct ?

> Right.  Unless we really want to support the hog-plug/unplug of each
> component, it'd be more straightforward to do the full hot-unplug upon
> every component unbind action.

As far as the individual ASoC drivers are concerned this should just be
something that happens at component deregistration time like the DAPM
power down and so on.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

On Tue, 05 Sep 2017 11:33:42 +0200,
Takashi Iwai wrote:
> 
> On Tue, 05 Sep 2017 10:58:37 +0200,
> Kuninori Morimoto wrote:
> > 
> > Hi Takashi-san
> > 
> > Thank you for your feedback
> > 
> > > Right, you can't cancel or return an error at that point.
> > > That is, you'd need to sync (wait) until the all top-level operations are canceled at remove callback.
> > >
> > > For example, snd_card_free() processes the disconnection procedure at first,
> > > then waits for the completion.  That's how the hot-unplug works safely.
> > > It's implemented, at least, in the top-level driver removal.
> > >
> > > Now for the lower level driver, you'd need a similar strategy; notify to the toplevel for
> > > hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
> > 
> > OK, it needs ALSA SoC framework side new feature.
> 
> Not only ASoC but also in all ALSA component generally.
> The component-level hot unplug isn't implemented yet properly.
> 
> > But can I confirm current situation ?
> > 
> > In ALSA SoC, it has Card/CPU/Codec/Platform drivers, and we can unbind these randomly.
> > Now, if I unbind CPU first, it checks connected Card, and will disconnect it if needed (Then, other drivers are as-is).
> > Because of this, Card will be disconnected automatically, and we can't use it again if user didn't remove all other
> > remaining drivers and re-bind all drivers again. This is current ALSA SoC I think.
> > 
> > If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
> > And then, Card want to wait all drivers are removed. Correct ?
> 
> Right.  Unless we really want to support the hog-plug/unplug of each
> component, it'd be more straightforward to do the full hot-unplug upon
> every component unbind action.
> 
> > I'm happy to work for it.
> > But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
> > Can we separate these ?
> 
> It belongs with the same thing.  Basically you're tweaking clk per PCM
> stream status.  By handling the full hot-plug properly, the PCM stream
> is assured to be stopped, thus you don't have to fiddle with clk in
> the remove callback at all.

So my idea is something like below (totally untested): call
snd_card_disconnect_sync() at the remove call of each component at the
beginning.
That assures stopping all pending operations and syncs with the file
releases.  For ASoC, we may want to wrap it with ASoC structs, but you
can have an idea by this patch.


Takashi

---
diff --git a/include/sound/core.h b/include/sound/core.h
index 4104a9d1001f..5f181b875c2f 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -133,6 +133,7 @@ struct snd_card {
 	struct device card_dev;		/* cardX object for sysfs */
 	const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
 	bool registered;		/* card_dev is registered? */
+	wait_queue_head_t remove_sleep;
 
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
@@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		 struct snd_card **card_ret);
 
 int snd_card_disconnect(struct snd_card *card);
+void snd_card_disconnect_sync(struct snd_card *card);
 int snd_card_free(struct snd_card *card);
 int snd_card_free_when_closed(struct snd_card *card);
 void snd_card_set_id(struct snd_card *card, const char *id);
diff --git a/sound/core/init.c b/sound/core/init.c
index 32ebe2f6bc59..d8b556911d0e 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 #ifdef CONFIG_PM
 	init_waitqueue_head(&card->power_sleep);
 #endif
+	init_waitqueue_head(&card->remove_sleep);
 
 	device_initialize(&card->card_dev);
 	card->card_dev.parent = parent;
@@ -452,6 +453,15 @@ int snd_card_disconnect(struct snd_card *card)
 }
 EXPORT_SYMBOL(snd_card_disconnect);
 
+void snd_card_disconnect_sync(struct snd_card *card)
+{
+	snd_card_disconnect(card);
+	wait_event_lock_irq(card->remove_sleep,
+			    list_empty(&card->files_list),
+			    card->files_lock);
+}
+EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
+
 static int snd_card_do_free(struct snd_card *card)
 {
 #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
@@ -957,6 +967,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file)
 			break;
 		}
 	}
+	if (list_empty(&card->files_list))
+		wake_up_all(&card->remove_sleep);
 	spin_unlock(&card->files_lock);
 	if (!found) {
 		dev_err(card->dev, "card file remove problem (%p)\n", file);

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

[Takashi Iwai]

On Tue, 05 Sep 2017 13:35:58 +0200,
Takashi Iwai wrote:
> 
> On Tue, 05 Sep 2017 11:33:42 +0200,
> Takashi Iwai wrote:
> > 
> > On Tue, 05 Sep 2017 10:58:37 +0200,
> > Kuninori Morimoto wrote:
> > > 
> > > Hi Takashi-san
> > > 
> > > Thank you for your feedback
> > > 
> > > > Right, you can't cancel or return an error at that point.
> > > > That is, you'd need to sync (wait) until the all top-level operations are canceled at remove callback.
> > > >
> > > > For example, snd_card_free() processes the disconnection procedure at first,
> > > > then waits for the completion.  That's how the hot-unplug works safely.
> > > > It's implemented, at least, in the top-level driver removal.
> > > >
> > > > Now for the lower level driver, you'd need a similar strategy; notify to the toplevel for
> > > > hot-unplug (disconnect in ALSA), and sync with the stop operation, then continue the rest of its own remove procedure.
> > > 
> > > OK, it needs ALSA SoC framework side new feature.
> > 
> > Not only ASoC but also in all ALSA component generally.
> > The component-level hot unplug isn't implemented yet properly.
> > 
> > > But can I confirm current situation ?
> > > 
> > > In ALSA SoC, it has Card/CPU/Codec/Platform drivers, and we can unbind these randomly.
> > > Now, if I unbind CPU first, it checks connected Card, and will disconnect it if needed (Then, other drivers are as-is).
> > > Because of this, Card will be disconnected automatically, and we can't use it again if user didn't remove all other
> > > remaining drivers and re-bind all drivers again. This is current ALSA SoC I think.
> > > 
> > > If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
> > > And then, Card want to wait all drivers are removed. Correct ?
> > 
> > Right.  Unless we really want to support the hog-plug/unplug of each
> > component, it'd be more straightforward to do the full hot-unplug upon
> > every component unbind action.
> > 
> > > I'm happy to work for it.
> > > But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
> > > Can we separate these ?
> > 
> > It belongs with the same thing.  Basically you're tweaking clk per PCM
> > stream status.  By handling the full hot-plug properly, the PCM stream
> > is assured to be stopped, thus you don't have to fiddle with clk in
> > the remove callback at all.
> 
> So my idea is something like below (totally untested): call
> snd_card_disconnect_sync() at the remove call of each component at the
> beginning.
> That assures stopping all pending operations and syncs with the file
> releases.  For ASoC, we may want to wrap it with ASoC structs, but you
> can have an idea by this patch.

An obvious spinlock was forgotten in one place, the revised patch
below.


Takashi

---
diff --git a/include/sound/core.h b/include/sound/core.h
index 4104a9d1001f..5f181b875c2f 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -133,6 +133,7 @@ struct snd_card {
 	struct device card_dev;		/* cardX object for sysfs */
 	const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
 	bool registered;		/* card_dev is registered? */
+	wait_queue_head_t remove_sleep;
 
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
@@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		 struct snd_card **card_ret);
 
 int snd_card_disconnect(struct snd_card *card);
+void snd_card_disconnect_sync(struct snd_card *card);
 int snd_card_free(struct snd_card *card);
 int snd_card_free_when_closed(struct snd_card *card);
 void snd_card_set_id(struct snd_card *card, const char *id);
diff --git a/sound/core/init.c b/sound/core/init.c
index 32ebe2f6bc59..5cde6cc0d867 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 #ifdef CONFIG_PM
 	init_waitqueue_head(&card->power_sleep);
 #endif
+	init_waitqueue_head(&card->remove_sleep);
 
 	device_initialize(&card->card_dev);
 	card->card_dev.parent = parent;
@@ -452,6 +453,17 @@ int snd_card_disconnect(struct snd_card *card)
 }
 EXPORT_SYMBOL(snd_card_disconnect);
 
+void snd_card_disconnect_sync(struct snd_card *card)
+{
+	snd_card_disconnect(card);
+	spin_lock_irq(&card->files_lock);
+	wait_event_lock_irq(card->remove_sleep,
+			    list_empty(&card->files_list),
+			    card->files_lock);
+	spin_unlock_irq(&card->files_lock);
+}
+EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
+
 static int snd_card_do_free(struct snd_card *card)
 {
 #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
@@ -957,6 +969,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file)
 			break;
 		}
 	}
+	if (list_empty(&card->files_list))
+		wake_up_all(&card->remove_sleep);
 	spin_unlock(&card->files_lock);
 	if (!found) {
 		dev_err(card->dev, "card file remove problem (%p)\n", file);

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 549 bytes --]

On Tue, Sep 05, 2017 at 01:35:58PM +0200, Takashi Iwai wrote:

> So my idea is something like below (totally untested): call
> snd_card_disconnect_sync() at the remove call of each component at the
> beginning.
> That assures stopping all pending operations and syncs with the file
> releases.  For ASoC, we may want to wrap it with ASoC structs, but you
> can have an idea by this patch.

I'd say that in ASoC we'd have the call to this in the core rather than
wrapping it, I'd expect the first thing the drivers do is to unregister
the component.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Takashi Iwai]

On Tue, 05 Sep 2017 16:04:00 +0200,
Mark Brown wrote:
> 
> On Tue, Sep 05, 2017 at 01:35:58PM +0200, Takashi Iwai wrote:
> 
> > So my idea is something like below (totally untested): call
> > snd_card_disconnect_sync() at the remove call of each component at the
> > beginning.
> > That assures stopping all pending operations and syncs with the file
> > releases.  For ASoC, we may want to wrap it with ASoC structs, but you
> > can have an idea by this patch.
> 
> I'd say that in ASoC we'd have the call to this in the core rather than
> wrapping it, I'd expect the first thing the drivers do is to unregister
> the component.

Yes, that makes sense.


Takashi

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

[Kuninori Morimoto]

Hi Takashi

This is reply for old email.
This thread talked about hot-unplug during playback.

	aplay xxx &
	echo xxx > /sys/bus/platform/drivers/xxx/unbind

We need to stop all components on framework level.
And you kindly created its prototype solution.

> ---
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 4104a9d1001f..5f181b875c2f 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -133,6 +133,7 @@ struct snd_card {
>  	struct device card_dev;		/* cardX object for sysfs */
>  	const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
>  	bool registered;		/* card_dev is registered? */
> +	wait_queue_head_t remove_sleep;
>  
>  #ifdef CONFIG_PM
>  	unsigned int power_state;	/* power state */
> @@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
>  		 struct snd_card **card_ret);
>  
>  int snd_card_disconnect(struct snd_card *card);
> +void snd_card_disconnect_sync(struct snd_card *card);
>  int snd_card_free(struct snd_card *card);
>  int snd_card_free_when_closed(struct snd_card *card);
>  void snd_card_set_id(struct snd_card *card, const char *id);
> diff --git a/sound/core/init.c b/sound/core/init.c
> index 32ebe2f6bc59..5cde6cc0d867 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
>  #ifdef CONFIG_PM
>  	init_waitqueue_head(&card->power_sleep);
>  #endif
> +	init_waitqueue_head(&card->remove_sleep);
>  
>  	device_initialize(&card->card_dev);
>  	card->card_dev.parent = parent;
> @@ -452,6 +453,17 @@ int snd_card_disconnect(struct snd_card *card)
>  }
>  EXPORT_SYMBOL(snd_card_disconnect);
>  
> +void snd_card_disconnect_sync(struct snd_card *card)
> +{
> +	snd_card_disconnect(card);
> +	spin_lock_irq(&card->files_lock);
> +	wait_event_lock_irq(card->remove_sleep,
> +			    list_empty(&card->files_list),
> +			    card->files_lock);
> +	spin_unlock_irq(&card->files_lock);
> +}
> +EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
> +
>  static int snd_card_do_free(struct snd_card *card)
>  {
>  #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
> @@ -957,6 +969,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file)
>  			break;
>  		}
>  	}
> +	if (list_empty(&card->files_list))
> +		wake_up_all(&card->remove_sleep);
>  	spin_unlock(&card->files_lock);
>  	if (!found) {
>  		dev_err(card->dev, "card file remove problem (%p)\n", file);


I tried this patch, and noticed 2 issues (at this point).
I think below thing happen on this process.

 1) snd_pcm_dev_disconnect() is called
  - status will be SNDRV_PCM_STATE_DISCONNECTED;

 2) snd_pcm_release() is called
  - snd_pcm_release_substream() is called
   - snd_pcm_drop() is called

snd_pcm_drop() will call snd_pcm_stop(), and
it stops each driver. This is needed.
The issue will be happen if 1) happen before 2) (I think)

1st issue is

snd_pcm_drop() has this check

	if (runtime->status->state == SNDRV_PCM_STATE_OPEN ||
	    runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED)
		return -EBADFD;

if 1) happen before 2), snd_pcm_drop() returns immediately,
and doesn't call snd_pcm_stop().

I modified this as quick-hack locally.
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);

Here, trigger isn't called, because snd_pcm_running() is 0.
This is because status is SNDRV_PCM_STATE_DISCONNECTED if 1) happen before 2).

I don't know detail of these status magic,
but do you have some idea/solution ?

Best regards
---
Kuninori Morimoto

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

[Kuninori Morimoto]

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
 - ASoC version of snd_card_disconnect_sync()
 - user driver (= rsnd) uses snd_soc_card_disconnect_sync()

---------------
diff --git a/include/sound/core.h b/include/sound/core.h
index 4104a9d..5f181b8 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -133,6 +133,7 @@ struct snd_card {
 	struct device card_dev;		/* cardX object for sysfs */
 	const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */
 	bool registered;		/* card_dev is registered? */
+	wait_queue_head_t remove_sleep;
 
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
@@ -240,6 +241,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 		 struct snd_card **card_ret);
 
 int snd_card_disconnect(struct snd_card *card);
+void snd_card_disconnect_sync(struct snd_card *card);
 int snd_card_free(struct snd_card *card);
 int snd_card_free_when_closed(struct snd_card *card);
 void snd_card_set_id(struct snd_card *card, const char *id);
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 4f05b0e..56d02f0 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -464,6 +464,8 @@ struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
 int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num);
 #endif
 
+void snd_soc_card_disconnect_sync(struct device *dev);
+
 struct snd_pcm_substream *snd_soc_get_dai_substream(struct snd_soc_card *card,
 		const char *dai_link, int stream);
 struct snd_soc_pcm_runtime *snd_soc_get_pcm_runtime(struct snd_soc_card *card,
diff --git a/sound/core/init.c b/sound/core/init.c
index 32ebe2f..f7f7050 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -255,6 +255,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 #ifdef CONFIG_PM
 	init_waitqueue_head(&card->power_sleep);
 #endif
+	init_waitqueue_head(&card->remove_sleep);
 
 	device_initialize(&card->card_dev);
 	card->card_dev.parent = parent;
@@ -452,6 +453,21 @@ int snd_card_disconnect(struct snd_card *card)
 }
 EXPORT_SYMBOL(snd_card_disconnect);
 
+void snd_card_disconnect_sync(struct snd_card *card)
+{
+	DECLARE_COMPLETION(comp);
+
+	if (snd_card_disconnect(card) < 0)
+		return;
+
+	spin_lock_irq(&card->files_lock);
+	wait_event_lock_irq(card->remove_sleep,
+			    list_empty(&card->files_list),
+			    card->files_lock);
+	spin_unlock_irq(&card->files_lock);
+}
+EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
+
 static int snd_card_do_free(struct snd_card *card)
 {
 #if IS_ENABLED(CONFIG_SND_MIXER_OSS)
@@ -957,6 +973,8 @@ int snd_card_file_remove(struct snd_card *card, struct file *file)
 			break;
 		}
 	}
+	if (list_empty(&card->files_list))
+		wake_up_all(&card->remove_sleep);
 	spin_unlock(&card->files_lock);
 	if (!found) {
 		dev_err(card->dev, "card file remove problem (%p)\n", file);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 2fec2fe..bc8124a 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1239,9 +1239,11 @@ static int snd_pcm_pre_stop(struct snd_pcm_substream *substream, int state)
 
 static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state)
 {
-	if (substream->runtime->trigger_master == substream &&
-	    snd_pcm_running(substream))
+	if ((substream->runtime->trigger_master == substream) &&
+	    (snd_pcm_running(substream) ||
+	     substream->runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED))
 		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
+
 	return 0; /* unconditonally stop all substreams */
 }
 
@@ -1882,8 +1884,7 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream)
 		return -ENXIO;
 	runtime = substream->runtime;
 
-	if (runtime->status->state == SNDRV_PCM_STATE_OPEN ||
-	    runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED)
+	if (runtime->status->state == SNDRV_PCM_STATE_OPEN)
 		return -EBADFD;
 
 	snd_pcm_stream_lock_irq(substream);
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 42366ce..31a6889 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1473,6 +1473,8 @@ static int rsnd_remove(struct platform_device *pdev)
 		ret |= rsnd_dai_call(remove, &rdai->capture, priv);
 	}
 
+	snd_soc_card_disconnect_sync(&pdev->dev);
+
 	for (i = 0; i < ARRAY_SIZE(remove_func); i++)
 		remove_func[i](priv);
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 0e2c34e..bdb91aa 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1136,6 +1136,16 @@ static int soc_init_dai_link(struct snd_soc_card *card,
 	return 0;
 }
 
+void snd_soc_card_disconnect_sync(struct device *dev)
+{
+	struct snd_soc_component *component = snd_soc_lookup_component(dev, NULL);
+
+	if (!component || !component->card)
+		return;
+
+	snd_card_disconnect_sync(component->card->snd_card);
+}
+
 /**
  * snd_soc_add_dai_link - Add a DAI link dynamically
  * @card: The ASoC card to which the DAI link is added
---------------

Best regards
---
Kuninori Morimoto

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

[Takashi Iwai]

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

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

[Kuninori Morimoto]

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

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

[Kuninori Morimoto]

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