[PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

Hi,

here is a refreshed series containing ALSA core changes Morimoto-san
posted.  Basically the code is same but it's split, and more comments
and an error message are added.  I tested lightly with the existing
hot-pluggable device and no regression was seen.

The commits are found in topic/card-disconnect branch of sound git
tree, too.  Mark, please pull it to yours when applying the further
patches relying on this new helper function.


thanks,

Takashi

===

Takashi Iwai (2):
  ALSA: add snd_card_disconnect_sync()
  ALSA: pcm: Forcibly stop at disconnect callback

 include/sound/core.h |  2 ++
 sound/core/init.c    | 32 ++++++++++++++++++++++++++++++++
 sound/core/pcm.c     |  4 ++++
 3 files changed, 38 insertions(+)

-- 
2.14.2

[PATCH v2 1/2] ALSA: add snd_card_disconnect_sync()

[Takashi Iwai]

In case of user unbind ALSA driver during playing back / capturing,
each driver needs to stop and remove it correctly. One note here is
that we can't cancel from remove function in such case, because
unbind operation doesn't check return value from remove function.
So, we *must* stop and remove in this case.

For this purpose, we need to sync (= wait) until the all top-level
operations are canceled at remove function.
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, we 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.

This patch adds snd_card_disconnect_sync(), and driver can use it from
remove function.

Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  2 ++
 sound/core/init.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

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..168ae03d3a1c 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,35 @@ int snd_card_disconnect(struct snd_card *card)
 }
 EXPORT_SYMBOL(snd_card_disconnect);
 
+/**
+ * snd_card_disconnect_sync - disconnect card and wait until files get closed
+ * @card: card object to disconnect
+ *
+ * This calls snd_card_disconnect() for disconnecting all belonging components
+ * and waits until all pending files get closed.
+ * It assures that all accesses from user-space finished so that the driver
+ * can release its resources gracefully.
+ */
+void snd_card_disconnect_sync(struct snd_card *card)
+{
+	int err;
+
+	err = snd_card_disconnect(card);
+	if (err < 0) {
+		dev_err(card->dev,
+			"snd_card_disconnect error (%d), skipping sync\n",
+			err);
+		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 +987,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);
-- 
2.14.2

Re: [PATCH v2 1/2] ALSA: add snd_card_disconnect_sync()

[Takashi Sakamoto]

Hi,

On Oct 11 2017 19:16, Takashi Iwai wrote:
> In case of user unbind ALSA driver during playing back / capturing,
> each driver needs to stop and remove it correctly. One note here is
> that we can't cancel from remove function in such case, because
> unbind operation doesn't check return value from remove function.
> So, we *must* stop and remove in this case.
> 
> For this purpose, we need to sync (= wait) until the all top-level
> operations are canceled at remove function.
> 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, we 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.
> 
> This patch adds snd_card_disconnect_sync(), and driver can use it from
> remove function.
> 
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   include/sound/core.h |  2 ++
>   sound/core/init.c    | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 34 insertions(+)

I have a question for this patch. If this patch is applied and the 
lower-level driver call this function in its .remove callback, after all 
references to an instance of snd_card by file instances corresponding to 
each of ALSA character devices are released, the lower-level driver must 
explicitly releases the last reference to the instance of snd_card. On 
ALSA SoC part, which context perform this important task? In my opinion, 
this patch easily allows leak of kobject. Do you have any safety?

At least, it's better for us to add enough comments to inform it to 
developers.


Regards

Takashi Sakamoto

Re: [PATCH v2 1/2] ALSA: add snd_card_disconnect_sync()

[Takashi Iwai]

On Wed, 11 Oct 2017 13:22:12 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Oct 11 2017 19:16, Takashi Iwai wrote:
> > In case of user unbind ALSA driver during playing back / capturing,
> > each driver needs to stop and remove it correctly. One note here is
> > that we can't cancel from remove function in such case, because
> > unbind operation doesn't check return value from remove function.
> > So, we *must* stop and remove in this case.
> >
> > For this purpose, we need to sync (= wait) until the all top-level
> > operations are canceled at remove function.
> > 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, we 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.
> >
> > This patch adds snd_card_disconnect_sync(), and driver can use it from
> > remove function.
> >
> > Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   include/sound/core.h |  2 ++
> >   sound/core/init.c    | 32 ++++++++++++++++++++++++++++++++
> >   2 files changed, 34 insertions(+)
> 
> I have a question for this patch. If this patch is applied and the
> lower-level driver call this function in its .remove callback, after
> all references to an instance of snd_card by file instances
> corresponding to each of ALSA character devices are released, the
> lower-level driver must explicitly releases the last reference to the
> instance of snd_card.

Not really.  This is only about files user-space opened.  It has
nothing to do with the refcount of objects / resources.  The refcount
sync is done by card->release_completion instead, which is sync'ed in
snd_card_free().

> On ALSA SoC part, which context perform this
> important task?

The "lower-level" might be confusing here; in the context, it's rather
meant as middle layer (component) that can be unbound freely.
Thus, for non-ASoC codes, this hasn't been a big problem, so far,
since the helper modules can't be unbound.
(Maybe the legacy AC97 codec driver needs something, and HD-audio has
 already some not-so-perfect workaround.)

> In my opinion, this patch easily allows leak of
> kobject. Do you have any safety?

There isn't anything different from the current state wrt kobjects.
The only merit of calling this in the remove callback is that it can
assure that the all actions are stopped, by disconnecting the devices.
It doesn't free resources by itself.

> At least, it's better for us to add enough comments to inform it to
> developers.

Well, I thought it well informed about the disconnection procedure.
Maybe the term "lower level driver" needs to be more explicit.


thanks,

Takashi

Re: [PATCH v2 1/2] ALSA: add snd_card_disconnect_sync()

[Takashi Iwai]

On Wed, 11 Oct 2017 14:19:52 +0200,
Takashi Iwai wrote:
> 
> > At least, it's better for us to add enough comments to inform it to
> > developers.
> 
> Well, I thought it well informed about the disconnection procedure.
> Maybe the term "lower level driver" needs to be more explicit.

I updated the patch description and added a couple of notes for
explaining your concerns.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: add snd_card_disconnect_sync()

In case of user unbind ALSA driver during playing back / capturing,
each driver needs to stop and remove it correctly. One note here is
that we can't cancel from remove function in such case, because
unbind operation doesn't check return value from remove function.
So, we *must* stop and remove in this case.

For this purpose, we need to sync (= wait) until the all top-level
operations are canceled at remove function.
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, we 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.

This patch adds snd_card_disconnect_sync(), and driver can use it from
remove function.

Note: the "lower level" driver here refers to a middle layer driver
(e.g. ASoC components) that can be unbound freely during operation.
Most of legacy ALSA helper drivers don't have such a problem because
they can't be unbound.

Note#2: snd_card_disconnect_sync() merely calls snd_card_disconnect()
and syncs with closing all pending files.  It takes only the files
opened by user-space into account, and doesn't care about object
refcounts.  (The latter is handled by snd_card_free() completion call,
BTW.)  Also, the function doesn't free resources by itself.

Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  2 ++
 sound/core/init.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

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..168ae03d3a1c 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,35 @@ int snd_card_disconnect(struct snd_card *card)
 }
 EXPORT_SYMBOL(snd_card_disconnect);
 
