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

[Takashi Iwai <tiwai@suse.de>]

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
>