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

Kuninori Morimoto
2017-09-27T05:14:30Z

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