+/**
+ * snd_card_disconnect_sync - disconnect card and wait until files get closed
+ * @card: card object to disconnect
+ *
+ * This calls snd_card_disconnect() for disconnecting all belonging components
+ * and waits until all pending files get closed.
+ * It assures that all accesses from user-space finished so that the driver
+ * can release its resources gracefully.
+ */
+void snd_card_disconnect_sync(struct snd_card *card)
+{
+	int err;
+
+	err = snd_card_disconnect(card);
+	if (err < 0) {
+		dev_err(card->dev,
+			"snd_card_disconnect error (%d), skipping sync\n",
+			err);
+		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 +987,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);
-- 
2.14.2

[PATCH v2 2/2] ALSA: pcm: Forcibly stop at disconnect callback

[Takashi Iwai]

So far we assumed that each driver implements the hotplug PCM handling
properly, e.g. dealing with the pending PCM stream at disconnect
callback.  But most codes don't care, and it eventually leaves the PCM
stream inconsistent state when an abrupt disconnection like sysfs
unbind happens.

This patch is simple but a big-hammer solution: invoke snd_pcm_stop()
at the common PCM disconnect callback always when the stream is
running.

Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm.c | 4 ++++
 1 file changed, 4 insertions(+)

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

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi-san

> here is a refreshed series containing ALSA core changes Morimoto-san
> posted.  Basically the code is same but it's split, and more comments
> and an error message are added.  I tested lightly with the existing
> hot-pluggable device and no regression was seen.
> 
> The commits are found in topic/card-disconnect branch of sound git
> tree, too.  Mark, please pull it to yours when applying the further
> patches relying on this new helper function.

I tested this patch-set.
I noticed that it doesn't work if I used DPCM
(Kernel has Oops).
I will investigate it next week

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Fri, 13 Oct 2017 09:39:00 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san
> 
> > here is a refreshed series containing ALSA core changes Morimoto-san
> > posted.  Basically the code is same but it's split, and more comments
> > and an error message are added.  I tested lightly with the existing
> > hot-pluggable device and no regression was seen.
> > 
> > The commits are found in topic/card-disconnect branch of sound git
> > tree, too.  Mark, please pull it to yours when applying the further
> > patches relying on this new helper function.
> 
> I tested this patch-set.
> I noticed that it doesn't work if I used DPCM
> (Kernel has Oops).
> I will investigate it next week

Could you show the Oops message if you have?
My concern is whether it happens by stopping at disconnect, or it's
just another missing piece.


thanks,

Takashi

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi

> > I tested this patch-set.
> > I noticed that it doesn't work if I used DPCM
> > (Kernel has Oops).
> > I will investigate it next week
> 
> Could you show the Oops message if you have?
> My concern is whether it happens by stopping at disconnect, or it's
> just another missing piece.

OK, but kernel log doesn't help you.
see below

My environment now is I'm using DPCM.
	FE : rsnd
	FE : rsnd
	BE : ak4613

1st issue is that kernel need below patch.
I guess BE is using dummy driver, and it doesn't have ops(?).
If this is needed, I can post it.

-----------------------
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 2fec2fe..972408b 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1241,7 +1241,8 @@ static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state)
 {
 	if (substream->runtime->trigger_master == substream &&
 	    snd_pcm_running(substream))
-		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
+		if (substream->ops && substream->ops->trigger)
+			substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
 	return 0; /* unconditonally stop all substreams */
 }
