[alsa-devel] [PATCH] ASoC: rsnd: stop all working stream when .remove
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Driver should stop all working stream when .remove timing. Current Renesas sound driver is assuming that all stream was stopped when .remove but it was wrong. This patch stops all working stream when .remove, otherwise kernel will get damage for example in below case. Special thanks to Truong, Hiep
> cd /sys/bus/platform/drivers/rcar_sound > aplay xxx.wav & > echo ec500000.sound > unbind
Reported-by: Hiep Cao Minh cm-hiep@jinso.co.jp Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/core.c | 68 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 1071332..1bf472f 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -197,12 +197,6 @@ void rsnd_mod_interrupt(struct rsnd_mod *mod, } }
-int rsnd_io_is_working(struct rsnd_dai_stream *io) -{ - /* see rsnd_dai_stream_init/quit() */ - return !!io->substream; -} - int rsnd_runtime_channel_original(struct rsnd_dai_stream *io) { struct snd_pcm_runtime *runtime = rsnd_io_to_runtime(io); @@ -610,20 +604,24 @@ struct rsnd_dai_stream *rsnd_rdai_to_io(struct rsnd_dai *rdai, return &rdai->capture; }
-static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd, - struct snd_soc_dai *dai) +int rsnd_io_is_working(struct rsnd_dai_stream *io) +{ + /* see rsnd_dai_stream_init/quit() */ + return !!io->substream; +} + +#define rsnd_io_start(priv, io, sub) rsnd_io_operation(priv, io, sub) +#define rsnd_io_stop(priv, io) rsnd_io_operation(priv, io, NULL) +static int rsnd_io_operation(struct rsnd_priv *priv, + struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream) { - struct rsnd_priv *priv = rsnd_dai_to_priv(dai); - struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai); - struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream); int ret; unsigned long flags;
spin_lock_irqsave(&priv->lock, flags);
- switch (cmd) { - case SNDRV_PCM_TRIGGER_START: - case SNDRV_PCM_TRIGGER_RESUME: + if (substream) { rsnd_dai_stream_init(io, substream);
ret = rsnd_dai_call(init, io, priv); @@ -638,9 +636,7 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd, if (ret < 0) goto dai_trigger_end;
- break; - case SNDRV_PCM_TRIGGER_STOP: - case SNDRV_PCM_TRIGGER_SUSPEND: + } else { ret = rsnd_dai_call(irq, io, priv, 0);
ret |= rsnd_dai_call(stop, io, priv); @@ -648,9 +644,6 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd, ret |= rsnd_dai_call(quit, io, priv);
rsnd_dai_stream_quit(io); - break; - default: - ret = -EINVAL; }
dai_trigger_end: @@ -659,6 +652,30 @@ static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd, return ret; }
+static int rsnd_soc_dai_trigger(struct snd_pcm_substream *substream, int cmd, + struct snd_soc_dai *dai) +{ + struct rsnd_priv *priv = rsnd_dai_to_priv(dai); + struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai); + struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream); + int ret; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + ret = rsnd_io_start(priv, io, substream); + break; + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + ret = rsnd_io_stop(priv, io); + break; + default: + ret = -EINVAL; + } + + return ret; +} + static int rsnd_soc_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai); @@ -1477,6 +1494,17 @@ static int rsnd_remove(struct platform_device *pdev) }; int ret = 0, i;
+ /* + * Stop all working io + */ + for_each_rsnd_dai(rdai, priv, i) { + if (rsnd_io_is_working(&rdai->playback)) + rsnd_io_stop(priv, &rdai->playback); + + if (rsnd_io_is_working(&rdai->capture)) + rsnd_io_stop(priv, &rdai->capture); + } + pm_runtime_disable(&pdev->dev);
for_each_rsnd_dai(rdai, priv, i) {
On Fri, 01 Sep 2017 06:34:48 +0200, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Driver should stop all working stream when .remove timing. Current Renesas sound driver is assuming that all stream was stopped when .remove but it was wrong. This patch stops all working stream when .remove, otherwise kernel will get damage for example in below case. Special thanks to Truong, Hiep
cd /sys/bus/platform/drivers/rcar_sound aplay xxx.wav & echo ec500000.sound > unbind
Reported-by: Hiep Cao Minh cm-hiep@jinso.co.jp Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Takashi
Hi Takashi
Driver should stop all working stream when .remove timing. Current Renesas sound driver is assuming that all stream was stopped when .remove but it was wrong. This patch stops all working stream when .remove, otherwise kernel will get damage for example in below case. Special thanks to Truong, Hiep
cd /sys/bus/platform/drivers/rcar_sound aplay xxx.wav & echo ec500000.sound > unbind
Reported-by: Hiep Cao Minh cm-hiep@jinso.co.jp Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Thanks. This path main sync is for clk ON/OFF
Best regards --- Kuninori Morimoto
On Fri, 01 Sep 2017 09:48:52 +0200, Kuninori Morimoto wrote:
Hi Takashi
Driver should stop all working stream when .remove timing. Current Renesas sound driver is assuming that all stream was stopped when .remove but it was wrong. This patch stops all working stream when .remove, otherwise kernel will get damage for example in below case. Special thanks to Truong, Hiep
cd /sys/bus/platform/drivers/rcar_sound aplay xxx.wav & echo ec500000.sound > unbind
Reported-by: Hiep Cao Minh cm-hiep@jinso.co.jp Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Thanks. This path main sync is for clk ON/OFF
Hm, but it's managed as PCM trigger, no? How can the rsnd_io_is_working() return true after PCM streams are stopped?
Takashi
Hi Takasi-san
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Thanks. This path main sync is for clk ON/OFF
Hm, but it's managed as PCM trigger, no? How can the rsnd_io_is_working() return true after PCM streams are stopped?
It is based on PCM trigger, thus, it returns false if PCM streams are stopped.
This driver calls clk_get() when PCM started, and clk_put() when stopped. And it calles clk_enable() on .probe, and clk_disable() on .remove.
My problem is that user unbinds driver during Sound playing, this means clk_get() is called, but clk_put() is not called. Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed. Is this clear answer for you ?
On Mon, 04 Sep 2017 19:44:36 +0200, Kuninori Morimoto wrote:
Hi Takasi-san
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Thanks. This path main sync is for clk ON/OFF
Hm, but it's managed as PCM trigger, no? How can the rsnd_io_is_working() return true after PCM streams are stopped?
It is based on PCM trigger, thus, it returns false if PCM streams are stopped.
This driver calls clk_get() when PCM started, and clk_put() when stopped. And it calles clk_enable() on .probe, and clk_disable() on .remove.
My problem is that user unbinds driver during Sound playing, this means clk_get() is called, but clk_put() is not called.
Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed. Is this clear answer for you ?
This isn't something you shouldn't fiddle with the codec layer. If the driver gets removed during the operation, you have to cancel the operation and sync with it in a proper way, then proceed the rest of the remove, not only a codec-specific resource management.
Admittedly, there is no common infrastructure for that. But it doesn't mean that each codec driver should do its own hack.
I can imagine a way like calling the card disconnect/free at codec remove, so that it can sync with the whole stop operation before doing the rest. This should be ignored when the code path is from the card removal -- e.g. checking card->shutdown flag.
thanks,
Takashi
On Mon, 04 Sep 2017 20:43:19 +0200, Takashi Iwai wrote:
On Mon, 04 Sep 2017 19:44:36 +0200, Kuninori Morimoto wrote:
Hi Takasi-san
The lack of stop sync is a known problem in the ALSA PCM infrastructure. The standard idiom is to do sync at both prepare and hw_free (or close) callbacks.
Thanks. This path main sync is for clk ON/OFF
Hm, but it's managed as PCM trigger, no? How can the rsnd_io_is_working() return true after PCM streams are stopped?
It is based on PCM trigger, thus, it returns false if PCM streams are stopped.
This driver calls clk_get() when PCM started, and clk_put() when stopped. And it calles clk_enable() on .probe, and clk_disable() on .remove.
My problem is that user unbinds driver during Sound playing, this means clk_get() is called, but clk_put() is not called.
Then, .remove will call clk_disable(), but clk_put() is not yet called. Then, kernel indicates clk user count mismatch. This patch calls missing PCM stop (= clk_put()) position function if needed. Is this clear answer for you ?
This isn't something you shouldn't fiddle with the codec layer. If the driver gets removed during the operation, you have to cancel the operation and sync with it in a proper way, then proceed the rest of the remove, not only a codec-specific resource management.
Admittedly, there is no common infrastructure for that. But it doesn't mean that each codec driver should do its own hack.
I can imagine a way like calling the card disconnect/free at codec remove, so that it can sync with the whole stop operation before doing the rest. This should be ignored when the code path is from the card removal -- e.g. checking card->shutdown flag.
Here I mentioned the codec driver, but it's applied to each lower-level component. It'd need some graceful way to communicate with the top-level card to assure the removal of the component.
Takashi
Hi Takashi-san
Thank you for your feedback
This isn't something you shouldn't fiddle with the codec layer. If the driver gets removed during the operation, you have to cancel the operation and sync with it in a proper way, then proceed the rest of the remove, not only a codec-specific resource management.
(snip)
Here I mentioned the codec driver, but it's applied to each lower-level component. It'd need some graceful way to communicate with the top-level card to assure the removal of the component.
I agree. I can't access to source code now (I'm in business-trip), but my head-acke is that kernel doesn't check return value from .remove when unbind case. Thus, we can't "cancel" remove operation. I'm happy if you can confirm it.
On Tue, 05 Sep 2017 09:40:11 +0200, Kuninori Morimoto wrote:
Hi Takashi-san
Thank you for your feedback
This isn't something you shouldn't fiddle with the codec layer. If the driver gets removed during the operation, you have to cancel the operation and sync with it in a proper way, then proceed the rest of the remove, not only a codec-specific resource management.
(snip)
Here I mentioned the codec driver, but it's applied to each lower-level component. It'd need some graceful way to communicate with the top-level card to assure the removal of the component.
I agree. I can't access to source code now (I'm in business-trip), but my head-acke is that kernel doesn't check return value from .remove when unbind case. Thus, we can't "cancel" remove operation. I'm happy if you can confirm it.
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.
thanks,
Takashi
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. 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 ?
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 ?
Morimoto
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.
Takashi
Hi Takashi-san
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.
Oh, yes, indeed. Thanks.
On Tue, Sep 05, 2017 at 11:33:42AM +0200, Takashi Iwai wrote:
Kuninori Morimoto wrote:
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.
As far as the individual ASoC drivers are concerned this should just be something that happens at component deregistration time like the DAPM power down and so on.
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);
On Tue, 05 Sep 2017 13:35:58 +0200, Takashi Iwai wrote:
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.
An obvious spinlock was forgotten in one place, the revised patch below.
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..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);
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);
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
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
Morimoto-san,
sorry for the late reply. It took time until I digest all pending stuff after vacation.
On Wed, 27 Sep 2017 07:14:21 +0200, Kuninori Morimoto wrote:
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
That needs a bit more investigation. When the device is disconnected, not all drivers expect that further PCM operations are done for non-existing devices. We might need either some flag to allow/prefer the stop-after-disconnection, or rethink whether we should actually stop at snd_pcm_dev_disconnect() like below.
- ASoC version of snd_card_disconnect_sync()
- user driver (= rsnd) uses snd_soc_card_disconnect_sync()
Yes, these should be split.
thanks,
Takashi
--- diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7eadb7fd8074..054e47ad23ed 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);
Hi Takashi-san
sorry for the late reply. It took time until I digest all pending stuff after vacation.
no problem
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
That needs a bit more investigation. When the device is disconnected, not all drivers expect that further PCM operations are done for non-existing devices. We might need either some flag to allow/prefer the stop-after-disconnection, or rethink whether we should actually stop at snd_pcm_dev_disconnect() like below.
Thank you for below patch. I will check/test it
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 7eadb7fd8074..054e47ad23ed 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);
Hi Takashi-san, again
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
That needs a bit more investigation. When the device is disconnected, not all drivers expect that further PCM operations are done for non-existing devices. We might need either some flag to allow/prefer the stop-after-disconnection, or rethink whether we should actually stop at snd_pcm_dev_disconnect() like below.
Thank you for below patch. I will check/test it
It solved my issue (without strange status magic :). I posted this patch-set. [1/3] is your patch but I posted. Please check (and fix) it
Best regards --- Kuninori Morimoto
On Tue, Sep 05, 2017 at 01:35:58PM +0200, Takashi Iwai wrote:
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.
I'd say that in ASoC we'd have the call to this in the core rather than wrapping it, I'd expect the first thing the drivers do is to unregister the component.
On Tue, 05 Sep 2017 16:04:00 +0200, Mark Brown wrote:
On Tue, Sep 05, 2017 at 01:35:58PM +0200, Takashi Iwai wrote:
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.
I'd say that in ASoC we'd have the call to this in the core rather than wrapping it, I'd expect the first thing the drivers do is to unregister the component.
Yes, that makes sense.
Takashi
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Takashi Iwai