[PATCH 0/3] ALSA: add snd_card_disconnect_sync()

[PATCH 0/3] ALSA: add snd_card_disconnect_sync()

[Kuninori Morimoto]

Hi Takashi-san, Mark

Current ALSA (SoC) might be kernel panic if user
unbinds sound driver during playback.

	> aplay xxx.wav &
	> echo xxxx > /sys/bus/platform/drivers/xxxx/unbind 

Main issue is we *can't* skip unbind with return -Exxx,
because unbind operation doesn't checks return value from each driver.
Thus, we *must* stop driver immediately when unbind.

1) is for ALSA
2) is for ALSA SoC
3) is for Renesas sound driver.

I think almost all driver want to have 3) type patch on .remove
but this patch-set doesn't care at this point.
Please let me know if they should have it.

Takashi Iwai (1):
  1) ALSA: add snd_card_disconnect_sync()

Kuninori Morimoto (2):
  2) ASoC: add snd_soc_card_disconnect_sync()
  3) ASoC: rsnd: call snd_soc_card_disconnect_sync() when remove

 include/sound/core.h     |  2 ++
 include/sound/soc.h      |  2 ++
 sound/core/init.c        | 18 ++++++++++++++++++
 sound/core/pcm.c         |  4 ++++
 sound/soc/sh/rcar/core.c |  2 ++
 sound/soc/soc-core.c     | 10 ++++++++++
 6 files changed, 38 insertions(+)

-- 
1.9.1



Best regards
---
Kuninori Morimoto

[PATCH 1/3] ALSA: add snd_card_disconnect_sync()

[Kuninori Morimoto]

From: Takashi Iwai <tiwai@suse.de>

In case of user unbind ALSA driver during playbacking/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, becasue
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 isconnection 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.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
Takashi-san

I created this patch and its git-log, but please fixup it
if needed.

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

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/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.c b/sound/core/pcm.c
index 7eadb7f..054e47a 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);
-- 
1.9.1

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

[Takashi Iwai]

On Wed, 11 Oct 2017 08:36:13 +0200,
Kuninori Morimoto wrote:
> 
> 
> From: Takashi Iwai <tiwai@suse.de>
> 
> In case of user unbind ALSA driver during playbacking/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, becasue
> 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 isconnection procedure at

disconnection

> 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.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> Takashi-san
> 
> I created this patch and its git-log, but please fixup it
> if needed.

I would split this again to two patches, one for adding
snd_card_disconnect_sync(), and another for adding snd_pcm_stop() call
at snd_pcm_dev_disconnect().

Basically the latter is for the drivers that don't handle the PCM stop
at disconnection explicitly, and it's not mandatory when the driver's
pcm dev_disconnect callback handles it.

Also, since these changes are about ALSA core, and at least the
PCM-stop one is intrusive and may have influences on the behavior of
other drivers, I'd like to test with usb-audio before landing to ASoC
world.  Once when confirmed, I'll publish a persistent branch
containing these API changes and let Mark pull it.

More comments below:

> 
>  include/sound/core.h |  2 ++
>  sound/core/init.c    | 18 ++++++++++++++++++
>  sound/core/pcm.c     |  4 ++++
>  3 files changed, 24 insertions(+)
> 
> 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/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)

We need the documentation for the public API function.

> +{
> +	DECLARE_COMPLETION(comp);

This completion isn't used here, no?

> +
> +	if (snd_card_disconnect(card) < 0)
> +		return;

OK, we should bail out here, but since we can't handle the error, it's
better to warn explicitly.  Not necessarily WARN_ON() but give a fat
error message.


thanks,

Takashi

> +
> +	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.c b/sound/core/pcm.c
> index 7eadb7f..054e47a 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);
> -- 
> 1.9.1
> 

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

[Kuninori Morimoto]

Hi Takashi

> > Takashi-san
> > 
> > I created this patch and its git-log, but please fixup it
> > if needed.
> 
> I would split this again to two patches, one for adding
> snd_card_disconnect_sync(), and another for adding snd_pcm_stop() call
> at snd_pcm_dev_disconnect().
> 
> Basically the latter is for the drivers that don't handle the PCM stop
> at disconnection explicitly, and it's not mandatory when the driver's
> pcm dev_disconnect callback handles it.
> 
> Also, since these changes are about ALSA core, and at least the
> PCM-stop one is intrusive and may have influences on the behavior of
> other drivers, I'd like to test with usb-audio before landing to ASoC
> world.  Once when confirmed, I'll publish a persistent branch
> containing these API changes and let Mark pull it.

Thanks.
I will re-post [2/3] [3/3] part patch on top of your persistent branch.

Best regards
---
Kuninori Morimoto

[PATCH 2/3] ASoC: add snd_soc_card_disconnect_sync()

[Kuninori Morimoto]

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

snd_soc_card_disconnect_sync() is ASoC version of
snd_card_disconnect_sync()

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |  2 ++
 sound/soc/soc-core.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index d05ddc5..b6ee692 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/soc/soc-core.c b/sound/soc/soc-core.c
index 138669a..59385f6 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
-- 
1.9.1

Re: [PATCH 2/3] ASoC: add snd_soc_card_disconnect_sync()

[Mark Brown]

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

On Wed, Oct 11, 2017 at 06:37:06AM +0000, Kuninori Morimoto wrote:

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

Is this the right name for ASoC or should it just be
snd_soc_disconnect_sync()?  It'll work for any device that's in a card.

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

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

Re: [PATCH 2/3] ASoC: add snd_soc_card_disconnect_sync()

[Kuninori Morimoto]

Hi Mark

> > +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);
> 
> Is this the right name for ASoC or should it just be
> snd_soc_disconnect_sync()?  It'll work for any device that's in a card.

Ahh, indeed.
snd_soc_disconnect_sync() is better naming.
I will fix name in v2 (Based on Takashi's branch)

Best regards
---
Kuninori Morimoto

[PATCH 3/3] ASoC: rsnd: call snd_soc_card_disconnect_sync() when remove

[Kuninori Morimoto]

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

Renesas R-Car sound driver should be stopped when unbind during
playbacking/capturing. Otherwise clock open/close counter mismatch
happen.

One note is that we can't skip from remove function (= return -Exxx)
in such case if user used unbind. Because unbind function doesn't
check return value from each driver's remove function.
This means we must to stop and remove driver in remove function.

Now ASoC has snd_soc_card_disconnect_sync() for this purpose.
Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 15f0929..77003a7 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1500,6 +1500,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);
 
-- 
1.9.1

Re: [PATCH 3/3] ASoC: rsnd: call snd_soc_card_disconnect_sync() when remove

[Kuninori Morimoto]

Hi Mark

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Renesas R-Car sound driver should be stopped when unbind during
> playbacking/capturing. Otherwise clock open/close counter mismatch
> happen.
> 
> One note is that we can't skip from remove function (= return -Exxx)
> in such case if user used unbind. Because unbind function doesn't
> check return value from each driver's remove function.
> This means we must to stop and remove driver in remove function.
> 
> Now ASoC has snd_soc_card_disconnect_sync() for this purpose.
> Let's use it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---

I'm sorry, but, I noticed this patch is not so good.
I want to post v2 patch if I could response from
Takashi-san about [1/3].

Best regards
---
Kuninori Morimoto