-----------------------
 
After this patch, my driver side clock counter mismatch Oops happen.
It seems this is because FE side wasn't called snd_pcm_stop();
(= BE side only called snd_pcm_stop()).

I could confirm this by printing who's stop was called by local quick hack.

Maybe timing reason, if kernel has Oops for some reasons,
then, both BE/FE snd_pcm_stop() are called.
If no Oops, BE snd_pcm_stop() only called.

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Fri, 13 Oct 2017 11:11:42 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi
> 
> > > I tested this patch-set.
> > > I noticed that it doesn't work if I used DPCM
> > > (Kernel has Oops).
> > > I will investigate it next week
> > 
> > Could you show the Oops message if you have?
> > My concern is whether it happens by stopping at disconnect, or it's
> > just another missing piece.
> 
> OK, but kernel log doesn't help you.
> see below
> 
> My environment now is I'm using DPCM.
> 	FE : rsnd
> 	FE : rsnd
> 	BE : ak4613
> 
> 1st issue is that kernel need below patch.
> I guess BE is using dummy driver, and it doesn't have ops(?).
> If this is needed, I can post it.

No, this can't be right.  Every PCM implementation mandates the
presence of a trigger callback.  It's a must.  If a fix needed, it has
to be fixed in the driver side.

> -----------------------
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 2fec2fe..972408b 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1241,7 +1241,8 @@ static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state)
>  {
>  	if (substream->runtime->trigger_master == substream &&
>  	    snd_pcm_running(substream))
> -		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
> +		if (substream->ops && substream->ops->trigger)
> +			substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
>  	return 0; /* unconditonally stop all substreams */
>  }
> -----------------------
> After this patch, my driver side clock counter mismatch Oops happen.
> It seems this is because FE side wasn't called snd_pcm_stop();
> (= BE side only called snd_pcm_stop()).

So the check might work around an Oops, but it's no "fix", per se.

> I could confirm this by printing who's stop was called by local quick hack.
> 
> Maybe timing reason, if kernel has Oops for some reasons,
> then, both BE/FE snd_pcm_stop() are called.
> If no Oops, BE snd_pcm_stop() only called.

Any pending delayed work (like rtd->delayed_work)?
This is flushed at soc_cleanup_card_resources(), but it's called at
card removal, thus it's too late for the hot-removal of the
component.


thanks,

Takashi

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Mark Brown]

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

On Fri, Oct 13, 2017 at 11:43:59AM +0200, Takashi Iwai wrote:
> Kuninori Morimoto wrote:

> > My environment now is I'm using DPCM.
> > 	FE : rsnd
> > 	FE : rsnd
> > 	BE : ak4613

> > 1st issue is that kernel need below patch.
> > I guess BE is using dummy driver, and it doesn't have ops(?).
> > If this is needed, I can post it.

> No, this can't be right.  Every PCM implementation mandates the
> presence of a trigger callback.  It's a must.  If a fix needed, it has
> to be fixed in the driver side.

I suspect this is a DPCM framework thing - when the paths are joined up
at runtime there's going to be at least one trigger in there but with
DPCM it's most likely going to be in the front end, the back end DMA is
often going to be transparent to the system and so not have a trigger.
However if you just iterate over all the PCMs you'll see the decomposed
front and back end links with their dummies in there.  Not sure what the
fix is here, probably we need to be hiding the back end links more from
the ALSA core.

