[alsa-devel] [RFC][PATCH] ASoC: soc-core: verify Sound Card normality
Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform drivers. If system uses Kernel modules, we can disable these drivers by using rmmod command. In such case, we can't disable CPU/Codec/Platform driver without disabling Sound Card driver.
But on the other hand, we can disable these drivers by using unbind command. In such case, we can disable these drivers randomly. In this case, we can create dirty Sound Card which is missing necessary components.
(1) If user disabled Sound Card first, but did nothing to other drivers, user can't use Sound because Sound Card is no longer exists. (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing to Sound Card, user still be able to use Sound Card, because dirty Sound Card which is missing necessary components still exists. In this case, Sound system will be crashed if user started sound playback/capture. But we can't block such random unbind now.
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- Mark, Lars-Peter
This is still RFC, but I think we need to consider about this issue. I tested this patch on my system, and it can block to sound card crash for me
include/sound/soc.h | 6 ++++-- sound/soc/soc-core.c | 12 +++++++++++- sound/soc/soc-pcm.c | 20 ++++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 316fdce..4813fc5 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1106,8 +1106,6 @@ struct snd_soc_card { struct mutex mutex; struct mutex dapm_mutex;
- bool instantiated; - int (*probe)(struct snd_soc_card *card); int (*late_probe)(struct snd_soc_card *card); int (*remove)(struct snd_soc_card *card); @@ -1197,6 +1195,10 @@ struct snd_soc_card { u32 pop_time;
void *drvdata; + + /* bit field */ + u32 instantiated:1; + u32 dirty:1; };
/* SoC machine DAI configuration, glues a codec and cpu DAI together */ diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 07e4eec..a18e106 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2882,7 +2882,15 @@ int snd_soc_unregister_card(struct snd_soc_card *card) { if (card->instantiated) { card->instantiated = false; - snd_soc_dapm_shutdown(card); + if (!card->dirty) { + /* + * In case of unbind, CPU/Codec/Platform component can + * be unbinded *before* Card unbind. In such case + * (= card->dirty) Card connected DAPMs are already + * doesn't exist. + */ + snd_soc_dapm_shutdown(card); + } soc_cleanup_card_resources(card); dev_dbg(card->dev, "ASoC: Unregistered card '%s'\n", card->name); } @@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
static void snd_soc_component_del_unlocked(struct snd_soc_component *component) { + if (component->card) + component->card->dirty = 1; list_del(&component->list); }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index efc5831..c861aba 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -455,6 +455,16 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) const char *codec_dai_name = "multicodec"; int i, ret = 0;
+ if (rtd->card->dirty) { + /* + * In case of unbind, each CPU/Codec/Platform component can + * be unbinded randomly. In such case (= card->dirty) + * Sound Card is no longer safety. Don't open it. + */ + dev_warn(cpu_dai->dev, "Sound SoC Card is dirty, Recheck your system\n"); + return -ENXIO; + } + pinctrl_pm_select_default_state(cpu_dai->dev); for (i = 0; i < rtd->num_codecs; i++) pinctrl_pm_select_default_state(rtd->codec_dais[i]->dev); @@ -2577,6 +2587,16 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) int ret; int stream = fe_substream->stream;
+ if (fe->card->dirty) { + /* + * In case of unbind, each CPU/Codec/Platform component can + * be unbinded randomly. In such case (= card->dirty) + * Sound Card is no longer safety. Don't open it. + */ + dev_warn(fe->dev, "Sound SoC Card is dirty, Recheck your system\n"); + return -ENXIO; + } + mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); fe->dpcm[stream].runtime = fe_substream->runtime;
Hi Morimoto-san,
On Wed, Mar 29, 2017 at 4:45 AM, Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform drivers. If system uses Kernel modules, we can disable these drivers by using rmmod command. In such case, we can't disable CPU/Codec/Platform driver without disabling Sound Card driver.
But on the other hand, we can disable these drivers by using unbind command. In such case, we can disable these drivers randomly. In this case, we can create dirty Sound Card which is missing necessary components.
(1) If user disabled Sound Card first, but did nothing to other drivers, user can't use Sound because Sound Card is no longer exists. (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing to Sound Card, user still be able to use Sound Card, because dirty Sound Card which is missing necessary components still exists. In this case, Sound system will be crashed if user started sound playback/capture. But we can't block such random unbind now.
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
--- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c
@@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
static void snd_soc_component_del_unlocked(struct snd_soc_component *component) {
if (component->card)component->card->dirty = 1;
Currently this is the only assignment to .dirty. Can this be undone later, when the driver is bound again?
list_del(&component->list);}
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert
Current ALSA SoC Sound Card basically consists of CPU/Codec/Platform drivers. If system uses Kernel modules, we can disable these drivers by using rmmod command. In such case, we can't disable CPU/Codec/Platform driver without disabling Sound Card driver.
But on the other hand, we can disable these drivers by using unbind command. In such case, we can disable these drivers randomly. In this case, we can create dirty Sound Card which is missing necessary components.
(1) If user disabled Sound Card first, but did nothing to other drivers, user can't use Sound because Sound Card is no longer exists. (2) If user disabled CPU/Codec/Platform driver randomly, but did nothing to Sound Card, user still be able to use Sound Card, because dirty Sound Card which is missing necessary components still exists. In this case, Sound system will be crashed if user started sound playback/capture. But we can't block such random unbind now.
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
--- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c
@@ -3236,6 +3244,8 @@ static void snd_soc_component_cleanup(struct snd_soc_component *component)
static void snd_soc_component_del_unlocked(struct snd_soc_component *component) {
if (component->card)component->card->dirty = 1;Currently this is the only assignment to .dirty. Can this be undone later, when the driver is bound again?
Does this "undone" mean "card->dirty = 0" ?
Card itself will be freed if Card was unbind. Thus, card will be re-created if it was bind again. .dirty will be automatically 0 cleared.
Best regards --- Kuninori Morimoto
On Wed, Mar 29, 2017 at 02:45:37AM +0000, Kuninori Morimoto wrote:
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
I think this is a good direction to at least start to mitigate these problems (which we really should be doing) and hopefully make it easier to do further improvements in future. There's obviously more places where we should be checking the flag (controls for example) but they can be added later. One thing I would like to see is instead of setting the flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
Hi Mark
Thank you for your feedback
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
I think this is a good direction to at least start to mitigate these problems (which we really should be doing) and hopefully make it easier to do further improvements in future. There's obviously more places where we should be checking the flag (controls for example) but they can be added later. One thing I would like to see is instead of setting the flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
I guess you are pointing about snd_soc_dapm_shutdown() in snd_soc_unregister_card() ? If so, I agree. Actually, I don't like this kind of adhoc handling. I want to explain about it.
Order [1] Disable Sound Card -> Disable CPU/Codec/Platform or do nothing [2] Disable CPU/Codec/Platform -> Disable Card or do nothing
Operation 1) shutdown all Card connected DAPM 2) cleanup Card resource.
In case of order [1], operation 1) -> 2) is no problem, because all card connected DAPM exists on Card. But, in case of order [2], operation 1) will try to call disconnected DAPM. The DAPM disconnection from Card is done by snd_soc_dapm_free() which is called from soc_remove_component(). This soc_remove_component() will be called with "remove_order" from operation 2). One note is that reordering operation to 2) -> 1) will avoid this crash issue, but it is no meaning, because 1) will do nothing ;)
Here, other solution is calling soc_remove_component() without "remove_order" or calling snd_soc_dapm_free() when CPU/Codec/Platform are disabled. But, 1 bad-point is that disabled CPU/Codec/Platform have no chance to be called about snd_soc_dapm_set_bias_level() (= 1) operation). Of course, adhoc function can solve this issue too.
If you are OK, I can work for this idea and remove adhoc operation from snd_soc_unregister_card()
Best regards --- Kuninori Morimoto
On Thu, 30 Mar 2017 23:53:34 +0200, Mark Brown wrote:
On Wed, Mar 29, 2017 at 02:45:37AM +0000, Kuninori Morimoto wrote:
To avoid Sound Card crash in (2) case, what we can do now is, add dirty flag on Sound Card, and avoid to open Sound Card. This patch solved this issue.
I think this is a good direction to at least start to mitigate these problems (which we really should be doing) and hopefully make it easier to do further improvements in future. There's obviously more places where we should be checking the flag (controls for example) but they can be added later. One thing I would like to see is instead of setting the flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
BTW, ALSA core has snd_card_disconnect() that does this kind of shut-up from user-space. It was introduced for hot-unplug, but basically unbinding is the software hot-unplug. So, if ASoC won't rebind a once-unbound component, you can simply call snd_card_disconnect() at the component unbinding time to assure that no further user actions can be done.
Takashi
Hi Takashi-san
I think this is a good direction to at least start to mitigate these problems (which we really should be doing) and hopefully make it easier to do further improvements in future. There's obviously more places where we should be checking the flag (controls for example) but they can be added later. One thing I would like to see is instead of setting the flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
BTW, ALSA core has snd_card_disconnect() that does this kind of shut-up from user-space. It was introduced for hot-unplug, but basically unbinding is the software hot-unplug. So, if ASoC won't rebind a once-unbound component, you can simply call snd_card_disconnect() at the component unbinding time to assure that no further user actions can be done.
Thanks. I checked about snd_card_disconnect(), and it will be called from snd_card_free(). And it will be called from snd_soc_unregister_card() So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform were unregsiterd.
This method also solve random unbind/bind Oops too. Here, random unbind/bind example is that expected correct operation is unbind all CPU/Codec/Platfrom/Card, and then, bind all CPU/Codec/Platfrom/Card again. (here unbind order can be random) But this case, we will get Oops if unbind Codec -> bind Codec -> unbind Card. Using snd_soc_unregister_card() can solve this issue too.
Best regards --- Kuninori Morimoto
On Mon, 03 Apr 2017 08:29:34 +0200, Kuninori Morimoto wrote:
Hi Takashi-san
I think this is a good direction to at least start to mitigate these problems (which we really should be doing) and hopefully make it easier to do further improvements in future. There's obviously more places where we should be checking the flag (controls for example) but they can be added later. One thing I would like to see is instead of setting the flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
BTW, ALSA core has snd_card_disconnect() that does this kind of shut-up from user-space. It was introduced for hot-unplug, but basically unbinding is the software hot-unplug. So, if ASoC won't rebind a once-unbound component, you can simply call snd_card_disconnect() at the component unbinding time to assure that no further user actions can be done.
Thanks. I checked about snd_card_disconnect(), and it will be called from snd_card_free(). And it will be called from snd_soc_unregister_card()
Yes, snd_card_free() assures the disconnection at first, syncs the all settled down, then releases the resources.
So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform were unregsiterd.
In theory yes, but you should be careful to do so, e.g. make sure that it won't be called again by the removal/unbind of other components / drivers.
I suggested snd_card_disconnect() because it doesn't release resources by itself, but it just disconnects from the further accesses. So, double-free won't happen in this case. It makes the hotunplug safer as long as the drivers manage the resource releases properly.
Takashi
This method also solve random unbind/bind Oops too. Here, random unbind/bind example is that expected correct operation is unbind all CPU/Codec/Platfrom/Card, and then, bind all CPU/Codec/Platfrom/Card again. (here unbind order can be random) But this case, we will get Oops if unbind Codec -> bind Codec -> unbind Card. Using snd_soc_unregister_card() can solve this issue too.
Best regards
Kuninori Morimoto
Hi Takashi-san
So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform were unregsiterd.
In theory yes, but you should be careful to do so, e.g. make sure that it won't be called again by the removal/unbind of other components / drivers.
I suggested snd_card_disconnect() because it doesn't release resources by itself, but it just disconnects from the further accesses. So, double-free won't happen in this case. It makes the hotunplug safer as long as the drivers manage the resource releases properly.
I had checked many unbind/bind pattern/order on 2nd [RFC] patch which I posted. At first, I believe Oops on unbind/bind issue was solved on it. 2nd, if my understanding was correct, it doesn't have double-free issue, or something like that. But, I'm not 100% sure about 2nd, thus it has [RFC] on patch.
Best regards --- Kuninori Morimoto
On Mon, 03 Apr 2017 10:26:05 +0200, Kuninori Morimoto wrote:
Hi Takashi-san
So, we can call snd_soc_unregister_card() whenever CPU/Codec/Platform were unregsiterd.
In theory yes, but you should be careful to do so, e.g. make sure that it won't be called again by the removal/unbind of other components / drivers.
I suggested snd_card_disconnect() because it doesn't release resources by itself, but it just disconnects from the further accesses. So, double-free won't happen in this case. It makes the hotunplug safer as long as the drivers manage the resource releases properly.
I had checked many unbind/bind pattern/order on 2nd [RFC] patch which I posted. At first, I believe Oops on unbind/bind issue was solved on it. 2nd, if my understanding was correct, it doesn't have double-free issue, or something like that. But, I'm not 100% sure about 2nd, thus it has [RFC] on patch.
Ah, I see that snd_soc_unregister_card() has the check of card->instantiated, so it should be fine to call multiple times.
thanks,
Takashi
On Mon, Apr 03, 2017 at 10:37:42AM +0200, Takashi Iwai wrote:
Ah, I see that snd_soc_unregister_card() has the check of card->instantiated, so it should be fine to call multiple times.
Yeah, we'd run into that often enough that it's worth handling nicely.
On Fri, Mar 31, 2017 at 09:48:02AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
flag directly when we see a problem call a function to do it. That way if we want to improve things in the future we can do that without having to update the callers again.
BTW, ALSA core has snd_card_disconnect() that does this kind of shut-up from user-space. It was introduced for hot-unplug, but basically unbinding is the software hot-unplug. So, if ASoC won't rebind a once-unbound component, you can simply call snd_card_disconnect() at the component unbinding time to assure that no further user actions can be done.
Ah, that's exactly the sort of improvement I was thinking of!
participants (4)
-
Geert Uytterhoeven -
Kuninori Morimoto -
Mark Brown -
Takashi Iwai