| Message ID | 874lsnniv3.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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) {