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

Kuninori Morimoto
2017-09-26T08:21:34Z

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