Liam?

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

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

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi

> > I could confirm this by printing who's stop was called by local quick hack.
> > 
> > Maybe timing reason, if kernel has Oops for some reasons,
> > then, both BE/FE snd_pcm_stop() are called.
> > If no Oops, BE snd_pcm_stop() only called.
> 
> Any pending delayed work (like rtd->delayed_work)?
> This is flushed at soc_cleanup_card_resources(), but it's called at
> card removal, thus it's too late for the hot-removal of the
> component.

Current my dirver used delayed_work, but we can control it
by snd_soc_runtime_ignore_pmdown_time();
I think my driver now doesn't use delayed_work.

But there is still issue.
I don't know detail, but it seems 
snd_pcm_dev_disconnect() and snd_pcm_relase() are called
it the same time, both are calling snd_pcm_stop().

Then, snd_pcm_relase() side will calls
snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.

Mark's suggestion (= hiding BE) can solve this ?

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Mon, 16 Oct 2017 04:26:42 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi
> 
> > > I could confirm this by printing who's stop was called by local quick hack.
> > > 
> > > Maybe timing reason, if kernel has Oops for some reasons,
> > > then, both BE/FE snd_pcm_stop() are called.
> > > If no Oops, BE snd_pcm_stop() only called.
> > 
> > Any pending delayed work (like rtd->delayed_work)?
> > This is flushed at soc_cleanup_card_resources(), but it's called at
> > card removal, thus it's too late for the hot-removal of the
> > component.
> 
> Current my dirver used delayed_work, but we can control it
> by snd_soc_runtime_ignore_pmdown_time();
> I think my driver now doesn't use delayed_work.
> 
> But there is still issue.
> I don't know detail, but it seems 
> snd_pcm_dev_disconnect() and snd_pcm_relase() are called
> it the same time, both are calling snd_pcm_stop().
> 
> Then, snd_pcm_relase() side will calls
> snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.

This must be also specific to DPCM.  Something is really wrong there.

Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race
since both are protected via pcm->open_mutex.  snd_pcm_stop() calls
are protected even more with substream lock.

> Mark's suggestion (= hiding BE) can solve this ?

Some of the issues might be addressed, yes, but I'm skeptical whether
it covers all.  The BE needs proper locking and refcounting that is
coupled with the FE, I suppose.


Takashi

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi-san, Mark

Thank you for your feedback

> > Then, snd_pcm_relase() side will calls
> > snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
> 
> This must be also specific to DPCM.  Something is really wrong there.
> 
> Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race
> since both are protected via pcm->open_mutex.  snd_pcm_stop() calls
> are protected even more with substream lock.
> 
> > Mark's suggestion (= hiding BE) can solve this ?
> 
> Some of the issues might be addressed, yes, but I'm skeptical whether
> it covers all.  The BE needs proper locking and refcounting that is
> coupled with the FE, I suppose.

So, this means, your helper patch seems OK,
but DPCM side needs more hack.

Mark
I'm happy to solve this issue, but I'm not good at DPCM.
If you can give me some help/advice, I can debug it.

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Tue, 17 Oct 2017 02:59:26 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san, Mark
> 
> Thank you for your feedback
> 
> > > Then, snd_pcm_relase() side will calls
> > > snd_pcm_detach_substream() and snd_pcm_dev_disconnect() side will die.
> > 
> > This must be also specific to DPCM.  Something is really wrong there.
> > 
> > Basically snd_pcm_dev_disconnect() and snd_pcm_release() can't race
> > since both are protected via pcm->open_mutex.  snd_pcm_stop() calls
> > are protected even more with substream lock.
> > 
> > > Mark's suggestion (= hiding BE) can solve this ?
> > 
> > Some of the issues might be addressed, yes, but I'm skeptical whether
> > it covers all.  The BE needs proper locking and refcounting that is
> > coupled with the FE, I suppose.
> 
> So, this means, your helper patch seems OK,
> but DPCM side needs more hack.
> 
> Mark
> I'm happy to solve this issue, but I'm not good at DPCM.
> If you can give me some help/advice, I can debug it.

It seems that the whole disconnect call can be dropped for the BE, as
it does nothing practically for the register callback, either.

Other than that, managing the object with the ALSA core device list is
still worth for tracking the memory release.

Below is the patch to do that.  It can be applied on top of the
previous two patches (snd_card_disconnect_sync() and
PCM-stop-at-disconnect).


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: pcm: Don't call register and disconnect callbacks for
 internal PCM

