[alsa-devel] [PATCH] ASoC: rsnd: stop all working stream when .remove

[Takashi Iwai]

On Tue, 05 Sep 2017 11:33:42 +0200,
Takashi Iwai wrote:
> 
> On Tue, 05 Sep 2017 10:58:37 +0200,
> Kuninori Morimoto wrote:
> > 
> > Hi Takashi-san
> > 
> > Thank you for your feedback
> > 
> > > Right, you can't cancel or return an error at that point.
> > > That is, you'd need to sync (wait) until the all top-level operations are canceled at remove callback.
> > >
> > > For example, snd_card_free() processes the disconnection 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, you'd 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.
> > 
> > OK, it needs ALSA SoC framework side new feature.
> 
> Not only ASoC but also in all ALSA component generally.
> The component-level hot unplug isn't implemented yet properly.
> 
> > But can I confirm current situation ?
> > 
> > In ALSA SoC, it has Card/CPU/Codec/Platform drivers, and we can unbind these randomly.
> > Now, if I unbind CPU first, it checks connected Card, and will disconnect it if needed (Then, other drivers are as-is).
> > Because of this, Card will be disconnected automatically, and we can't use it again if user didn't remove all other
> > remaining drivers and re-bind all drivers again. This is current ALSA SoC I think.
> > 
> > If my understanding was correct, your idea is that we want to call remove function for all connected drivers somehow.
> > And then, Card want to wait all drivers are removed. Correct ?
> 
> Right.  Unless we really want to support the hog-plug/unplug of each
> component, it'd be more straightforward to do the full hot-unplug upon
> every component unbind action.
> 
> > I'm happy to work for it.
> > But adding new unplug feature is for sync with all "drivers", and this patch is sync for "clk" for my CPU driver.
> > Can we separate these ?
> 
> It belongs with the same thing.  Basically you're tweaking clk per PCM
> stream status.  By handling the full hot-plug properly, the PCM stream
> is assured to be stopped, thus you don't have to fiddle with clk in
> the remove callback at all.

So my idea is something like below (totally untested): call
snd_card_disconnect_sync() at the remove call of each component at the
beginning.
That assures stopping all pending operations and syncs with the file
releases.  For ASoC, we may want to wrap it with ASoC structs, but you
can have an idea by this patch.


Takashi

---
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..d8b556911d0e 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,15 @@ 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);
+	wait_event_lock_irq(card->remove_sleep,
+			    list_empty(&card->files_list),
+			    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 +967,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);