On Wed, 11 Oct 2017 08:36:13 +0200,
Kuninori Morimoto wrote:
>
>
> From: Takashi Iwai <tiwai@xxxxxxx>
>
> 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@xxxxxxx>
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