The internal PCM (aka DPCM backend PCM) doesn't need any registration
procedure, thus currently we bail out immediately at dev_register
callback.  Similarly, its counterpart, dev_disconnect callback, is
superfluous for the internal PCM.  For simplifying and avoiding the
conflicting disconnect call for internal PCM objects, this patch drops
dev_register and dev_disconnect callbacks for the internal ops.

The only uncertain thing by this action is whether skipping the PCM
state change to SNDRV_PCM_STATE_DISCONNECT for the internal PCM is
mandatory.  Looking through the current implementations, this doesn't
look so, hence dropping the whole dev_disconnect would make more
sense.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 7eadb7fd8074..1b073ed0b1f9 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -775,6 +775,9 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device,
 		.dev_register =	snd_pcm_dev_register,
 		.dev_disconnect = snd_pcm_dev_disconnect,
 	};
+	static struct snd_device_ops internal_ops = {
+		.dev_free = snd_pcm_dev_free,
+	};
 
 	if (snd_BUG_ON(!card))
 		return -ENXIO;
@@ -801,7 +804,8 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device,
 	if (err < 0)
 		goto free_pcm;
 
-	err = snd_device_new(card, SNDRV_DEV_PCM, pcm, &ops);
+	err = snd_device_new(card, SNDRV_DEV_PCM, pcm,
+			     internal ? &internal_ops : &ops);
 	if (err < 0)
 		goto free_pcm;
 
@@ -1099,8 +1103,6 @@ static int snd_pcm_dev_register(struct snd_device *device)
 	if (snd_BUG_ON(!device || !device->device_data))
 		return -ENXIO;
 	pcm = device->device_data;
-	if (pcm->internal)
-		return 0;
 
 	mutex_lock(&register_mutex);
 	err = snd_pcm_add(pcm);
@@ -1159,12 +1161,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
 			snd_pcm_stream_unlock_irq(substream);
 		}
 	}
-	if (!pcm->internal) {
-		pcm_call_notify(pcm, n_disconnect);
-	}
+
+	pcm_call_notify(pcm, n_disconnect);
 	for (cidx = 0; cidx < 2; cidx++) {
-		if (!pcm->internal)
-			snd_unregister_device(&pcm->streams[cidx].dev);
+		snd_unregister_device(&pcm->streams[cidx].dev);
 		free_chmap(&pcm->streams[cidx]);
 	}
 	mutex_unlock(&pcm->open_mutex);
-- 
2.14.2

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Mark Brown]

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

On Tue, Oct 17, 2017 at 12:37:42PM +0200, Takashi Iwai wrote:
> Kuninori Morimoto wrote:

> > I'm happy to solve this issue, but I'm not good at DPCM.
> > If you can give me some help/advice, I can debug it.

Honestly I'm not super good with DPCM either, Liam wrote that code and
I've never needed to delve too aggressively into it.

> It seems that the whole disconnect call can be dropped for the BE, as
> it does nothing practically for the register callback, either.

That feels like it's going to blow up on us at some point...  it's
probably right for most things though.

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

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

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi-san

> > It seems that the whole disconnect call can be dropped for the BE, as
> > it does nothing practically for the register callback, either.
> 
> That feels like it's going to blow up on us at some point...  it's
> probably right for most things though.

Wow !! Perfect !!

I tested your new "internal PCM" patch, and at least my kernel panic issue
on DPCM was solved !

Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

# I din't know that "internal" option was for DPCM backend PCM...

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Wed, 18 Oct 2017 04:08:44 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Takashi-san
> 
> > > It seems that the whole disconnect call can be dropped for the BE, as
> > > it does nothing practically for the register callback, either.
> > 
> > That feels like it's going to blow up on us at some point...  it's
> > probably right for most things though.
> 
> Wow !! Perfect !!
> 
> I tested your new "internal PCM" patch, and at least my kernel panic issue
> on DPCM was solved !
> 
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

OK, I'll keep the branch not merged to for-next yet for a while in
case we find any other breakage.  The plan is to merge these changes
for 4.15, so this will happen in a few days.

> # I din't know that "internal" option was for DPCM backend PCM...

That needs more documentation...


thanks,

Takashi

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Kuninori Morimoto]

Hi Takashi

> > I tested your new "internal PCM" patch, and at least my kernel panic issue
> > on DPCM was solved !
> > 
> > Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> OK, I'll keep the branch not merged to for-next yet for a while in
> case we find any other breakage.  The plan is to merge these changes
> for 4.15, so this will happen in a few days.

Thanks !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Sakamoto]

Hi guys,

On Oct 17 2017 09:59, Kuninori Morimoto wrote:>> Some of the issues might be addressed, yes, but I'm skeptical whether
>> it covers all.  The BE needs proper locking and refcounting that is
>> coupled with the FE, I suppose.
> 
> So, this means, your helper patch seems OK,
> but DPCM side needs more hack.

In my opinion, it's better for us to take enough proofs and explanations
when changing core functionalities.


Below patch for snd-dummy is to enable unbind operation to platform_device
for this module. (but it's too rough to apply mainline.) This works without
Iwai-san's two patches.

Let's try:
$ modprobe snd-dummy
$ aplay -D hw:Dummy /dev/null &
$ lsmod | grep dummy
$ echo -n snd_dummy.0 > /sys/bus/platform/drivers/snd_dummy/unbind
$ lsmod | grep dummy
$ cat /proc/asound/cards
$ modprobe -r snd-dummy
$ cat /proc/asound/cards

Actually, we have no addressed issues in these operations. The issue is
only on ALSA SoC part.

But I note that even if unbind works fine to shift state of sound devices
into disconnected, poll(2) call to ALSA control character devices does not
return (e.g. run 'amixer events'). I don't know exactly the cause yet...

======== 8< --------

>From 74bb5c45f0bf36e6487538088c654b88e1efb5b5 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Tue, 17 Oct 2017 17:58:47 +0900
Subject: [PATCH] ALSA: dummy: support unbind operation to shift state of sound
 devices to disconnected

---
 sound/drivers/dummy.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index c0939a0164a6..dcdefd2931c7 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -92,6 +92,7 @@ MODULE_PARM_DESC(hrtimer, "Use hrtimer as the timer source.");
 #endif
 
 static struct platform_device *devices[SNDRV_CARDS];
+static struct snd_card *cards[SNDRV_CARDS];
 
 #define MIXER_ADDR_MASTER	0
 #define MIXER_ADDR_LINE		1
@@ -1134,7 +1135,24 @@ static int snd_dummy_probe(struct platform_device *devptr)
 
 static int snd_dummy_remove(struct platform_device *devptr)
 {
-	snd_card_free(platform_get_drvdata(devptr));
+	struct snd_card *card = platform_get_drvdata(devptr);
+	struct snd_device *dev;
+	int i;
+
+	for (i = 0; i < SNDRV_CARDS; ++i) {
+		/* Temporary for module removal. */
+		if (devices[i] == devptr)
+			cards[i] = card;
+	}
+
+	/*
+	 * This shifts state of an instance of sound card into disconnected, but
+	 * don't wait till all of references to instances of sound devices are
+	 * released.
+	 */
+	list_for_each_entry_reverse(dev, &card->devices, list)
+		snd_device_disconnect(card, dev->device_data);
+
 	return 0;
 }
 
@@ -1178,8 +1196,23 @@ static void snd_dummy_unregister_all(void)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(devices); ++i)
-		platform_device_unregister(devices[i]);
+	for (i = 0; i < ARRAY_SIZE(devices); ++i) {
+		struct platform_device *devptr = devices[i];
+		struct snd_card *card = cards[i];
+
+		if (devptr == NULL)
+			continue;
+		if (card) {
+			/*
+			 * This can wait till the final reference to an
+			 * instance of sound card is released.
+			 */
+			snd_card_free(card);
+		}
+
+		platform_device_unregister(devptr);
+	}
+
 	platform_driver_unregister(&snd_dummy_driver);
 	free_fake_buffer();
 }
-- 
2.11.0

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Tue, 17 Oct 2017 16:25:10 +0200,
Takashi Sakamoto wrote:
> 
> Hi guys,
> 
> On Oct 17 2017 09:59, Kuninori Morimoto wrote:>> Some of the issues might be addressed, yes, but I'm skeptical whether
> >> it covers all.  The BE needs proper locking and refcounting that is
> >> coupled with the FE, I suppose.
> > 
> > So, this means, your helper patch seems OK,
> > but DPCM side needs more hack.
> 
> In my opinion, it's better for us to take enough proofs and explanations
> when changing core functionalities.
> 
> 
> Below patch for snd-dummy is to enable unbind operation to platform_device
> for this module. (but it's too rough to apply mainline.) This works without
> Iwai-san's two patches.
> 
> Let's try:
> $ modprobe snd-dummy
> $ aplay -D hw:Dummy /dev/null &
> $ lsmod | grep dummy
> $ echo -n snd_dummy.0 > /sys/bus/platform/drivers/snd_dummy/unbind
> $ lsmod | grep dummy
> $ cat /proc/asound/cards
> $ modprobe -r snd-dummy
> $ cat /proc/asound/cards
> 
> Actually, we have no addressed issues in these operations. The issue is
> only on ALSA SoC part.

Well, as I mentioned, a potential breakage would appear in the legacy
AC97 codec binding -- if it would do any hot-unplug.  But most of
legacy drivers work fine without my patches just because it has only
top-level unbind.

So, the top-level hot-unplug as you tested with the dummy driver works
as is.  The problem is only about unbinding the middle-layer
component, as mentioned in the patch.  It's found mostly in ASoC, but
not necessarily tied only with ASoC.


> But I note that even if unbind works fine to shift state of sound devices
> into disconnected, poll(2) call to ALSA control character devices does not
> return (e.g. run 'amixer events'). I don't know exactly the cause yet...

The disconnection doesn't close the device by itself (we can't), but
it replaces with the dummy ops so that it never touches the driver
except for closing.  That is, the device is in the idle state and just
accepts closing.


Takashi

> ======== 8< --------
> 
> >From 74bb5c45f0bf36e6487538088c654b88e1efb5b5 Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Tue, 17 Oct 2017 17:58:47 +0900
> Subject: [PATCH] ALSA: dummy: support unbind operation to shift state of sound
>  devices to disconnected
> 
> ---
>  sound/drivers/dummy.c | 39 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index c0939a0164a6..dcdefd2931c7 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -92,6 +92,7 @@ MODULE_PARM_DESC(hrtimer, "Use hrtimer as the timer source.");
>  #endif
>  
>  static struct platform_device *devices[SNDRV_CARDS];
> +static struct snd_card *cards[SNDRV_CARDS];
>  
>  #define MIXER_ADDR_MASTER	0
>  #define MIXER_ADDR_LINE		1
> @@ -1134,7 +1135,24 @@ static int snd_dummy_probe(struct platform_device *devptr)
>  
>  static int snd_dummy_remove(struct platform_device *devptr)
>  {
> -	snd_card_free(platform_get_drvdata(devptr));
> +	struct snd_card *card = platform_get_drvdata(devptr);
> +	struct snd_device *dev;
> +	int i;
> +
> +	for (i = 0; i < SNDRV_CARDS; ++i) {
> +		/* Temporary for module removal. */
> +		if (devices[i] == devptr)
> +			cards[i] = card;
> +	}
> +
> +	/*
> +	 * This shifts state of an instance of sound card into disconnected, but
> +	 * don't wait till all of references to instances of sound devices are
> +	 * released.
> +	 */
> +	list_for_each_entry_reverse(dev, &card->devices, list)
> +		snd_device_disconnect(card, dev->device_data);
> +
>  	return 0;
>  }
>  
> @@ -1178,8 +1196,23 @@ static void snd_dummy_unregister_all(void)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(devices); ++i)
> -		platform_device_unregister(devices[i]);
> +	for (i = 0; i < ARRAY_SIZE(devices); ++i) {
> +		struct platform_device *devptr = devices[i];
> +		struct snd_card *card = cards[i];
> +
> +		if (devptr == NULL)
> +			continue;
> +		if (card) {
> +			/*
> +			 * This can wait till the final reference to an
> +			 * instance of sound card is released.
> +			 */
> +			snd_card_free(card);
> +		}
> +
> +		platform_device_unregister(devptr);
> +	}
> +
>  	platform_driver_unregister(&snd_dummy_driver);
>  	free_fake_buffer();
>  }
> -- 
> 2.11.0
> 

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Sakamoto]

On Oct 17 2017 23:34, Takashi Iwai wrote:
> Well, as I mentioned, a potential breakage would appear in the legacy
> AC97 codec binding -- if it would do any hot-unplug.  But most of
> legacy drivers work fine without my patches just because it has only
> top-level unbind.

Yep. Recent drivers get no suffers from the issue because AC'97 is
enough ancient, at least for rsnd.

> So, the top-level hot-unplug as you tested with the dummy driver works
> as is.  The problem is only about unbinding the middle-layer
> component, as mentioned in the patch.  It's found mostly in ASoC, but
> not necessarily tied only with ASoC.

Are there any driver outside of ALSA SoC part to use the feature?
Actually, not. It's quite natural to assume that the feature is somehow
specialized for ALSA SoC part even it it's in ALSA PCM core.

And today I have little time to review the patch. Please check
in-reply-to field of my previous message. This is not a reply to yours.

>> But I note that even if unbind works fine to shift state of sound devices
>> into disconnected, poll(2) call to ALSA control character devices does not
>> return (e.g. run 'amixer events'). I don't know exactly the cause yet...
> 
> The disconnection doesn't close the device by itself (we can't), but
> it replaces with the dummy ops so that it never touches the driver
> except for closing.  That is, the device is in the idle state and just
> accepts closing.

You did misread. I just said that a call of poll(2) doesn't return.

1599 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
1600 {
1601         unsigned int mask;
1602         struct snd_ctl_file *ctl;
1603
1604         ctl = file->private_data;
1605         if (!ctl->subscribed)
1606                 return 0;
1607         poll_wait(file, &ctl->change_sleep, wait);

This should be awakened by below lines:

1775 static int snd_ctl_dev_disconnect(struct snd_device *device)
1776 {
1777         struct snd_card *card = device->device_data;
1778         struct snd_ctl_file *ctl;
1779
1780         read_lock(&card->ctl_files_rwlock);
1781         list_for_each_entry(ctl, &card->ctl_files, list) {
1782                 wake_up(&ctl->change_sleep);

As long as I observed, it doesn't even if the above line is executed.
(how odd...) I suspect to hit some other bugs such as buffer-overrun
depending on my environment. But it's out of my current plan and I
didn't investigate further. That's all of what I experience.


Thanks

Takashi Sakamoto

Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai]

On Tue, 17 Oct 2017 17:16:51 +0200,
Takashi Sakamoto wrote:
> 
> On Oct 17 2017 23:34, Takashi Iwai wrote:
> > Well, as I mentioned, a potential breakage would appear in the legacy
> > AC97 codec binding -- if it would do any hot-unplug.  But most of
> > legacy drivers work fine without my patches just because it has only
> > top-level unbind.
> 
> Yep. Recent drivers get no suffers from the issue because AC'97 is
> enough ancient, at least for rsnd.

Erm, no, what I meant is the legacy AC97 codec driver implementation,
i.e. the code in sound/pci/ac97/ac97_codec.c.  The driver using this
codec implementations are all non-ASoC.  But AFAIK most of its usage
is always with the hard-dependency, not via bus binding, so most
likely we don't see the issue in the legacy drivers, luckily or
unluckily.

> > So, the top-level hot-unplug as you tested with the dummy driver works
> > as is.  The problem is only about unbinding the middle-layer
> > component, as mentioned in the patch.  It's found mostly in ASoC, but
> > not necessarily tied only with ASoC.
> 
> Are there any driver outside of ALSA SoC part to use the feature?

If a middle-layer driver can unbind over bus or class, yes, any driver
code may suffer from the very same problem.  The soundbus of AOA
drivers likely has the same problem, for example, and I can imagine
some of media stuff has a similar problem, too.

> Actually, not. It's quite natural to assume that the feature is somehow
> specialized for ALSA SoC part even it it's in ALSA PCM core.

Please stop blaming as if it were only ASoC-specific.  It's the nature
of Linux driver model.  As long as you can unbind some partial
component, it has to sync with the rest for a proper removal.
The lack of such mechanism in ASoC is partly because ALSA core doesn't
provide the capability, and it's the very reason we're discussing
here.

> And today I have little time to review the patch. Please check
> in-reply-to field of my previous message. This is not a reply to yours.
>
> >> But I note that even if unbind works fine to shift state of sound devices
> >> into disconnected, poll(2) call to ALSA control character devices does not
> >> return (e.g. run 'amixer events'). I don't know exactly the cause yet...
> > 
> > The disconnection doesn't close the device by itself (we can't), but
> > it replaces with the dummy ops so that it never touches the driver
> > except for closing.  That is, the device is in the idle state and just
> > accepts closing.
> 
> You did misread. I just said that a call of poll(2) doesn't return.
> 
> 1599 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
> 1600 {
> 1601         unsigned int mask;
> 1602         struct snd_ctl_file *ctl;
> 1603
> 1604         ctl = file->private_data;
> 1605         if (!ctl->subscribed)
> 1606                 return 0;
> 1607         poll_wait(file, &ctl->change_sleep, wait);
> 
> This should be awakened by below lines:
> 
> 1775 static int snd_ctl_dev_disconnect(struct snd_device *device)
> 1776 {
> 1777         struct snd_card *card = device->device_data;
> 1778         struct snd_ctl_file *ctl;
> 1779
> 1780         read_lock(&card->ctl_files_rwlock);
> 1781         list_for_each_entry(ctl, &card->ctl_files, list) {
> 1782                 wake_up(&ctl->change_sleep);
> 
> As long as I observed, it doesn't even if the above line is executed.
> (how odd...)

Hm, then it's worth to check, but it's irrelevant from the issue we're
chasing.

> I suspect to hit some other bugs such as buffer-overrun
> depending on my environment.

How can such a thing happen...?

> But it's out of my current plan and I
> didn't investigate further. That's all of what I experience.

OK, thanks for reporting.


Takashi